When you open a securable object, make sure you pass the security mask you actually want (no more, no less)


There are two categories of "Access denied" errors. One occurs when you attempt to create the handle, and the other occurs when you attempt to use the handle.

HANDLE hEvent = OpenEvent(SYNCHRONIZE, FALSE, TEXT("MyEvent"));

If this call fails with Access denied, then it means that you don't have access to the object to the level you requested. In the above example, it means that you don't have SYNCHRONIZE access to the event.

A common reason for getting an Access denied when trying to create a handle is that you asked for too much access. For example, you might write

HKEY hkey;
LONG lError = RegOpenKeyEx(
    hkeyRoot, subkeyName, 0, KEY_ALL_ACCESS, &hkey);
if (lError == ERROR_SUCCESS) {
 DWORD dwType;
 DWORD dwData;
 DWORD cbData = sizeof(dwData);
 lError = RegQueryValueEx(hkey, TEXT("ValueName"), nullptr,
                          &dwType, &dwData, &cbData);
 if (lError == ERROR_SUCCESS && dwType == REG_DWORD &&
     cbData == sizeof(dwData)) {
  .. do something with dwData ..
 }
 RegCloseKey(hkey);
}

The call to Reg­Open­Key­Ex fails with Access denied. The proximate reason is that you don't have KEY_ALL_ACCESS permission on the registry key, which makes sense because KEY_ALL_ACCESS asks for permission to do everything imaginable to the registry key, including crazy things like "Change the permissions of the key to deny access to the rightful owner."

But why are you asking for full access to the key if all you're going to do is read from it?

HKEY hkey;
LONG lError = RegOpenKeyEx(
    hkeyRoot, subkeyName, 0, KEY_READ, &hkey);
if (lError == ERROR_SUCCESS) {
 DWORD dwType;
 DWORD dwData;
 DWORD cbData = sizeof(dwData);
 lError = RegQueryValueEx(hkey, TEXT("ValueName"), nullptr,
                          &dwType, &dwData, &cbData);
 if (lError == ERROR_SUCCESS && dwType == REG_DWORD &&
     cbData == sizeof(dwData)) {
  .. do something with dwData ..
 }
 RegCloseKey(hkey);
}

If you want to go for bonus points, ask for KEY_QUERY_VALUE instead of KEY_READ, since all you are going to do with the key is read a value.

When requesting access to an object, it's best to ask for the minimum access required to get the job done.

This is like the old principle of mathematics: After you've proved something, try to weaken the hypothesis as much as possible and strengthen the conclusions as much as possible. In other words, once you've solved a problem, figure out the absolute minimum requirements for your solution to work, and figure out the largest amount of information your solution produces.

On the other hand, if you get an Access denied error when trying to use a handle, then the problem is that you didn't open the handle with enough access.

HKEY hkey;
LONG lError = RegOpenKeyEx(
    hkeyRoot, subkeyName, 0, KEY_READ, &hkey);
if (lError == ERROR_SUCCESS) {
 DWORD dwData = 1;
 lError = RegSetValueEx(hkey, TEXT("ValueName"), nullptr,
             REG_DWORD, (const BYTE*>)&dwData, sizeof(dwData));
 if (lError == ERROR_SUCCESS && dwType == REG_DWORD &&
     cbData == sizeof(dwData)) {
  .. do something with dwData ..
 }
 RegCloseKey(hkey);
}

Here, the Reg­Open­Key­Ex succeeds, but the Reg­Set­Value­Ex fails. That's because the registry key was opened for KEY_READ access, but the Reg­Set­Value­Ex operation requires KEY_SET_VALUE access. To fix this, you need to open the key with the access you actually want:

HKEY hkey;
LONG lError = RegOpenKeyEx(
    hkeyRoot, subkeyName, 0, KEY_SET_VALUE, &hkey);
if (lError == ERROR_SUCCESS) {
 DWORD dwData = 1;
 lError = RegSetValueEx(hkey, TEXT("ValueName"), nullptr,
             REG_DWORD, (const BYTE*>)&dwData, sizeof(dwData));
 if (lError == ERROR_SUCCESS && dwType == REG_DWORD &&
     cbData == sizeof(dwData)) {
  .. do something with dwData ..
 }
 RegCloseKey(hkey);
}

When requesting access to an object, it's best to ask for the minimum access required to get the job done, but no less.

Armed with this information, you can solve this problem:

In the main thread, we create an event like this:

TheEvent = CreateEvent(NULL, TRUE, FALSE, name);

A worker thread opens the event like this:

EventHandle = OpenEvent(SYNCHRONIZE, FALSE, name);

The Open­Event succeeds, but we try to use the handle, we get Access denied:

SetEvent(EventHandle);

On the other hand, if the worker thread uses the Create­Event function to get the handle, then the Set­Event succeeds.

What are we doing wrong?

Comments (15)
  1. Joshua says:

    What we are doing wrong: calling OpenEvent on an event in the same process. If for some reason you need another handle, call DuplicateHandle; but you should design your way out of needing it.

  2. Medinoc says:

    SYNCHRONIZE is for waiting; setting an event requires EVENT_MODIFY_STATE.

  3. David Totzke says:

    You asked for SYNCHRONIZE when what you really wanted was SYNCHRONIZE | EVENT_MODIFY_STATE.

  4. David Totzke says:

    @Joshua Given that CreateEvent returns with EVENT_ALL_ACCESS wouldn't DuplicateHandle give your copy the same access rights?  This would seem to violate the "When requesting access to an object, it's best to ask for the minimum access required to get the job done" rule.  Curious and too lazy to look it up.  :)

  5. skSdnW says:

    And if you are lazy you can use MAXIMUM_ALLOWED. This is especially useful when supporting XP/2003 (PROCESS_QUERY_INFORMATION) and Vista+ (PROCESS_QUERY_LIMITED_INFORMATION), otherwise you would have to OpenProcess twice in some cases on NT6.

  6. Marvin says:

    One reason many people ask for too much at create time is because the second failure – to use – is often problematic.

    It can happen only rarely on some unusual code path and not caught in normal testing

    It requires modifying code possibly far away from the use point under development.

    The whole thing seems to be insufficiently well designed. Why cannot rights be, if allowed, increased at use time?

    That is if access X is denied at use time try to add the permission to the handle and only fail if that fails.

    I know I could probably code such a wrapper myself but it seems like something that ought to be baked in.

  7. Joshua says:

    @David Totzke: "When you have an object, almost never request it."

  8. Someone says:

    >>That is if access X is denied at use time try to add the permission to the handle and only fail if that fails.<<

    You want to be able to open all files without specifying any restriction, and if you call FileRead() or FileWrite(), it should just happen (if the user of your application has this permission in theory)?

    Beside other issues, this would delay permission checks to some random points in the code instead being performed at CreateFile.

    If you want to be able to write to something, what is in the way for you to just open the handle with write permission?

    [Other problems: File locking. (If you could upgrade a handle, then that would invalidate prior file locking decisions.) Also, giving an intentionally low-access handle to another component. (E.g., a handle to an event you can wait on, but which you cannot signal.) The basic principle is that permissions are checked at the time the handle is created. This saves having to do permission checks at every operation. -Raymond]
  9. Marvin says:

    Someone wrote: "If you want to be able to write to something, what is in the way for you to just open the handle with write permission?"

    Please re-read my comment where I gave some examples why. The basic issue is that the code doing the use is often written at a different time from the code that creates and not even by the same person.

    Raymond: I get the basic principle, the perf benefit it provides and that there are scenarios where having non-upgradable handle is deliberate. My point is that the most common scenario is made cumbersome by this. Whenever you write UseObjectX you need to find where the handle was created and validate that you have the permission (or deduce it in some other way). If you *remove* the UseObjectX line you ideally need to go and validate that the permissions you ask for are not an overkill now (nobody really does that of course).

    It *would* be better design IMO where you had to explicitly do something for special scenarios you mention (e.g. mark the handle as non-upgradable at create time if you really want it to) while the most common way "just works".

    This isn't a super big issue by any means but enough people get bitten by it as your blog post and my own experience suggest.

    [The perf benefit is definitely non-negligible. Suppose you access a file or registry key that is remote. You get "access denied". Now instead of propagating the result back to the caller, you try to upgrade the handle, which is another network round trip. Everything now takes twice as long. (Also, upgrading the handle will invalidate earlier security decisions made based on the previous handle access level.) And finally, making handles auto-upgrade means that the actual error condition is harder to find. "Most of the time, the upgrade succeeds, but occasionally, there is a sharing violation and the upgrade fails." Instead of catching the problem up front, it is detected only if the code path is exercised. Going back to the original problem: You should set up the "give me a handle" function so you tell it the access you want from that handle, instead of having that function guess what access you want and hoping for the best. -Raymond]
  10. Rosalinda Ramirez says:

    What happens when your access gets revoked while you have the handle open?

  11. alegr1 says:

    @Rozalinda Ramirez:

    The security descriptor (ACL) is only checked when you open the object handle. If the ACL changes after that, it doesn't affect access through existing handles.

  12. Joshua says:

    ["Most of the time, the upgrade succeeds, but occasionally, there is a sharing violation and the upgrade fails." -Raymond]

    Ah good you saw the same thing I did. Upgrading from read to write automatically is not so smart.

  13. jgh says:

    Decades ago I had to debug loads of brain-dead code that did "open for update" when all it wanted to subsequently do was read from the file, and the original authors couldn't understand the concept of not having the authority to write to a file.

  14. S says:

    "The basic issue is that the code doing the use is often written at a different time from the code that creates and not even by the same person."

    No, this is a non-issue. You have an object with some methods, all designed around the usual "open", work and "close" principal (for example, logging to a file or reading some text file). The file access is only done by the very few methods of this object. it is not visible to anyone else, and there is absolutely no desire to read from a logfile or to write to your input file.

    There is no "often" here.

  15. dave says:

    >That is if access X is denied at use time try to add the permission to the handle and only fail if that fails.

    Why stop there?  We could get rid of opening files entirely.  Just write to the file, and if necessary, all of the 'open' nonsense happens for you.

    In reality, one major reason for the existence of 'open' is that so we can get the expensive stuff over and done with once. Where expensive stuff includes ACL checking.

    On a more philosophical level, if I open for read access and then my program does a write, why should the system conclude that I *want* to write?   Perhaps it's a bug, and if it is, I want to fail, not possibly corrupt data.  

Comments are closed.

Skip to main content