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.

2 files changed, 56 insertions, 25 deletions

diff --git a/ChangeLog b/ChangeLog
index f4a6b72..d065f45 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -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.
+
 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 08e5b42..78128d6 100644
--- a/win/tclWinFile.c
+++ b/win/tclWinFile.c
@@ -1540,20 +1540,30 @@ NativeAccess(
 	}
     }
 
-#ifndef UNICODE
+    if (mode == F_OK) {
+	/*
+	 * File exists, nothing else to check.
+	 */
+
+	return 0;
+    }
+
     if ((mode & W_OK)
-	    && (attr & FILE_ATTRIBUTE_READONLY)) {
+	&& (attr & FILE_ATTRIBUTE_READONLY)
+	&& !(attr & FILE_ATTRIBUTE_DIRECTORY)) {
 	/*
-	 * We don't have the advanced 'GetFileSecurity', and our
-	 * attributes say the file is not writable. If we do have
-	 * 'GetFileSecurity', 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);
 	return -1;
     }
-#endif /* !UNICODE */
 
     if (mode & X_OK) {
 	if (!(attr & FILE_ATTRIBUTE_DIRECTORY) && !NativeIsExec(nativePath)) {
@@ -1572,15 +1582,15 @@ 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) {
+#ifndef UNICODE
+    {
 	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;
@@ -1596,7 +1606,8 @@ NativeAccess(
 	size = 0;
 	GetFileSecurity(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
@@ -1629,7 +1640,8 @@ NativeAccess(
 
 	if (!GetFileSecurity(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
 	     */
@@ -1638,6 +1650,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.
 	 */
@@ -1714,18 +1746,8 @@ 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;
-	}
     }
+#endif /* !UNICODE */
     return 0;
 }