Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address review comments on libuv patch #16

Open
wants to merge 6 commits into
base: julia-uv2-1.44.2
Choose a base branch
from
Open

Address review comments on libuv patch #16

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c9d68ff
win,fs Use `AccessCheck()` for all `access()` calls
staticfloat Oct 15, 2020
8a29cb7
win,fs: Do not allow setting troublesome permissions on Windows
staticfloat Oct 15, 2020
3b2ec18
Fix null pointer exception reported in https://github.com/JuliaLang/l…
staticfloat Oct 18, 2022
89c1b49
Apply "other" permissions to all groups we are not a part of
staticfloat Oct 18, 2022
0097e68
Use `DWORD` instead of `mode_t` on Windows
Dec 5, 2022
4f92483
Define `S_IRWXU` and others if not already defined.
staticfloat May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
302 changes: 158 additions & 144 deletions src/win/fs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -2126,141 +2143,131 @@ 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. */
SET_REQ_SUCCESS(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
Expand Down Expand Up @@ -2369,7 +2376,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,
Expand Down Expand Up @@ -2427,60 +2434,62 @@ 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;
}

/* 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) */
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
Comment on lines +2454 to +2455
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean SetEntriesInAcl, which we should perhaps just avoid due to this (like cygwin)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my initial explorations, if we don't use SetEntriesInAcl, it's a lot more work. We have to manually merge entries and such that I would much prefer not to do. The Cygwin function to do this is many hundreds of lines of code; not counting their ACL datatype abstraction. I don't actually care about this case (others with more permissions than group) very much, so I'm alright with just not supporting it and doing something less-broken than before.

* 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);
build_access_struct(&ea[1], psidGroup, TRUSTEE_IS_GROUP, 0, REVOKE_ACCESS);
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<numOldEAs; ++ea_idx) {
BOOL isMember = FALSE;
PSID pEASid = (PSID)pOldEAs[ea_idx].Trustee.ptstrName;
/* 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;
}

Expand All @@ -2491,16 +2500,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 */
Expand Down