fix(win32): only increase ACL size if a new ACE is about to be added

src/common/filesystembase.cpp

@@ -1,3 +1,3 @@
 /*
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2018 ownCloud GmbH
@@ -779,7 +779,8 @@
     fileHandle.reset(CreateFileW(rawPath, desiredAccess, shareMode, nullptr, creationDisposition, flagsAndAttributes, nullptr));
 
     if (fileHandle.get() == INVALID_HANDLE_VALUE) {
-        qCWarning(lcFileSystem).nospace() << "CreateFileW failed, path=" << path << " errorMessage=" << Utility::formatWinError(GetLastError());
+        const auto lastError = GetLastError();
+        qCWarning(lcFileSystem).nospace() << "CreateFileW failed, path=" << path << " errorMessage=" << Utility::formatWinError(lastError);
         return false;
     }
 
@@ -800,33 +801,67 @@
     {
         PSID sidUnmanaged = nullptr;
         if (!ConvertStringSidToSidW(L"S-1-5-32-545", &sidUnmanaged)) {
-            qCWarning(lcFileSystem).nospace() << "ConvertStringSidToSidW failed, path=" << path << " errorMessage=" << Utility::formatWinError(GetLastError());
+            const auto lastError = GetLastError();
+            qCWarning(lcFileSystem).nospace() << "ConvertStringSidToSidW failed, path=" << path << " errorMessage=" << Utility::formatWinError(lastError);
             return false;
         }
         sid.reset(sidUnmanaged);
     }
 
     ACL_SIZE_INFORMATION aclSize;
     if (!GetAclInformation(resultDacl, &aclSize, sizeof(aclSize), AclSizeInformation)) {
-        qCWarning(lcFileSystem).nospace() << "GetAclInformation failed, path=" << path << " errorMessage=" << Utility::formatWinError(GetLastError());
+        const auto lastError = GetLastError();
+        qCWarning(lcFileSystem).nospace() << "GetAclInformation failed, path=" << path << " errorMessage=" << Utility::formatWinError(lastError);
         return false;
     }
+    qCDebug(lcFileSystem).nospace() << "retrieved ACL size information"
+        << " aclSize.AceCount=" << aclSize.AceCount
+        << " aclSize.AclBytesInUse=" << aclSize.AclBytesInUse
+        << " aclSize.AclBytesFree=" << aclSize.AclBytesFree;
 
-    const auto newAclSize = aclSize.AclBytesInUse + sizeof(ACCESS_DENIED_ACE) + GetLengthSid(sid.get());
+    auto newAclSize = aclSize.AclBytesInUse;
+    if (permissions == FileSystem::FolderPermissions::ReadOnly) {
+        // When marking a folder as read-only also allocate the size of the `ACCESS_DENIED_ACE` excluding the `SidStart`
+        // (`DWORD`) member, + the SID the ACE uses
+        // https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-initializeacl#remarks
+        //
+        // Changing the ACL to read-write will remove all previously defined `ACCESS_DENIED_ACE`s, reducing the ACL size.
+        // There is no need to allocate more than what is already present.
+
+        if (const auto accessDeniedAceSize = sizeof(ACCESS_DENIED_ACE) - sizeof(DWORD) + GetLengthSid(sid.get());
+            newAclSize + accessDeniedAceSize < 65532) {
+            // Apparently the maximum size of an ACL is 65532 bytes (quite close to u16_max).  Any value above that
+            // causes the `InitialiceAcl()` function to fail with "WindowsError 57: The parameter is incorrect".
+            //
+            // Client versions < 4.0.2 had a bug where a new access denied ACE was always prepended when a folder was marked
+            // as read-only.  This worked fine until the ACL was reaching that size limit ...
+            //
+            // Since 4.0.2 the client skips these duplicate access denied ACE entries.  This however only works if the ACL
+            // wasn't already too big.  In that case let's assume that the ACL has grown to that size due to this bug -- the
+            // current ACL is already large enough to hold the single access denied ACE.
+            newAclSize += accessDeniedAceSize;
+        }
+    }
     std::unique_ptr<ACL> newDacl{reinterpret_cast<PACL>(new char[newAclSize])};
     int newAceIndex = 0;
     qCDebug(lcFileSystem) << "allocated a new DACL object of size" << newAclSize;
 
     if (!InitializeAcl(newDacl.get(), newAclSize, ACL_REVISION)) {
-        qCWarning(lcFileSystem).nospace() << "InitializeAcl failed, path=" << path << " errorMessage=" << Utility::formatWinError(GetLastError());
+        const auto lastError = GetLastError();
+        qCWarning(lcFileSystem).nospace() << "InitializeAcl failed,"
+            << " path=" << path
+            << " aclSize.AclBytesInUse=" << aclSize.AclBytesInUse
+            << " newAclSize=" << newAclSize
+            << " errorMessage=" << Utility::formatWinError(lastError);
         return false;
     }
 
     if (permissions == FileSystem::FolderPermissions::ReadOnly) {
         // the access denied ACE needs to appear at the start of the ACL
         if (!AddAccessDeniedAceEx(newDacl.get(), ACL_REVISION, NO_PROPAGATE_INHERIT_ACE,
                                   FILE_DELETE_CHILD | DELETE | FILE_WRITE_DATA | FILE_WRITE_EA | FILE_APPEND_DATA, sid.get())) {
-            qCWarning(lcFileSystem).nospace() << "AddAccessDeniedAceEx failed, path=" << path << " errorMessage=" << Utility::formatWinError(GetLastError());
+            const auto lastError = GetLastError();
+            qCWarning(lcFileSystem).nospace() << "AddAccessDeniedAceEx failed, path=" << path << " errorMessage=" << Utility::formatWinError(lastError);
             return false;
         }
         newAceIndex++;
@@ -835,7 +870,8 @@
     for (int currentAceIndex = 0; currentAceIndex < aclSize.AceCount; ++currentAceIndex) {
         void *currentAce = nullptr;
         if (!GetAce(resultDacl, currentAceIndex, &currentAce)) {
-            qCWarning(lcFileSystem).nospace() << "GetAce failed, path=" << path << " errorMessage=" << Utility::formatWinError(GetLastError());
+            const auto lastError = GetLastError();
+            qCWarning(lcFileSystem).nospace() << "GetAce failed, path=" << path << " errorMessage=" << Utility::formatWinError(lastError);
             return false;
         }
 
@@ -854,9 +890,10 @@
         }
 
         if (!AddAce(newDacl.get(), ACL_REVISION, newAceIndex, currentAce, currentAceHeader->AceSize)) {
+            const auto lastError = GetLastError();
             qCWarning(lcFileSystem).nospace() << "AddAce failed,"
                 << " path=" << path
-                << " errorMessage=" << Utility::formatWinError(GetLastError())
+                << " errorMessage=" << Utility::formatWinError(lastError)
                 << " newAclSize=" << newAclSize
                 << " newAceIndex=" << newAceIndex;
             return false;

src/common/utility_win.cpp

@@ -572,7 +572,8 @@ QString Utility::getCurrentUserName()
     DWORD len = sizeof(username) / sizeof(TCHAR);
 
     if (!GetUserName(username, &len)) {
-        qCWarning(lcUtility) << "Could not retrieve Windows user name." << formatWinError(GetLastError());
+        const auto lastError = GetLastError();
+        qCWarning(lcUtility).nospace() << "Could not retrieve Windows user name. errorMessage=" << formatWinError(lastError);
     }
 
     return QString::fromWCharArray(username);

src/libsync/filesystem.cpp

@@ -480,7 +480,8 @@ bool FileSystem::isFolderReadOnly(const std::filesystem::path &path) noexcept
     fileHandle.reset(CreateFileW(rawLongPath, desiredAccess, shareMode, nullptr, creationDisposition, flagsAndAttributes, nullptr));
 
     if (fileHandle.get() == INVALID_HANDLE_VALUE) {
-        qCWarning(lcFileSystem).nospace() << "CreateFileW failed, path=" << longPath << " errorMessage=" << Utility::formatWinError(GetLastError());
+        const auto lastError = GetLastError();
+        qCWarning(lcFileSystem).nospace() << "CreateFileW failed, path=" << longPath << " errorMessage=" << Utility::formatWinError(lastError);
         return false;
     }
 
@@ -500,7 +501,8 @@ bool FileSystem::isFolderReadOnly(const std::filesystem::path &path) noexcept
 
     ACL_SIZE_INFORMATION aclSize;
     if (!GetAclInformation(resultDacl, &aclSize, sizeof(aclSize), AclSizeInformation)) {
-        qCWarning(lcFileSystem).nospace() << "GetAclInformation failed, path=" << longPath << " errorMessage=" << Utility::formatWinError(GetLastError());
+        const auto lastError = GetLastError();
+        qCWarning(lcFileSystem).nospace() << "GetAclInformation failed, path=" << longPath << " errorMessage=" << Utility::formatWinError(lastError);
         return false;
     }
 

test/testfilesystem.cpp

@@ -1,3 +1,3 @@
 /*
  * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: CC0-1.0
@@ -7,7 +7,7 @@
  * any purpose.
  */
 
 #include <QtTest>
 #include <QTemporaryDir>
 #include <QTemporaryFile>
 
@@ -16,6 +16,12 @@
 
 #include "libsync/filesystem.h"
 
+#ifdef Q_OS_WIN
+#include <securitybaseapi.h>
+#include <aclapi.h>
+#include <sddl.h>
+#endif
+
 using namespace OCC;
 using namespace Qt::StringLiterals;
 namespace std_fs = std::filesystem;
@@ -153,6 +159,167 @@
         // path should now be writable aagain
         QVERIFY(FileSystem::isWritable(path));
     }
+
+#ifdef Q_OS_WIN
+    void testAclWithManyDeniedAces()
+    {
+        // Regression from client versions < 4.0.2; see GH issue nextcloud/desktop#8860
+        //
+        // When a file/folder was set to read-only, the access denied ACE was always
+        // added, eventually leading to failures when modifying the ACL.
+
+        QTemporaryDir tempDir;
+        QDir tempQDir{tempDir.path()};
+        tempQDir.mkpath("readonlyTest");
+
+        // Test case setup: retrieve the ACL and size info from the temporary folder using a win32 HANDLE
+        // implementation adapted from `FileSystem::setAclPermission`
+        const auto testPath = tempDir.filePath("readonlyTest");
+        const auto testPathLong = FileSystem::longWinPath(testPath);
+        const auto testPathRaw = reinterpret_cast<const wchar_t *>(testPathLong.utf16());
+
+        Utility::UniqueHandle fileHandle;
+
+        constexpr SECURITY_INFORMATION securityInfo = DACL_SECURITY_INFORMATION | READ_CONTROL | WRITE_DAC;
+
+        PACL resultDacl = nullptr; // this is a part of the `securityDescriptor` and won't need to be free
+        Utility::UniqueLocalFree<PSECURITY_DESCRIPTOR> securityDescriptor;
+        Utility::UniqueLocalFree<PSID> sid;
+
+        constexpr DWORD desiredAccess = READ_CONTROL | WRITE_DAC | MAXIMUM_ALLOWED;
+        constexpr DWORD shareMode = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
+        constexpr DWORD creationDisposition = OPEN_EXISTING;
+        constexpr DWORD flagsAndAttributes = FILE_ATTRIBUTE_NORMAL | FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT;
+        fileHandle.reset(CreateFileW(testPathRaw, desiredAccess, shareMode, nullptr, creationDisposition, flagsAndAttributes, nullptr));
+
+        if (fileHandle.get() == INVALID_HANDLE_VALUE) {
+            const auto lastError = GetLastError();
+            qCWarning(lcFileSystem).nospace() << "CreateFileW failed, path=" << testPath << " errorMessage=" << Utility::formatWinError(lastError);
+            QVERIFY(false);
+        }
+
+        {
+            PSID sidUnmanaged = nullptr;
+            if (!ConvertStringSidToSidW(L"S-1-5-32-545", &sidUnmanaged)) {
+                const auto lastError = GetLastError();
+                qCWarning(lcFileSystem).nospace() << "ConvertStringSidToSidW failed, path=" << testPath
+                << " errorMessage=" << Utility::formatWinError(lastError);
+                QVERIFY(false);
+            }
+            sid.reset(sidUnmanaged);
+        }
+
+        // Test helper to retrieve the most recent ACL data returning the ACL size.
+        // This will modify resultDacl as side effect, which is fine for this specific test.
+        const auto getAclSizeInformation = [&fileHandle, &resultDacl, &testPath, &securityDescriptor]() -> ACL_SIZE_INFORMATION {
+            PSECURITY_DESCRIPTOR securityDescriptorUnmanaged = nullptr;
+            if (const auto lastError = GetSecurityInfo(fileHandle.get(), SE_FILE_OBJECT, securityInfo, nullptr, nullptr, &resultDacl, nullptr, &securityDescriptorUnmanaged); lastError != ERROR_SUCCESS) {
+                qCWarning(lcFileSystem).nospace() << "GetSecurityInfo failed, path=" << testPath << " errorMessage=" << Utility::formatWinError(lastError);
+                Q_ASSERT(false);
+            }
+            securityDescriptor.reset(securityDescriptorUnmanaged);
+
+            if (!resultDacl) {
+                qCWarning(lcFileSystem).nospace() << "failed to retrieve DACL needed to set a folder read-only or read-write, path=" << testPath;
+                Q_ASSERT(false);
+            }
+
+            ACL_SIZE_INFORMATION aclSize;
+            if (!GetAclInformation(resultDacl, &aclSize, sizeof(aclSize), AclSizeInformation)) {
+                const auto lastError = GetLastError();
+                qCWarning(lcFileSystem).nospace() << "GetAclInformation failed, path=" << testPath << " errorMessage=" << Utility::formatWinError(lastError);
+                Q_ASSERT(false);
+            }
+
+            qCDebug(lcFileSystem).nospace() << "retrieved ACL size information"
+                << " aclSize.AceCount=" << aclSize.AceCount
+                << " aclSize.AclBytesInUse=" << aclSize.AclBytesInUse
+                << " aclSize.AclBytesFree=" << aclSize.AclBytesFree;
+            return aclSize;
+        };
+
+        // ACLs can be up to 65532 bytes in size, so let's add as many ACEs as possible to reproduce the original issue
+        const auto aclSizeInitial = getAclSizeInformation();
+        const auto accessDeniedAceSize = (sizeof(ACCESS_DENIED_ACE) - sizeof(DWORD) + GetLengthSid(sid.get()));
+        const auto accessDeniedAceTotalCount = (65532 - aclSizeInitial.AclBytesInUse) / accessDeniedAceSize;
+
+        auto newAclSize = aclSizeInitial.AclBytesInUse + accessDeniedAceSize * accessDeniedAceTotalCount;
+        std::unique_ptr<ACL> newDacl{reinterpret_cast<PACL>(new char[newAclSize])};
+        int newAceIndex = 0;
+        qCDebug(lcFileSystem) << "allocated a new DACL object of size" << newAclSize;
+
+        if (!InitializeAcl(newDacl.get(), newAclSize, ACL_REVISION)) {
+            const auto lastError = GetLastError();
+            qCWarning(lcFileSystem).nospace() << "InitializeAcl failed,"
+                << " path=" << testPath
+                << " aclSizeInitial.AclBytesInUse=" << aclSizeInitial.AclBytesInUse
+                << " newAclSize=" << newAclSize
+                << " errorMessage=" << Utility::formatWinError(lastError);
+            QVERIFY(false);
+        }
+
+        qCDebug(lcFileSystem) << "adding" << accessDeniedAceTotalCount << "access denied ACEs";
+        for (auto i = 0; i < accessDeniedAceTotalCount; i++) {
+            if (!AddAccessDeniedAceEx(newDacl.get(), ACL_REVISION, NO_PROPAGATE_INHERIT_ACE, FILE_DELETE_CHILD | DELETE | FILE_WRITE_DATA | FILE_WRITE_EA | FILE_APPEND_DATA, sid.get())) {
+                const auto lastError = GetLastError();
+                qCWarning(lcFileSystem).nospace() << "AddAccessDeniedAceEx failed, path=" << testPath << " errorMessage=" << Utility::formatWinError(lastError);
+                QVERIFY(false);
+            }
+            newAceIndex++;
+        }
+
+        // Finally, append the ACEs from the original ACL
+        for (int currentAceIndex = 0; currentAceIndex < aclSizeInitial.AceCount; ++currentAceIndex) {
+            void *currentAce = nullptr;
+            if (!GetAce(resultDacl, currentAceIndex, &currentAce)) {
+                const auto lastError = GetLastError();
+                qCWarning(lcFileSystem).nospace() << "GetAce failed, path=" << testPath << " errorMessage=" << Utility::formatWinError(lastError);
+                QVERIFY(false);
+            }
+
+            const auto currentAceHeader = reinterpret_cast<PACE_HEADER>(currentAce);
+
+            if (!AddAce(newDacl.get(), ACL_REVISION, newAceIndex, currentAce, currentAceHeader->AceSize)) {
+                const auto lastError = GetLastError();
+                qCWarning(lcFileSystem).nospace() << "AddAce failed,"
+                    << " path=" << testPath
+                    << " errorMessage=" << Utility::formatWinError(lastError)
+                    << " newAclSize=" << newAclSize
+                    << " newAceIndex=" << newAceIndex;
+                QVERIFY(false);
+            }
+            newAceIndex++;
+        }
+
+        if (const auto lastError = SetSecurityInfo(fileHandle.get(), SE_FILE_OBJECT, PROTECTED_DACL_SECURITY_INFORMATION | securityInfo, nullptr, nullptr, newDacl.get(), nullptr); lastError != ERROR_SUCCESS) {
+            qCWarning(lcFileSystem).nospace() << "SetSecurityInfo failed, path=" << testPath << " errorMessage=" << Utility::formatWinError(lastError);
+            QVERIFY(false);
+        }
+
+        // Test setup done, so let's verify the behaviour.
+        // ensure we have more ACEs than before and that the used bytes are close to the limit
+        const auto aclSizeDuplicateACEs = getAclSizeInformation();
+        QCOMPARE_GT(aclSizeDuplicateACEs.AceCount, aclSizeInitial.AceCount);
+        QCOMPARE_GT(aclSizeDuplicateACEs.AclBytesInUse, 65511);
+
+        // Try to mark the folder as readonly.  This used to fail because the ACL size was always extended,
+        // even if it wouldn't have been necessary or even possible.
+        QVERIFY(FileSystem::setAclPermission(testPath, FileSystem::FolderPermissions::ReadOnly));
+
+        const auto aclSizeAfterReadOnly = getAclSizeInformation();
+        QCOMPARE_GT(aclSizeAfterReadOnly.AceCount, aclSizeInitial.AceCount);
+        QCOMPARE_LT(aclSizeAfterReadOnly.AceCount, aclSizeDuplicateACEs.AceCount);
+        // The ACLs containing the required ACEs are rather small (usually around 100-200 bytes) -- but it's definitely way less than before:
+        QCOMPARE_LT(aclSizeAfterReadOnly.AclBytesInUse, 512);
+
+        // Finally, remove the read-only ACL :)
+        // This should end up with the same ACEs as before the test.
+        QVERIFY(FileSystem::setAclPermission(testPath, FileSystem::FolderPermissions::ReadWrite));
+        const auto aclSizeAfterReadWrite = getAclSizeInformation();
+        QCOMPARE_EQ(aclSizeAfterReadWrite.AceCount, aclSizeInitial.AceCount);
+        QCOMPARE_EQ(aclSizeAfterReadWrite.AclBytesInUse, aclSizeInitial.AclBytesInUse);
+    }
+#endif
 };
 
 QTEST_GUILESS_MAIN(TestFileSystem)