Which use of AdjustTokenPrivileges in KB131065 is correct?

Question

KB131065 demonstrates how to activate SeDebugPrivilege to open a handle to any process. It has a function named SetPrivilege that calls AdjustTokenPrivileges, but there are two implementations and no mention why.

The first implementation calls ATP in two passes:

// 
// first pass.  get current privilege setting
// 
tp.PrivilegeCount           = 1;
tp.Privileges[0].Luid       = luid;
tp.Privileges[0].Attributes = 0;

AdjustTokenPrivileges(
        hToken,
        FALSE,
        &tp,
        sizeof(TOKEN_PRIVILEGES),
        &tpPrevious,
        &cbPrevious
        );

if (GetLastError() != ERROR_SUCCESS) return FALSE;

// 
// second pass.  set privilege based on previous setting
// 
tpPrevious.PrivilegeCount       = 1;
tpPrevious.Privileges[0].Luid   = luid;

if(bEnablePrivilege) {
    tpPrevious.Privileges[0].Attributes |= (SE_PRIVILEGE_ENABLED);
}
else {
    tpPrevious.Privileges[0].Attributes ^= (SE_PRIVILEGE_ENABLED &
        tpPrevious.Privileges[0].Attributes);
}

AdjustTokenPrivileges(
        hToken,
        FALSE,
        &tpPrevious,
        cbPrevious,
        NULL,
        NULL
        );

That looks like overkill. I don't see anything in the documentation to indicate that the Attributes fields of the previous-state parameter receive any additional flags that need to be preserved. It looks like more like that code starts out by disabling the privilege, and then it goes back and either re-enables it or disables it again. Have I interpreted it correctly? Do others recommend that technique?

The second implementation of SetPrivilege looks more like what I'd expect:

tp.PrivilegeCount = 1;
tp.Privileges[0].Luid = luid;
if(bEnablePrivilege) {
    tp.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;
} else {
    tp.Privileges[0].Attributes = 0;
}
AdjustTokenPrivileges( hToken, FALSE, &tp, cb, NULL, NULL );

Is the second version oversimplified? I notice that it's essentially the same as what's demonstrated in Enabling and Disabling Privileges in C++, except that the KB article ignores the API's return value and goes directly to GetLastError to detect success and failure.

Answer 1

I've always used the second example.

I can't see any point at all in the first example. It would make sense for, say, the GetWindowLong\SetWindowLong functions, but for privileges, there are simply no persisted settings that could be combined and therefore would require retrieving the previous state.

And yes, they actually disable the privilege in order to get its current state, which is wrong.
Then again, there are many pieces of terrible code on MSDN, unfortunately.