What happens if you call RevertToSelf when not impersonating?


A customer wanted to know what happens if you call Revert­To­Self from a thread that is not impersonating. "Does the call succeed or fail? This particular scenario is not explicitly discussed in the documentation. We have a bunch of places in our code that say if (impersonating) RevertToSelf(); and we were wondering whether the if test was really necessary."

The answer to the question is that calling Revert­To­Self when the thread is not impersonating will return success without doing anything (because the thread is already not impersonating).

However, that doesn't mean that you can blindly remove all your if tests. You don't want to over-revert either. Consider:

// Error checking elided for expository purposes.
void DoSomething()
{
 bool impersonating = false;

 if (!ThreadIsAlreadyImpersonating() &&
     ImpersonationIsNeeded()) {
   StartImpersonating();
   impersonating = true;
 }

 DoWork();

 if (impersonating) {
  RevertToSelf();
 }
}

If you remove the if (impersonating) and unconditionally revert, then you have a security defect if the thread was already impersonating, because your modified code will unconditionally revert and prematurely end the existing impersonation.

So yes, it's okay to call Revert­To­Self when the thread is not impersonating, but that doesn't relieve you of the responsiblity of knowing when to revert.

Comments (7)
  1. kantos says:

    Seems like something you could make an RAII helper for:

    class Impersonator{
    bool impersonating;
    // prevent copy/move
    Impersonator(Impersonator&&) = delete;
    public:
    Impersonator():impersonating(false){
    if (!ThreadIsAlreadyImpersonating() &&
    ImpersonationIsNeeded()) {
    StartImpersonating();
    impersonating = true;
    }
    }

    ~Impersonator() noexcept{
    if (impersonating) {
    RevertToSelf();
    }
    }
    };

    1. That rephrases the question but does nothing to help answer it.

      1. kantos says:

        Not my intent, my intent was merely to demonstrate that you could take the sample code and wrap it up in an RAII helper for exception safety and ease of use.

        1. I intentionally present the code in a straightforward linear manner in order to avoid distractions from the issue at hand. In real life, you would use RAII, add error handling, include comments, all that stuff. The RAII class as written is not very reusable. You cannot plug in custom ImpersonationIsNeeded() or StartImpersonating() methods, so it would be a one-shot class specifically for DoSomething() that isn’t reusable.

          1. kantos says:

            Apologies, feel free to delete my comment

    2. ZLB says:

      Another approach to this which we took in a project a long time ago was to tie the impersonation stuff up in thread-factory.

      Rather than having to track impersonation, you simply asked for thread with an impersonation token and you got a thread that is impersonating that user back. Obviously, this won’t work well if you have to impersonate a whole ton of users.

      This meant that there was a clearer seperation between code that impersonates and code that doesn’t. This seperation is important becuase theres a whole ton of stuff that will break if you call under impersonation.

    3. Killer{R} says:

      Better OpenThreadToken in c-tor, remember resulted hanlde in field, then impersonate under specified token, in d-tor: revert and and re-impersonate under old token (if it was set on thread) back. This will allow nested impersonation under different user’s account, but only need to be careful with access rights to token objects themselves.

Comments are closed.

Skip to main content