diff options
| -rw-r--r-- | ChangeLog | 9 | ||||
| -rw-r--r-- | win/tclWinFile.c | 69 |
2 files changed, 54 insertions, 24 deletions
@@ -1,3 +1,12 @@ +2012-02-02 Don Porter <dgp@users.sourceforge.net> + + * win/tclWinFile.c: [Bugs 2974459,2879351,1951574,1852572, + 1661378,1613456]: Revisions to the NativeAccess() routine that + queries file permissions on Windows native filesystems. Meant to + fix numerous bugs where [file writable|readable|executable] "lies" + about what operations are possible, especially when the file resides + on a Samba share. Patch merged from the fix-win-native-access branch. + 2012-02-01 Donal K. Fellows <dkf@users.sf.net> * doc/AddErrInfo.3: [Bug 3482614]: Documentation nit. diff --git a/win/tclWinFile.c b/win/tclWinFile.c index 54cf0cf..08413d6 100644 --- a/win/tclWinFile.c +++ b/win/tclWinFile.c @@ -1550,14 +1550,25 @@ NativeAccess( return -1; } + if (mode == F_OK) { + /* + * File exists, nothing else to check. + */ + + return 0; + } + if ((mode & W_OK) - && (tclWinProcs->getFileSecurityProc == NULL) - && (attr & FILE_ATTRIBUTE_READONLY)) { + && (attr & FILE_ATTRIBUTE_READONLY) + && !(attr & FILE_ATTRIBUTE_DIRECTORY)) { /* - * We don't have the advanced 'getFileSecurityProc', and our - * attributes say the file is not writable. If we do have - * 'getFileSecurityProc', we'll do a more robust XP-related check - * below. + * The attributes say the file is not writable. If the file is a + * regular file (i.e., not a directory), then the file is not + * writable, full stop. For directories, the read-only bit is + * (mostly) ignored by Windows, so we can't ascertain anything about + * directory access from the attrib data. However, if we have the + * advanced 'getFileSecurityProc', then more robust ACL checks + * will be done below. */ Tcl_SetErrno(EACCES); @@ -1581,15 +1592,14 @@ NativeAccess( * we have a more complex permissions structure so we try to check that. * The code below is remarkably complex for such a simple thing as finding * what permissions the OS has set for a file. - * - * If we are simply checking for file existence, then we don't need all - * these complications (which are really quite slow: with this code 'file - * readable' is 5-6 times slower than 'file exists'). */ - if ((mode != F_OK) && (tclWinProcs->getFileSecurityProc != NULL)) { + if (tclWinProcs->getFileSecurityProc != NULL) { SECURITY_DESCRIPTOR *sdPtr = NULL; unsigned long size; + SID *pSid = 0; + BOOL SidDefaulted; + SID_IDENTIFIER_AUTHORITY samba_unmapped = { 0, 0, 0, 0, 0, 22 }; GENERIC_MAPPING genMap; HANDLE hToken = NULL; DWORD desiredAccess = 0, grantedAccess = 0; @@ -1605,7 +1615,8 @@ NativeAccess( size = 0; (*tclWinProcs->getFileSecurityProc)(nativePath, OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION - | DACL_SECURITY_INFORMATION, 0, 0, &size); + | DACL_SECURITY_INFORMATION | LABEL_SECURITY_INFORMATION, + 0, 0, &size); /* * Should have failed with ERROR_INSUFFICIENT_BUFFER @@ -1638,7 +1649,8 @@ NativeAccess( if (!(*tclWinProcs->getFileSecurityProc)(nativePath, OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION - | DACL_SECURITY_INFORMATION, sdPtr, size, &size)) { + | DACL_SECURITY_INFORMATION | LABEL_SECURITY_INFORMATION, + sdPtr, size, &size)) { /* * Error getting owner SD */ @@ -1647,6 +1659,26 @@ NativeAccess( } /* + * As of Samba 3.0.23 (10-Jul-2006), unmapped users and groups are + * assigned to SID domains S-1-22-1 and S-1-22-2, where "22" is the + * top-level authority. If the file owner and group is unmapped then + * the ACL access check below will only test against world access, + * which is likely to be more restrictive than the actual access + * restrictions. Since the ACL tests are more likely wrong than + * right, skip them. Moreover, the unix owner access permissions are + * usually mapped to the Windows attributes, so if the user is the + * file owner then the attrib checks above are correct (as far as they + * go). + */ + + if(!GetSecurityDescriptorOwner(sdPtr,&pSid,&SidDefaulted) || + memcmp(GetSidIdentifierAuthority(pSid),&samba_unmapped, + sizeof(SID_IDENTIFIER_AUTHORITY))==0) { + HeapFree(GetProcessHeap(), 0, sdPtr); + return 0; /* Attrib tests say access allowed. */ + } + + /* * Perform security impersonation of the user and open the resulting * thread token. */ @@ -1723,17 +1755,6 @@ NativeAccess( return -1; } - /* - * For directories the above checks are ok. For files, though, we must - * still check the 'attr' value. - */ - - if ((mode & W_OK) - && !(attr & FILE_ATTRIBUTE_DIRECTORY) - && (attr & FILE_ATTRIBUTE_READONLY)) { - Tcl_SetErrno(EACCES); - return -1; - } } return 0; } |