Failure to Assess File Permissions Via SMB on Windows

General JUCE discussion
1

Recently had a user report that our app was refusing to load audio files from their SMB network drive, and complaining about “bad permissions.” When I reproduced the issue locally, it appears that any file on an SMB drive will return false for File::hasReadAccess and File::hasWriteAccess. Upon removing the permissions check in our code, the file was able to be read without a problem (specifically, a FileInputStream opens without error). I’m also able to manipulate and delete these files in File Explorer.

I dug into the source and all I could suss out is that the AccessCheck() call being performed is setting AccessStatus to FALSE. I tried to do more reading into this to see if there are any caveats to access-checking when using shared network drives, but my familiarity with the Windows APIs is pretty poor. The closest thing I’ve found is this StackOverflow post.

I have a solution that should patch the issue for our user, but I still wanted to report this because I think this might be a bug (or I’m missing some rather obvious step for interfacing with shared network files).

2

Hi Juce team, Hi @officialnsa !

I hope you are all right

We encounter the same issue recently and this was not present before:

Since we update from Juce 5 to Juce 8, Users saving projects on a Samba or NAS share get “no write access” from our Juce based software, although Explorer and every other application can write to the same files.

Juce Change; commit 9a12e93f, May 2022 rewrote File::hasWriteAccess() on Windows and added hasReadAccess(). The new implementation fetches the file’s DACL with GetNamedSecurityInfo and evaluates it locally with AccessCheck against a duplicate of the process token. On an SMB share it does not: the DACL returned by a Samba or NAS server contains the server’s own SIDs, which do not match the local token, and some NAS boxes return no usable DACL at all. AccessCheck therefore says “denied” for a user the server would let write.

Here a patch proposal for juce_Files_windows.cpp :

File-hasWriteAccess-on-NAS-.patch.diff.txt (5.3 KB)

hasFileAccess() no longer evaluates ACLs client side. It opens the path with CreateFile and the requested access (GENERIC_READ or GENERIC_WRITE, FILE_FLAG_BACKUP_SEMANTICS for directories), closes the handle, and treats a sharing violation as “access granted, file merely busy”. This asks the file system itself, which is exactly what the real read or write will do, and on a share the server evaluates it with its own identity mapping. Local semantics are unchanged: a deny ACE, a read-only attribute, or a denied directory still yield false.

Here a JUCE_UNIT_TESTS for juce_File.cpp

The discriminating test reproduces the NAS situation in mirror image: the thread impersonates a restricted copy of the process token with the Authenticated Users group disabled, on a file whose DACL grants write to that group only. The real write fails, the fix answers false, the old code answers true because it only looked at the process token.

juce_file.cpp_JUCE_UNIT_TESTS.diff.patch.txt (7.0 KB)

Hoping this may helps

Warm regards

3

fwiw if your code is shaped like

if file.hasWritePerms() :
   write(file, data);

it already has a bug in it because permissions can change in between any fs call.

The suggested fix opens a file to determine the permissions, for your use case of opening a file to read or write to it. The correct fix is to check the error after open, not to check permissions before doing an operation (and all fs operations are fallable so you must check errors after every fs call - especially when dealing with network file systems).

It would be nice to have the method return correctly but I wanted to point out that the reason for it is already flawed and the fix wouldn’t do much good there.

So for this example you would do something like

juce::File file(path);
auto os = path.createOutputStream();
if (os->getStatus().failed()) {
    /* Permission denied error surfaces here */
}
4

Hi @holy-city, Many thanks for your reactivity on this complex subject.

I’m completely agreed with you on the race condition case:

The open result must be checked whatever a pre-check said, and our code does, but

  1. createOutputStream() returns null when the open fails, so the Result is lost. So we need a FileOutputStream constructed directly plus getStatus().

  2. hasWriteAccess() is not only a pre-write guard. It drives UI state (read-only badges, greyed Save…), and JUCE itself relies on it: AndroidDocument fills its write-permission flag from it, and the POSIX move path guards copyInternal() with it. IHMO, a method that may answers wrong on network shares is a bug on its own.

  3. The current Windows implementation evaluates the DACL in user mode against a duplicate of the process token. On SMB shares the DACL carries the server’s SIDs, which never match the local token, or the server returns no usable DACL and GetNamedSecurityInfo fails. Either way the answer is “denied” for a file the server lets you write. The proposed fix opens the path with the requested access and closes the handle, which lets the local OS or the server do the access check with the identity that will actually perform the I/O. That is what the POSIX side already does with access(W_OK), so it makes the two platforms consistent. Cost: an open for write breaks other clients’ opportunistic lock on that file, and maybe file date will be touched by this “check only” call.

The unit test pins the defect locally: the thread impersonates a restricted copy of the process token with Authenticated Users disabled, on a file whose DACL grants write to that group only. The real open fails, yet the current code answers true because it consulted the process token. On a NAS the same wrong identity gives the opposite answer. The test fail on current develop and succeed with the fix, as old Juce 5 implementation does.

Waiting to heard from your thinking about all of this.

I wish you the best.