From c9d68ffda581297fe543c35d9ca3a2f51f8077e7 Mon Sep 17 00:00:00 2001 From: Elliot Saba Date: Wed, 14 Oct 2020 18:16:08 -0700 Subject: [PATCH 1/6] win,fs Use `AccessCheck()` for all `access()` calls No longer shy away from using `AccessCheck()` for simple `W_OK`/`R_OK` checks; use it for all calls unconditionally. --- src/win/fs.c | 216 ++++++++++++++++++++++++--------------------------- 1 file changed, 103 insertions(+), 113 deletions(-) diff --git a/src/win/fs.c b/src/win/fs.c index c25497fb94c..a8a78fe46f4 100644 --- a/src/win/fs.c +++ b/src/win/fs.c @@ -2126,133 +2126,123 @@ static void fs__access(uv_fs_t* req) { SET_REQ_WIN32_ERROR(req, GetLastError()); return; } + DWORD sdLen = 0, err = 0, tokenAccess = 0, executeAccessRights = 0, + grantedAccess = 0, privilegesLen = 0; + SECURITY_INFORMATION si = NULL; + PSECURITY_DESCRIPTOR sd = NULL; + HANDLE hToken = NULL, hImpersonatedToken = NULL; + GENERIC_MAPPING mapping = { 0xFFFFFFFF }; + PRIVILEGE_SET privileges = { 0 }; + BOOL result = FALSE; /* - * If write access was requested, ensure that either - * the requested file is not marked as READONLY, - * or that it's actually a directory (directories - * cannot be read-only in Windows) - */ - if ((req->fs.info.mode & W_OK) && - ((attr & FILE_ATTRIBUTE_READONLY) || - !(attr & FILE_ATTRIBUTE_DIRECTORY))) { - SET_REQ_WIN32_ERROR(req, UV_EPERM); + * First, we must allocate enough space. We do that + * by first passing in a zero-length null pointer, + * storing the desired length into `sd_length`. + * We expect this call to fail with a certain error code. + */ + si = OWNER_SECURITY_INFORMATION | + GROUP_SECURITY_INFORMATION | + DACL_SECURITY_INFORMATION; + if (GetFileSecurityW(req->file.pathw, si, NULL, 0, &sdLen)) { + SET_REQ_RESULT(req, UV_UNKNOWN); + return; + } + err = GetLastError(); + if (ERROR_INSUFFICIENT_BUFFER != err) { + SET_REQ_WIN32_ERROR(req, err); return; } - /* - * If executable access was requested, we must check - * with the AccessCheck() ACL call. This is mildly - * expensive, so only do it if `X_OK` was requested. - */ - if (req->fs.info.mode & X_OK) { - DWORD sdLen = 0, err = 0, tokenAccess = 0, executeAccessRights = 0, - grantedAccess = 0, privilegesLen = 0; - SECURITY_INFORMATION si = NULL; - PSECURITY_DESCRIPTOR sd = NULL; - HANDLE hToken = NULL, hImpersonatedToken = NULL; - GENERIC_MAPPING mapping = { 0xFFFFFFFF }; - PRIVILEGE_SET privileges = { 0 }; - BOOL result = FALSE; - - /* - * First, we must allocate enough space. We do that - * by first passing in a zero-length null pointer, - * storing the desired length into `sd_length`. - * We expect this call to fail with a certain error code. - */ - si = OWNER_SECURITY_INFORMATION | - GROUP_SECURITY_INFORMATION | - DACL_SECURITY_INFORMATION; - if (GetFileSecurityW(req->file.pathw, si, NULL, 0, &sdLen)) { - SET_REQ_RESULT(req, UV_UNKNOWN); - return; - } - err = GetLastError(); - if (ERROR_INSUFFICIENT_BUFFER != err) { - SET_REQ_WIN32_ERROR(req, err); - return; - } + /* Now that we know how big `sd` must be, allocate it */ + sd = (PSECURITY_DESCRIPTOR)uv__malloc(sdLen); + if (!sd) { + uv_fatal_error(ERROR_OUTOFMEMORY, "uv__malloc"); + } - /* Now that we know how big `sd` must be, allocate it */ - sd = (PSECURITY_DESCRIPTOR)uv__malloc(sdLen); - if (!sd) { - uv_fatal_error(ERROR_OUTOFMEMORY, "uv__malloc"); - } + /* Call `GetFileSecurity()` with the requisite `sd` structure. */ + if (!GetFileSecurityW(req->file.pathw, si, sd, sdLen, &sdLen)) { + SET_REQ_WIN32_ERROR(req, GetLastError()); + goto accesscheck_cleanup; + } - /* Call `GetFileSecurity()` with the requisite `sd` structure. */ - if (!GetFileSecurityW(req->file.pathw, si, sd, sdLen, &sdLen)) { - SET_REQ_WIN32_ERROR(req, GetLastError()); - goto accesscheck_cleanup; - } + /* + * Next we need to create an impersonation token representing + * the current user and the current process. + */ + tokenAccess = TOKEN_IMPERSONATE | + TOKEN_QUERY | + TOKEN_DUPLICATE | + STANDARD_RIGHTS_READ; + if (!OpenProcessToken(GetCurrentProcess(), tokenAccess, &hToken )) { + SET_REQ_WIN32_ERROR(req, GetLastError()); + goto accesscheck_cleanup; + } + if (!DuplicateToken(hToken, SecurityImpersonation, &hImpersonatedToken)) { + SET_REQ_WIN32_ERROR(req, GetLastError()); + goto accesscheck_cleanup; + } + /* + * Next, construct a mapping from generic access rights to + * the more specific access rights that AccessCheck expects. + */ + executeAccessRights = 0; + if (req->fs.info.mode & R_OK) { + executeAccessRights |= FILE_GENERIC_READ; + mapping.GenericRead = FILE_GENERIC_READ; + } + if (req->fs.info.mode & W_OK) { + executeAccessRights |= FILE_GENERIC_WRITE; + mapping.GenericWrite = FILE_GENERIC_WRITE; + } + if (req->fs.info.mode & X_OK) { + executeAccessRights |= FILE_GENERIC_EXECUTE; + mapping.GenericExecute = FILE_GENERIC_EXECUTE; + } + MapGenericMask(&executeAccessRights, &mapping); + + privilegesLen = sizeof(privileges); + result = FALSE; + if (AccessCheck(sd, + hImpersonatedToken, + executeAccessRights, + &mapping, + &privileges, + &privilegesLen, + &grantedAccess, + &result)) { /* - * Next we need to create an impersonation token representing - * the current user and the current process. - */ - tokenAccess = TOKEN_IMPERSONATE | - TOKEN_QUERY | - TOKEN_DUPLICATE | - STANDARD_RIGHTS_READ; - if (!OpenProcessToken(GetCurrentProcess(), tokenAccess, &hToken )) { - SET_REQ_WIN32_ERROR(req, GetLastError()); - goto accesscheck_cleanup; - } - if (!DuplicateToken(hToken, SecurityImpersonation, &hImpersonatedToken)) { - SET_REQ_WIN32_ERROR(req, GetLastError()); + * If AccessCheck passes, nothing went wrong, but + * we must still check that we have access. + */ + if (!result) { + SET_REQ_WIN32_ERROR(req, UV_EPERM); goto accesscheck_cleanup; } - + } else { /* - * Next, construct a mapping from generic access rights to - * the more specific access rights that AccessCheck expects. - */ - executeAccessRights = FILE_GENERIC_EXECUTE; - mapping.GenericExecute = FILE_GENERIC_EXECUTE; - MapGenericMask(&executeAccessRights, &mapping); - - privilegesLen = sizeof(privileges); - result = FALSE; - if (AccessCheck(sd, - hImpersonatedToken, - executeAccessRights, - &mapping, - &privileges, - &privilegesLen, - &grantedAccess, - &result)) { - /* - * If AccessCheck passes, nothing went wrong, but - * we must still check that we have access. - */ - if (!result) { - SET_REQ_WIN32_ERROR(req, UV_EPERM); - goto accesscheck_cleanup; - } - } else { - /* - * This signifies that something went wrong with the - * actual AccessCheck() invocation itself. - */ - SET_REQ_WIN32_ERROR(req, GetLastError()); - goto accesscheck_cleanup; - } + * This signifies that something went wrong with the + * actual AccessCheck() invocation itself. + */ + SET_REQ_WIN32_ERROR(req, GetLastError()); + goto accesscheck_cleanup; + } accesscheck_cleanup: - uv__free(sd); - if (hImpersonatedToken != NULL) { - CloseHandle(hImpersonatedToken); - } - if (hToken != NULL) { - CloseHandle(hToken); - } - /* - * If the result is false, return immediately. - * Some error code has been set in the `req` already. - */ - if (!result) { - return; - } + uv__free(sd); + if (hImpersonatedToken != NULL) { + CloseHandle(hImpersonatedToken); + } + if (hToken != NULL) { + CloseHandle(hToken); + } + /* + * If the result is false, return immediately. + * Some error code has been set in the `req` already. + */ + if (!result) { + return; } /* If we get to the end, everything worked out. */ From 8a29cb7fbe6df51aa9e4efc77f60a66c3885bd1a Mon Sep 17 00:00:00 2001 From: Elliot Saba Date: Wed, 14 Oct 2020 18:17:12 -0700 Subject: [PATCH 2/6] win,fs: Do not allow setting troublesome permissions on Windows Because our ACL implementation does not have the required flexibility to represent situations such as `0o757` due to the permissions ordering that Windows naturally applies, we simply do not allow the `other` entity to have permissions that the `group` entity does not have. This causes `chmod(0o757)` to be transparently mapped to `chmod(0o755)`, as an example. --- src/win/fs.c | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/src/win/fs.c b/src/win/fs.c index a8a78fe46f4..c999a52c5a9 100644 --- a/src/win/fs.c +++ b/src/win/fs.c @@ -2417,7 +2417,8 @@ static void fs__chmod(uv_fs_t* req) { /* Skip this EA if it isn't an SID, or it is "Everyone" or our actual group */ if (pOldEAs[ea_idx].Trustee.TrusteeForm != TRUSTEE_IS_SID || EqualSid(pEASid, psidEveryone) || - EqualSid(pEASid, psidGroup)) { + EqualSid(pEASid, psidGroup) || + EqualSid(pEASid, psidOwner)) { continue; } @@ -2434,11 +2435,22 @@ static void fs__chmod(uv_fs_t* req) { } /* Create an ACE for each triplet (user, group, other) */ - numNewEAs = 8 + 3*numOtherGroups; + numNewEAs = 7 + 3*numOtherGroups; ea = (PEXPLICIT_ACCESS_W) malloc(sizeof(EXPLICIT_ACCESS_W)*numNewEAs); u_mode = ((req->fs.info.mode & S_IRWXU) >> 6); g_mode = ((req->fs.info.mode & S_IRWXG) >> 3); - o_mode = ((req->fs.info.mode & S_IRWXO) >> 0); + + /* + * Because we do not control the ordering of ACE entries within the ACL that + * we're building, the `SetNamedSecurityInfoW()` function call below will + * place all DENY entries first, and all `ALLOW` entries second. This + * makes it impossible to support e.g. 0o757 permissions, because in order + * to support an "allow" for "other", then a "deny" for "group", we are + * unable to have an "allow" before the "deny" for "group". To address this, + * we simply do not allow the "other" entity to have permissions that "group" + * itself does not have. + */ + o_mode = ((req->fs.info.mode & S_IRWXO) >> 0) & g_mode; /* We start by revoking previous permissions for trustees we care about */ build_access_struct(&ea[0], psidOwner, TRUSTEE_IS_USER, 0, REVOKE_ACCESS); @@ -2446,31 +2458,29 @@ static void fs__chmod(uv_fs_t* req) { build_access_struct(&ea[2], psidEveryone, TRUSTEE_IS_GROUP, 0, REVOKE_ACCESS); /* - * We also add explicit denies to user and group if the user shouldn't have - * a permission but the group or everyone can, for instance. + * We also add explicit denies to user if the group shouldn't have a permission. */ - u_deny_mode = (~u_mode) & (g_mode | o_mode); - g_deny_mode = (~g_mode) & o_mode; + u_deny_mode = (~u_mode) & (g_mode); build_access_struct(&ea[3], psidOwner, TRUSTEE_IS_USER, u_deny_mode, DENY_ACCESS); - build_access_struct(&ea[4], psidGroup, TRUSTEE_IS_GROUP, g_deny_mode, DENY_ACCESS); /* Next, add explicit allows for (owner, group, other) */ - build_access_struct(&ea[5], psidOwner, TRUSTEE_IS_USER, u_mode, SET_ACCESS); - build_access_struct(&ea[6], psidGroup, TRUSTEE_IS_GROUP, g_mode, SET_ACCESS); - build_access_struct(&ea[7], psidEveryone, TRUSTEE_IS_GROUP, o_mode, SET_ACCESS); + build_access_struct(&ea[4], psidOwner, TRUSTEE_IS_USER, u_mode, SET_ACCESS); + build_access_struct(&ea[5], psidGroup, TRUSTEE_IS_GROUP, g_mode, SET_ACCESS); + build_access_struct(&ea[6], psidEveryone, TRUSTEE_IS_GROUP, o_mode, SET_ACCESS); /* * Iterate over all old ACEs, looking for groups that we belong to, and setting * the appropriate access bits for those groups (as g_mode) */ - ea_write_idx = 8; + ea_write_idx = 7; for (ea_idx=0; ea_idx Date: Tue, 18 Oct 2022 14:45:44 -0400 Subject: [PATCH 3/6] Fix null pointer exception reported in https://github.com/JuliaLang/libuv/issues/23 --- src/win/fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/win/fs.c b/src/win/fs.c index c999a52c5a9..2200fb3f98c 100644 --- a/src/win/fs.c +++ b/src/win/fs.c @@ -2359,7 +2359,7 @@ static void fs__chmod(uv_fs_t* req) { * We fix it by forcibly clearing some kind of cache by setting the security info with the * old DACL, then attempting to read it in again. */ - if (numOldEAs != pOldDACL->AceCount) { + if (pOldDACL != NULL && numOldEAs != pOldDACL->AceCount) { if (ERROR_SUCCESS != SetNamedSecurityInfoW( req->file.pathw, SE_FILE_OBJECT, From 89c1b49c13fb3ee3739010dcd5beacd087d1dc4f Mon Sep 17 00:00:00 2001 From: Elliot Saba Date: Tue, 18 Oct 2022 15:35:50 -0400 Subject: [PATCH 4/6] Apply "other" permissions to all groups we are not a part of Previously, we would apply permissions only to groups that we were a part of, but we should apply our "other" permissions to groups that we are not a part of. --- src/win/fs.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/win/fs.c b/src/win/fs.c index 2200fb3f98c..f7dbc1a7147 100644 --- a/src/win/fs.c +++ b/src/win/fs.c @@ -2422,16 +2422,8 @@ static void fs__chmod(uv_fs_t* req) { continue; } - /* Check to see if our user is a member of this group */ - if (!CheckTokenMembership(hImpersonatedToken, pEASid, &isMember)) { - SET_REQ_WIN32_ERROR(req, GetLastError()); - goto chmod_cleanup; - } - - /* If we're a member, then count it */ - if (isMember) { - numOtherGroups++; - } + /* Count these as "other" groups */ + numOtherGroups++; } /* Create an ACE for each triplet (user, group, other) */ @@ -2491,16 +2483,21 @@ static void fs__chmod(uv_fs_t* req) { } /* - * If we're a member, then count it. We limit our `ea_write_idx` to avoid - * the unlikely event that we have been added to a group since we first - * calculated `numOtherGroups`. + * We limit our `ea_write_idx` to avoid the unlikely event that we + * have been added to a group since we first calculated `numOtherGroups`. */ - if (isMember && ea_write_idx < numNewEAs) { - build_access_struct(&ea[ea_write_idx], pEASid, TRUSTEE_IS_GROUP, 0, REVOKE_ACCESS); + assert(ea_write_idx <= numNewEAs - 3); + if (isMember) { + build_access_struct(&ea[ea_write_idx + 0], pEASid, TRUSTEE_IS_GROUP, 0, REVOKE_ACCESS); build_access_struct(&ea[ea_write_idx + 1], pEASid, TRUSTEE_IS_GROUP, g_deny_mode, DENY_ACCESS); build_access_struct(&ea[ea_write_idx + 2], pEASid, TRUSTEE_IS_GROUP, g_mode, SET_ACCESS); - ea_write_idx += 3; + } else { + /* We revoke a second time here to keep offset management simple in groups of three. */ + build_access_struct(&ea[ea_write_idx + 0], pEASid, TRUSTEE_IS_GROUP, 0, REVOKE_ACCESS); + build_access_struct(&ea[ea_write_idx + 1], pEASid, TRUSTEE_IS_GROUP, 0, REVOKE_ACCESS); + build_access_struct(&ea[ea_write_idx + 2], pEASid, TRUSTEE_IS_GROUP, o_mode, SET_ACCESS); } + ea_write_idx += 3; } /* Set entries in the ACL object */ From 0097e6846693366a4d5359ce91fb14ea30d3001a Mon Sep 17 00:00:00 2001 From: Your Name Date: Mon, 5 Dec 2022 07:06:06 -0800 Subject: [PATCH 5/6] Use `DWORD` instead of `mode_t` on Windows It's not needed to use `mode_t` here, and it just confuses VS. --- src/win/fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/win/fs.c b/src/win/fs.c index f7dbc1a7147..a3ae15194ee 100644 --- a/src/win/fs.c +++ b/src/win/fs.c @@ -2250,7 +2250,7 @@ static void fs__access(uv_fs_t* req) { } static void build_access_struct(EXPLICIT_ACCESS_W* ea, PSID owner, - TRUSTEE_TYPE user_type, mode_t mode_triplet, + TRUSTEE_TYPE user_type, DWORD mode_triplet, ACCESS_MODE allow_deny) { /* * We map the typical POSIX mode bits r/w/x as the Windows From 4f92483c6c91e4b31db65ee18ef460666a507f40 Mon Sep 17 00:00:00 2001 From: Elliot Saba Date: Tue, 23 May 2023 14:30:23 +0000 Subject: [PATCH 6/6] Define `S_IRWXU` and others if not already defined. --- src/win/fs.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/win/fs.c b/src/win/fs.c index a3ae15194ee..b4f6899d622 100644 --- a/src/win/fs.c +++ b/src/win/fs.c @@ -44,6 +44,23 @@ #define UV_FS_FREE_PTR 0x0008 #define UV_FS_CLEANEDUP 0x0010 +#ifndef S_IRWXU +#define S_IRWXU 0000700 /* RWX mask for owner */ +#define S_IRUSR 0000400 /* R for owner */ +#define S_IWUSR 0000200 /* W for owner */ +#define S_IXUSR 0000100 /* X for owner */ + +#define S_IRWXG 0000070 /* RWX mask for group */ +#define S_IRGRP 0000040 /* R for group */ +#define S_IWGRP 0000020 /* W for group */ +#define S_IXGRP 0000010 /* X for group */ + +#define S_IRWXO 0000007 /* RWX mask for other */ +#define S_IROTH 0000004 /* R for other */ +#define S_IWOTH 0000002 /* W for other */ +#define S_IXOTH 0000001 /* X for other */ +#endif + /* number of attempts to generate a unique directory name before declaring failure */ #define TMP_MAX 32767