Is it wrong to call SHFileOperation from a service? Revised


My initial reaction to this question was to say, "I don't know if I'd call it wrong, but I'd call it highly inadvisable."

I'd like to revise my guidance.

It's flat-out wrong, at least in the case where you call it while impersonating.

The registry key HKEY_CURRENT_USER is bound to the current user at the time the key is first accessed by a process:

The mapping between HKEY_CURRENT_USER and HKEY_USERS is per process and is established the first time the process references HKEY_CURRENT_USER. The mapping is based on the security context of the first thread to reference HKEY_CURRENT_USER. If this security context does not have a registry hive loaded in HKEY_USERS, the mapping is established with HKEY_USERS\.Default. After this mapping is established it persists, even if the security context of the thread changes.

Emphasis mine.

This means that if you impersonate a user, and then access HKEY_CURRENT_USER, then that binds HKEY_CURRENT_USER to the impersonated user. Even if you stop impersonating, future references to HKEY_CURRENT_USER will still refer to that user.

This is probably not what you expected.

The shell takes a lot of settings from the current user. If you impersonate a user and then call into the shell, your service is now using that user's settings, which is effectively an elevation of privilege: An unprivileged user is now modifying settings for a service. For example, if the user has customized the Print verb for text files, and you use Shell­Execute to invoke the print verb on a text document, you are at the mercy of whatever the user's print verb is bound to. Maybe it runs Notepad, but maybe it runs pwnz0rd.exe. You don't know.

Similarly, the user might have a per-user registered copy hook or namespace extension, and now you just loaded a user-controlled COM object into your service.

In both cases, this is known to insiders as hitting the jackpot.

Okay, so what about if you call Shell­Execute or some other shell function while not impersonating? You might say, "That's okay, because the current user's registry is the service user, not the untrusted attacker user." But look at that sentence I highlighted up there. Once HKEY_CURRENT_USER get bound to a particular user, it remains bound to that user even after impersonation ends. If somebody else inadvisedly called a shell function while impersonating, and that shell function happens to be the first one to access HKEY_CURRENT_USER, then your call to a shell function while not impersonating will still use that impersonated user's registry. Congratulations, you are now running untrusted code, and you're not even impersonating any more!

So my recommendation is don't do it. Don't call shell functions while impersonating unless the function is explicitly documented as supporting impersonation. (The only ones I'm aware of that fall into this category are functions like SHGet­Folder­Path which accept an explicit token handle.) Otherwise, you may have created (or in the case of copy hooks, definitely created) a code injection security vulnerability in your service.

Comments (28)
  1. Zarat says:

    Ouch. Does this also affect "normal" (ie non-service) programs? Cause then I really need to add code accessing HKCU in all my 'main' functions to make sure no external code gets around to do that before me.

    ["Normal" programs cannot impersonate, so the issue does not exist. -Raymond]
  2. Anon says:

    This sounds like a thing which needs to be blocked, given that the behaviour is not only inconsistent, but in the best case counter-intuitive and in the worst case places the machine's security into a completely unknown state.

  3. alegr1 says:

    Most applications access HKCU on startup (to read settings, etc), so this will not be a problem.

  4. Zarat says:

    @alegr1

    If this is indeed an issue I'd rather add explicit code accessing HKCU (and comment it appropriately) than relying on the implicit access from loading settings or other stuff. At least for the code I write myself, I prefer to keep it safe by design instead of 'safe by luck' ;)

    Also I worry that accessing environment variables does not need HKCU so I could get (for example) into the current users %AppData% without freezing HKCU on that user.

  5. Wyatt says:

    @alger1 -> Most SERVICE applications do not access HKCU, ever.

    So, if I'm writing a Service application (I have a few), should just intentionally access HKCU on service start, just to make sure some third party component (or other future developer) doesn't accidentally reference HKCU under impersonation?

  6. Zarat says:

    Never mind my comment about environment variables, I was thinking too much about what could go wrong. Relying on environment variables representing the current user is in itself probably not a good idea from a security POV.

  7. Cherry says:

    Why doesn't Windows establish the link on process creation instead of waiting for the first access? I can't imagine it being that expensive.

  8. Joshua says:

    @Cherry: Meh. If you think you're going to hit the problem, you can do it yourself at process startup time.

  9. Cesar says:

    There should be a DisableProcessHkeyCurrentUser() which makes your program abort and dump core if anything on your process ever tries to establish that mapping. Impersonating services could call that function as the first thing on their wWinMain and not have to worry about this issue anymore.

    (In before someone reports that one of these crazy inject-into-everything DLLs calls into the shell for some inane reason.)

  10. Cherry says:

    I understand that. I wasn't compaining, I was asking "why", that is, asking for the reasoning. After all, the reason I enjoy reading this block is that many "why" questions are being answered here.

  11. Cherry says:

    (The last comment was directed to Joshua. And it should have been "blog", not "block", of course.)

  12. JamesJohnston says:

    It would be useful if Microsoft provided a tool to audit compiled services for statically-linked calls to unapproved / unsupported Windows API calls from a service. Obviously, this wouldn't catch anyone intentionally dodging this tool (e.g. via LoadLibrary/GetProcAddress), but would help catch many cases.

    In this case, it would be trivial for me to load the compiled EXE in Dependency Walker and see that it is statically linked to SHFileOperation in shell32.dll. An automated tool could flag this any many other questionable API calls, including ones like CreateWindow(Ex), simply by checking these links. Any linked 3rd-party DLLs would also be checked.

    A problematic case would be unapproved COM objects: CoCreateInstance itself isn't bad; some COM objects are OK to use from a service. Perhaps a dynamic mode of this verification tool could be used to shim the COM APIs and check class/interface IDs against a blacklist or whitelist. (And also perform the above linkage checks on any dynamically-loaded DLL. While you're at it, you could shim GetProcAddress as well.)

    Or maybe I can pose a different question: how does Microsoft ensure no dangerous API calls creep into any of their own services? (Surely they don't depend on a 100% manual process of code reviews?)

    [Such a tool would kick way too many false positives: Your EXE links to a DLL, and that DLL links to SHFileOperation. But the EXE doesn't use the DLL in a way that leads to SHFileOperation. The tool doesn't know that, though. -Raymond]
  13. Mike Dimmick says:

    Cesar: Windows does allow you to substitute a key for one of the root keys using the RegOverridePredefKey function. This function is really designed to allow installers to put a self-registering library into a sandbox and detect what it did, but you could certainly use it to put your own canary HKEY_CURRENT_USER key in place. If you want code to fail you just need to deny all rights to create subkeys of the key you substitute.

  14. Scott Brickey says:

    experienced a similar problem with a Windows service that hosted a WCF service (which was called with impersonation)... the WCF methods would use a logging framework defined in the app.config.

    it was reported that the service would occasionally stop working, and after some investigation, it was identified that this only occurred after windows updates caused a reboot. When it had happened previously, it was always restarted and then troubleshot by a local admin (thus the "first call" was by someone who had access to the app.config file). This would work until the next reboot.

    quick change that it would log on app startup (under the service account), and it never happened again.

    TL;DR: not just the registery, but potentially also config files (app.config/etc)

  15. Joshua says:

    [Such a tool would kick way too many false positives: Your EXE links to a DLL, and that DLL links to SHFileOperation. But the EXE doesn't use the DLL in a way that leads to SHFileOperation. The tool doesn't know that, though. -Raymond]

    Unless the tool requires symbols so it can study the call fan-out. Then you only get a handful of false positives.

    @James Johnston: CreateWindowEx is normal/fine for a service (message-only window). I've never done that one, but I've done GDI to HBITMAP a few times.

    @Mike Dimmick: Oh my that use of RegOverridePredefKey is genius.

  16. The hkey binding issue can be mitigated if your service calls RegDisablePredefinedCacheEx first thing upon startup.  (Since the API toggles a process-wide setting, don't use the API if you don't own the entire process.)

    That of course doesn't change Raymond's guidance; don't use the shell in a service.

  17. Gabe says:

    Jeffrey Tippet: It looks like RegDisablePredefinedCache is more appropriate for this situation because it only affects HKCU. RegDisablePredefinedCacheEx is overkill because it affects all predefined keys.

  18. Ben Voigt says:

    @Gabe: RegDisablePredefinedCache is less appropriate because it fails to affect HKEY_CLASSES_ROOT and HKEY_CURRENT_USER_LOCAL_SETTINGS.

    The documentation specifically says "Services that use impersonation should call RegDisablePredefinedCacheEx before using predefined registry handles."

  19. cheong00 says:

    @Cherry: Like Wyatt said above, most service do not need HKCU at all, and for good reason.

    Imagine if your service needs HKCU hive and it is somehow running as a user that is on roaming profile. If the service is not started as "Automatic (Delayed)" the boot time would have been lengthened while waiting for the HKCU hive to load.

  20. cheong00 says:

    I do hope the windows header file can change impersonation related APIs ( msdn.microsoft.com/.../cc246062.aspx ) into macro that sets some flags before calling them, then make some "unsafe to call within impersonation context" APIs generate compiler warnings when seeing the flag.

    Never mind... on a second thought it would be too much work and possibly result in incomplete list (say, what if new API that's not safe from impersonation is introduced but forgotten to add the warning directive? The users could simply assume there's no problem calling that function).

    [It wouldn't help if the impersonation and the unsafe call are in separate functions. Or if they are in the same function, but the impersonation and unsafe calls are in separate "if" blocks that happen never to be both true at the same time due to external factors not visible to the compiler. -Raymond]
  21. cheong00 says:

    [It wouldn't help if the impersonation and the unsafe call are in separate functions. Or if they are in the same function, but the impersonation and unsafe calls are in separate "if" blocks that happen never to be both true at the same time due to external factors not visible to the compiler. -Raymond]

    For the first case, I wonder if C++ would introduce function to relay warning directive metadata to callers, so people use those libraries can see the warnings while writing.

    Maybe it has little use for 3rd party libraries (as they'll probably just suppress all warnings to avoid looking ugly), but could be good for internal built shared libraries. (In my old company, the shared library that contains most non-customizable business logic is maintained by a core team. The other teams just get the compiled libraries only. Those teams can see the comments in XML doc, but warnings written there were not verbose enough and could be unintentionally ignored)

  22. Cube 8 says:

    @JamesJohnston

    How would you catch a LoadLibrary call for shell32 and then the call to SHFileOperation through GetProcAddress?

  23. Dave Bacher says:

    Really, ultimately, everyone wants bug-free, crash proof programs.

    This isn't Pokémon, we don't need to catch them all.

    There is a subset of Windows.h that is valid (supported) from services.  By having a #define for that, and limiting down to the subset, you'd create an actionable red-squiggle.  The third party libraries aren't helped, but nobody cares.  The third party libraries can be broken.

    There is a subset of Windows.h that is valid (supported) from an impersonated context.  By having a separate #define for that, and limiting down to the subset, you'd create actionable red-squiggles.  Again, third party libraries wouldn't be flagged, and again nobody cares.

    I can't act on a third party library being broken, and that library can be changed out from under me by another app.  Even if I have a private copy, all I can do is report the bug.  I can't act on it.

    And so they don't matter -- similarly GetProcAddr doesn't matter.

    Basically, it's the same as the argument for #define STRICT.  #define STRICT can't catch problems in third party libraries, either.  But it gives you the red squiggles in your own code, so that you can correct your own code.  You can't be sure everyone else's code is correct, but at least you can know your own code is.

    I think that's the goal here -- to report the issue, and make sure it is adequately communicated to the developer, at build time, in the code that the developer directly controls.  Opt-in (so it doesn't break existing code).

  24. cheong00 says:

    @Dave Bacher: I think Raymond's argument is that when false positive is too high, everyone would just ignore the warning. And I agree with that.

    While the compiler could help detecting (like what they do to detect unused variables, but perhaps in more complicated way), this opens up a new category of "detective works" for compiler to do. And of the list of "function properties" to consider have context validation on, I think the check on "thread safe functions" are on higher priority.

  25. JamesJohnston says:

    @Cube 8: You could have a dynamic version of the tool hook those APIs.  But the main reason for the tool would not be to detect explicit attempts to avoid said tool.  If you are trying to work around the tool, you are probably not going to use it in the first place.

    Personally, most of the API calls I make are statically linked, and I only use LoadLibrary/GetProcAddress for newer APIs where I'm also maintaining a fall-back option for Windows XP.  This isn't the majority of calls.  A simple "grep" of the code for GetProcAddress would yield a manageable list and could be inspected by hand.

    I really like the idea that @Dave Bacher proposes of having a #define to eliminate the APIs unsupported from services.  It's probably a better solution than I propose and would be easy to use - not requiring a special tool.  Selection of 3rd-party libraries by the developer can be limited to those that also use the #define.

    I don't think I agree with the "why bother? too many false positives" perspective.  You could make the same argument against compiler warnings.  Sure - maybe what you were doing was safe in that particular case - but the compiler wasn't sure, and the fix is easy.  Some developers fall into the trap of ignoring compiler warnings, and suddenly we have "too many false positives."  Professional developers will go in and fix the warnings - even the "false positive" warnings - so that we have zero warnings.

    Eliminating the problematic imports in your own binary should be easy and essential.  If you link with a 3rd-party DLL that is kicking too many false positives, maybe you should contact your vendor and make sure they are really false positives...  To me, excessive false positives would be a sign of some badly-needed refactoring and shouldn't be used as an excuse to ignore the problem.

  26. ZLB says:

    @JamesJohnston: "You could have a dynamic version of the tool..."

    Like 'Application Verifier'?

  27. I just searched a bunch of my source code for the past few years.  Found an instance of SHFileOperation that does run in a service context, but I'm >99% certain there's no impersonation.  The reason I called it was that I needed to delete a directory hierarchy.  The MSDN doc for RemoveDirectory recommends SHFileOperation for that purpose.  Using SHFileOperation seemed better than writing custom code to recursively delete files and directories, or to shell out to "rd /s /q dirname"...

  28. jonathancd says:

    We had some surprising bugs caused by this for some time, luckily it hadn't manifested as a security issue - but I wasted weeks until I stumbled into RegDisablePredefinedCacheEx.

    Definitely something that should be documented better. E.g. In the .Net or WINAPI docs, inlined into any documentation about writing a service. Not only would that mitigate security issues, but also headaches.

Comments are closed.

Skip to main content