https://git.reactos.org/?p=reactos.git;a=commitdiff;h=59e74584ac5c48e9edb2b…
commit 59e74584ac5c48e9edb2bf494c2224bd5ca6fa90
Author: George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Tue Jun 6 19:06:11 2023 +0200
Commit: George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Fri Jun 9 11:53:56 2023 +0200
[NTOS:SE] Refactor SeTokenCanImpersonate
- Refactor most of the code, since there's quite some stuff that don't make much sense.
For instance ImpersonationLevel is basically the requested impersonation level a
server asks for. PsImpersonateClient doesn't explicitly say that SecurityAnonymous
and SecurityIdentification are not allowed. If the server was to give such levels
it simply means it doesn't want to impersonate the client.
Another thing that doesn't make much sense is that we check if the client is
associated with an anonymous token, then avoid impersonating regular anonymous
tokens that weren't created by the system. Only system can create such tokens
and an anonymous token basically means a token with hidden security info.
- Check that the server is within the same client logon session.
- If the server is granted the SeImpersonatePrivilege privilege, allow impersonation
regardless of the conditions we want to check for.
- Update the documentation and code comments.
---
ntoskrnl/se/token.c | 127 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 83 insertions(+), 44 deletions(-)
diff --git a/ntoskrnl/se/token.c b/ntoskrnl/se/token.c
index bdb360e8672..dae5a411dff 100644
--- a/ntoskrnl/se/token.c
+++ b/ntoskrnl/se/token.c
@@ -2158,22 +2158,47 @@ SeTokenIsWriteRestricted(
/**
* @brief
- * Ensures that client impersonation can occur by checking if the token
- * we're going to assign as the impersonation token can be actually impersonated
- * in the first place. The routine is used primarily by PsImpersonateClient.
+ * Determines whether the server is allowed to impersonate on behalf
+ * of a client or not. For further details, see Remarks.
*
* @param[in] ProcessToken
- * Token from a process.
+ * A pointer to the primary access token of the server process
+ * that requests impersonation of the client target.
*
* @param[in] TokenToImpersonate
- * Token that we are going to impersonate.
+ * A pointer to an access token that represents a client that is to
+ * be impersonated.
*
* @param[in] ImpersonationLevel
- * Security impersonation level grade.
+ * The requested impersonation level.
*
* @return
* Returns TRUE if the conditions checked are met for token impersonation,
* FALSE otherwise.
+ *
+ * @remarks
+ * The server has to meet the following criteria in order to impersonate
+ * a client, that is:
+ *
+ * - The server must not impersonate a client beyond the level that
+ * the client imposed on itself.
+ *
+ * - The server must be authenticated on the same logon session of
+ * the target client.
+ *
+ * - IF NOT then the server's user ID has to match to that of the
+ * target client.
+ *
+ * - The server must not be restricted in order to impersonate a
+ * client that is not restricted.
+ *
+ * If the associated access token that represents the security properties
+ * of the server is granted the SeImpersonatePrivilege privilege the server
+ * is given immediate impersonation, regardless of the conditions above.
+ * If the client in question is associated with an anonymous token then
+ * the server is given immediate impersonation. Or if the server simply
+ * doesn't ask for impersonation but instead it wants to get the security
+ * identification of a client, the server is given immediate impersonation.
*/
BOOLEAN
NTAPI
@@ -2186,73 +2211,87 @@ SeTokenCanImpersonate(
PAGED_CODE();
/*
- * SecurityAnonymous and SecurityIdentification levels do not
- * allow impersonation.
+ * The server may want to obtain identification details of a client
+ * instead of impersonating so just give the server a pass.
*/
- if (ImpersonationLevel == SecurityAnonymous ||
- ImpersonationLevel == SecurityIdentification)
+ if (ImpersonationLevel < SecurityIdentification)
{
- return FALSE;
+ DPRINT("The server doesn't ask for impersonation\n");
+ return TRUE;
}
/* Time to lock our tokens */
SepAcquireTokenLockShared(ProcessToken);
SepAcquireTokenLockShared(TokenToImpersonate);
- /* What kind of authentication ID does the token have? */
+ /*
+ * As the name implies, an anonymous token has invisible security
+ * identification details. By the general rule these tokens do not
+ * pose a danger in terms of power escalation so give the server a pass.
+ */
if (RtlEqualLuid(&TokenToImpersonate->AuthenticationId,
&SeAnonymousAuthenticationId))
{
- /*
- * OK, it looks like the token has an anonymous
- * authentication. Is that token created by the system?
- */
- if (TokenToImpersonate->TokenSource.SourceName != SeSystemTokenSource.SourceName &&
- !RtlEqualLuid(&TokenToImpersonate->TokenSource.SourceIdentifier, &SeSystemTokenSource.SourceIdentifier))
- {
- /* It isn't, we can't impersonate regular tokens */
- DPRINT("SeTokenCanImpersonate(): Token has an anonymous authentication ID, can't impersonate!\n");
- CanImpersonate = FALSE;
- goto Quit;
- }
+ DPRINT("The token to impersonate has an anonymous authentication ID, allow impersonation either way\n");
+ CanImpersonate = TRUE;
+ goto Quit;
}
- /* Are the SID values from both tokens equal? */
- if (!RtlEqualSid(ProcessToken->UserAndGroups->Sid,
- TokenToImpersonate->UserAndGroups->Sid))
+ /* Allow impersonation for the process server if it's granted the impersonation privilege */
+ if ((ProcessToken->TokenFlags & TOKEN_HAS_IMPERSONATE_PRIVILEGE) != 0)
{
- /* They aren't, bail out */
- DPRINT("SeTokenCanImpersonate(): Tokens SIDs are not equal!\n");
- CanImpersonate = FALSE;
+ DPRINT("The process is granted the impersonation privilege, allow impersonation\n");
+ CanImpersonate = TRUE;
goto Quit;
}
/*
- * Make sure the tokens aren't diverged in terms of
- * restrictions, that is, one token is restricted
- * but the other one isn't.
+ * Deny impersonation for the server if it wants to impersonate a client
+ * beyond what the impersonation level originally permits.
*/
- if (SeTokenIsRestricted(ProcessToken) !=
- SeTokenIsRestricted(TokenToImpersonate))
+ if (ImpersonationLevel > TokenToImpersonate->ImpersonationLevel)
{
- /*
- * One token is restricted so we cannot
- * continue further at this point, bail out.
- */
- DPRINT("SeTokenCanImpersonate(): One token is restricted, can't continue!\n");
+ DPRINT1("Cannot impersonate a client above the permitted impersonation level!\n");
CanImpersonate = FALSE;
goto Quit;
}
+ /* Is the server authenticated on the same client originating session? */
+ if (!RtlEqualLuid(&ProcessToken->AuthenticationId,
+ &TokenToImpersonate->OriginatingLogonSession))
+ {
+ /* It's not, check that at least both the server and client are the same user */
+ if (!RtlEqualSid(ProcessToken->UserAndGroups[0].Sid,
+ TokenToImpersonate->UserAndGroups[0].Sid))
+ {
+ DPRINT1("Server and client aren't the same user!\n");
+ CanImpersonate = FALSE;
+ goto Quit;
+ }
+
+ /*
+ * Make sure the tokens haven't diverged in terms of restrictions
+ * that is one token is restricted but the other one isn't. If that
+ * would have been the case then the server would have impersonated
+ * a less restricted client thus potentially triggering an elevation,
+ * which is not what we want.
+ */
+ if (SeTokenIsRestricted(ProcessToken) !=
+ SeTokenIsRestricted(TokenToImpersonate))
+ {
+ DPRINT1("Attempting to impersonate a less restricted client token, bail out!\n");
+ CanImpersonate = FALSE;
+ goto Quit;
+ }
+ }
+
/* If we've reached that far then we can impersonate! */
- DPRINT("SeTokenCanImpersonate(): We can impersonate.\n");
+ DPRINT("We can impersonate\n");
CanImpersonate = TRUE;
Quit:
- /* We're done, unlock the tokens now */
- SepReleaseTokenLock(ProcessToken);
SepReleaseTokenLock(TokenToImpersonate);
-
+ SepReleaseTokenLock(ProcessToken);
return CanImpersonate;
}
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=8e2fe925f25cd14688ed8…
commit 8e2fe925f25cd14688ed82f4b84e2d1a8fb172cb
Author: George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Tue Jun 6 17:24:15 2023 +0200
Commit: George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Fri Jun 9 11:53:55 2023 +0200
[NTOS:PS] Do not reference the copied token twice and properly assign the impersonation level in case the server can't impersonate
As it currently stands the PsImpersonateClient routine does the following approach.
If impersonation couldn't be granted to a client the routine will make a copy
of the client's access token. As it makes a copy of the said token PsImpersonateClient
will reference the copied token after impersonation info have been filled out.
In the same code path we are assigning the desired level for impersonation to thread
impersonation info.
This is wrong for two reasons:
- On a copy situation the SeCopyClientToken routine holds a reference as the object
has been created. Referencing it at the bottom of the PsImpersonateClient routine
will make it that the token is referenced twice and whenever a server stops
impersonation the token still has an extra reference count which keeps the token
still alive in object database and memory space.
- If client impersonation is not possible the thread impersonation info should
have been assigned SecurityIdentification level to further indicate that the
actual impersonation of the thread is not currently in force but instead we
are assigning the impersonation level that is supplied by the caller. For instance
if the requested level is SecurityDelegation but impersonation is not possible
the level will be assigned that of SecurityDelegation yet the token has an
impersonation level of SecurityIdentification. This could lead to erratic behaviors
as well as potential impersonation escalation.
Fix the aforementioned issues by avoiding a double reference and properly assign
the impersonation level to SecurityIdentification if the server is not able to
impersonate the target client.
---
ntoskrnl/ps/security.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/ntoskrnl/ps/security.c b/ntoskrnl/ps/security.c
index 23145529743..4d47764fc07 100644
--- a/ntoskrnl/ps/security.c
+++ b/ntoskrnl/ps/security.c
@@ -615,6 +615,7 @@ PsImpersonateClient(IN PETHREAD Thread,
{
PPS_IMPERSONATION_INFORMATION Impersonation, OldData;
PTOKEN OldToken = NULL, ProcessToken = NULL;
+ BOOLEAN CopiedToken = FALSE;
PACCESS_TOKEN NewToken, ImpersonationToken;
PEJOB Job;
NTSTATUS Status;
@@ -706,7 +707,13 @@ PsImpersonateClient(IN PETHREAD Thread,
return Status;
}
- /* Since we cannot impersonate, assign the newly copied token */
+ /*
+ * Since we cannot impersonate, assign the newly copied token.
+ * SeCopyClientToken already holds a reference to the copied token,
+ * let the code path below know that it must not reference it twice.
+ */
+ CopiedToken = TRUE;
+ ImpersonationLevel = SecurityIdentification;
ImpersonationToken = NewToken;
}
@@ -721,6 +728,11 @@ PsImpersonateClient(IN PETHREAD Thread,
if ((Job->SecurityLimitFlags & JOB_OBJECT_SECURITY_NO_ADMIN) &&
SeTokenIsAdmin(ImpersonationToken))
{
+ if (CopiedToken)
+ {
+ ObDereferenceObject(ImpersonationToken);
+ }
+
return STATUS_ACCESS_DENIED;
}
@@ -728,6 +740,11 @@ PsImpersonateClient(IN PETHREAD Thread,
if ((Job->SecurityLimitFlags & JOB_OBJECT_SECURITY_RESTRICTED_TOKEN) &&
SeTokenIsRestricted(ImpersonationToken))
{
+ if (CopiedToken)
+ {
+ ObDereferenceObject(ImpersonationToken);
+ }
+
return STATUS_ACCESS_DENIED;
}
@@ -758,7 +775,12 @@ PsImpersonateClient(IN PETHREAD Thread,
Impersonation->CopyOnOpen = CopyOnOpen;
Impersonation->EffectiveOnly = EffectiveOnly;
Impersonation->Token = ImpersonationToken;
- ObReferenceObject(ImpersonationToken);
+
+ /* Do not reference the token again if we copied it */
+ if (!CopiedToken)
+ {
+ ObReferenceObject(ImpersonationToken);
+ }
/* Unlock the thread */
PspUnlockThreadSecurityExclusive(Thread);
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=d8bfe2a2618c3d2f4a303…
commit d8bfe2a2618c3d2f4a3038c7fd4f221978c865ba
Author: George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Fri Jun 9 11:41:52 2023 +0200
Commit: George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Fri Jun 9 11:43:04 2023 +0200
[GITHUB] Draft PRs should be exempt from closure by the stale PR bot
---
.github/workflows/stale.yml | 1 +
1 file changed, 1 insertion(+)
diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml
index 5ffbec47d18..51f61b8b51d 100644
--- a/.github/workflows/stale.yml
+++ b/.github/workflows/stale.yml
@@ -26,6 +26,7 @@ jobs:
days-before-close: 14
days-before-issue-close: -1
exempt-all-assignees: true
+ exempt-draft-pr: true
stale-pr-message: 'This PR is stale because it received no updates in the last 4 months. Without removing the stale label, or commenting on this ticket it will be closed in 2 weeks.'
stale-issue-label: 'no-issue-activity'
stale-pr-label: 'no-pr-activity'