What's wrong with this code, part 20: Yet another reason that named synchronization objects are dangerous, the answers

Microsoft can be quite obsessive about instrumentation and metrics.  We have a significant body of tools that perform static and dynamic analysis on our operating systems.  Some of these tools (for example prefast and FxCop) are public, some are not.

Friday I posted a small snippet of code that showed a couple of the issues using named shared synchronization objects.  The example was abstract, but the problems called out is quite real.

If you run the Microsoft prefix tool on the example, it will point out the first problem in the snippet:  The calls to CreateEvent are made without specifying a DACL.  This is bad, because the objects get a DACL based on the creator's token, which means you don't get to control it, and it's entirely possible that the DACL you get is to permissive.

Even if you're not worried about a bad guy (and you always have to worry about the bad guy), if you're using the shared synchronization object in a client/server scenario, it means that the client might not have rights to access the server  because the newly created object will get a security descriptor based on the token of the creator.  Even worse, it's possible that the SD in the client grants the server rights but not vice-versa.  That means that depending on which side gets to run first, you might get different results.  That kind of timing problem can be a nightmare to debug.

If the ACL creates is too permissive, then things get scary.  If the DACL grants full access, it means that a bad guy can do ANYTHING to your event - they can change it's state, they can set a new DACL on it (locking your application out), etc.  Weak DACLs are a recipe for denial of service attacks (or worse - depending on the access rights granted and the circumstances, they can even enable elevation of privilege attacks).

Of course the fix is simple: Provide a DACL that grants the relevant access rights to the principles that are going to be opening the shared synchronization object.

That's what we did when our source code analysis tools showed the first problem - they correctly detected that we were using a named object and hadn't specified a DACL for the named object.

 

A couple of weeks later, I got a new bug report, this time from the security guys.  It seems that they had ran a second tool which looked for ACLs that were too permissive.  In their scan, they found our named shared object, which had granted GENERIC_ALL access to the windows audio service.

<grumble>

I fixed the problem by tightening the ACL on the shared object to only grant SYNCHRONIZE access to the audio service (that's all the audio service needed), and I ran afoul of the second problem.

As the documentation for CreateEvent clearly spells out, if the named synchronization object already existed, CreateEvent API (and CreateMutex and CreateSemaphore) opens the object for EVENT_ALL_ACCESS (or SEMAPHORE_ALL_ACCESS or MUTEX_ALL_ACCESS).  That means that your DACL needs to grant EVENT_ALL_ACCESS (or...) or the call to CreateEvent will fail.  And I just changed the DACL to only grant SYNCHRONIZE access.

The problem is that we used CreateEvent to prevent from the race condition that occurs if you try to call OpenEvent/CreateEvent - it's possible for the OpenEvent call to fail and have all the consumers of the named event fall through to the CreateEvent API - to avoid this race, the code simply used CreateEvent.  I wasn't willing to add a call to OpenEvent because it would leave the race condition still present.

 

 

The good news is that the COSD people had identified this problem in the named synchronization object APIs and in Vista the new CreateEventEx was added to enable applications to work around this deficiency in the APIs: The CreateEventEx API allows you to specify an access mask to be used on the object after it is opened.  So now it's possible to have accurate DACLs on named synchronization objects AND prevent squatting attacks.

 

 

 

NB: The current documentation for CreateEventEx says that it opens for EVENT_ALL_ACCESS if the named object already exists, this is a typo in the documentation, I've already pointed it out to the doc people to let them know about the error.