(zookeeper) 02/05: setACL race — missing acl attribute setting

[an...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

anmolnar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zookeeper.git

commit 4571e5fb1af2832c30830417dc229fa940c6a424
Author: Andor Molnar <an...@cloudera.com>
AuthorDate: Fri Sep 18 10:49:13 2026 -0500

    setACL race — missing acl attribute setting
---
 .../zookeeper/server/PrepRequestProcessor.java     |  8 +++
 .../zookeeper/server/PrepRequestProcessorTest.java | 65 ++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
index d263fd75c4..0eb266bb7e 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
@@ -555,6 +555,14 @@ protected void pRequest2Txn(int type, long zxid, Request request, Record record)
             request.setTxn(new SetACLTxn(path, listACL, newVersion));
             nodeRecord = nodeRecord.duplicate(request.getHdr().getZxid());
             nodeRecord.stat.setAversion(newVersion);
+            // Publish the new ACL onto the outstanding ChangeRecord. getRecordForPath()
+            // serves this record (in preference to the committed tree) to every request
+            // prepped before this setACL commits, so without this line those requests are
+            // authorized against the stale, pre-revocation ACL and their writes linearize
+            // after the revocation (TOCTOU). Mirrors the create path, which already sets
+            // the ACL on its ChangeRecord. ACL is not part of the node digest, so this
+            // does not affect digest calculation.
+            nodeRecord.acl = listACL;
             nodeRecord.precalculatedDigest = precalculateDigest(
                     DigestOpCode.UPDATE, path, nodeRecord.data, nodeRecord.stat);
             setTxnDigest(request, nodeRecord.precalculatedDigest);
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java
index 3cf993abbb..1d25ad8e00 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java
@@ -49,6 +49,7 @@
 import org.apache.zookeeper.proto.CreateRequest;
 import org.apache.zookeeper.proto.ReconfigRequest;
 import org.apache.zookeeper.proto.RequestHeader;
+import org.apache.zookeeper.proto.SetACLRequest;
 import org.apache.zookeeper.proto.SetDataRequest;
 import org.apache.zookeeper.server.ZooKeeperServer.ChangeRecord;
 import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
@@ -292,6 +293,70 @@ public void testInvalidPath() throws Exception {
         assertEquals(outcome.getException().code(), KeeperException.Code.BADARGUMENTS);
     }
 
+    /**
+     * A setACL that is prepped but not yet committed must publish the NEW ACL onto
+     * its outstanding ChangeRecord. getRecordForPath() serves that record to every
+     * request prepped before the setACL commits, so if it still carries the old ACL
+     * those requests are authorized against the pre-revocation ACL (TOCTOU).
+     *
+     * This is the direct regression test for the missing
+     * {@code nodeRecord.acl = listACL;} in the setACL case: before the fix the
+     * outstanding record keeps the old OPEN_ACL_UNSAFE; after the fix it carries the
+     * new read-only ACL.
+     */
+    @Test
+    public void testSetACLPublishesNewAclOnOutstandingChangeRecord() throws Exception {
+        zks.getZKDatabase().dataTree.createNode("/foo", new byte[0], Ids.OPEN_ACL_UNSAFE, 0, 0, 0, 0);
+        assertNull(zks.outstandingChangesForPath.get("/foo"));
+
+        pLatch = new CountDownLatch(1);
+        processor = new PrepRequestProcessor(zks, new MyRequestProcessor());
+        SetACLRequest setAcl = new SetACLRequest("/foo", Ids.READ_ACL_UNSAFE, -1);
+        // admin identity so the ADMIN permission check is not what is under test here
+        processor.pRequest(createRequest(setAcl, OpCode.setACL, true));
+        assertTrue(pLatch.await(5, TimeUnit.SECONDS), "request hasn't been processed in chain");
+
+        ChangeRecord cr = zks.outstandingChangesForPath.get("/foo");
+        assertNotNull(cr, "Change record wasn't set");
+        assertEquals(Ids.READ_ACL_UNSAFE, cr.acl,
+                "Outstanding ChangeRecord must carry the new ACL so later preps are checked against it");
+    }
+
+    /**
+     * End-to-end (at the prep layer) version of the above: a client's write that is
+     * prepped while an ACL revocation is still outstanding must be checked against the
+     * NEW ACL and denied. Before the fix the write is authorized against the stale ACL
+     * and would commit after the revocation.
+     */
+    @Test
+    public void testRacingWriteAfterAclRevocationIsDenied() throws Exception {
+        // Node is world-writable to start with (OPEN_ACL_UNSAFE grants ALL to world:anyone).
+        zks.getZKDatabase().dataTree.createNode("/foo", new byte[0], Ids.OPEN_ACL_UNSAFE, 0, 0, 0, 0);
+
+        processor = new PrepRequestProcessor(zks, new MyRequestProcessor());
+
+        // 1. A world client revokes write access: setACL -> read-only. This is prepped and
+        //    left in outstandingChangesForPath (not yet committed). OPEN_ACL_UNSAFE grants
+        //    ADMIN, so the world client is allowed to perform the setACL.
+        pLatch = new CountDownLatch(1);
+        SetACLRequest setAcl = new SetACLRequest("/foo", Ids.READ_ACL_UNSAFE, -1);
+        processor.pRequest(createRequest(setAcl, OpCode.setACL, false));
+        assertTrue(pLatch.await(5, TimeUnit.SECONDS), "setACL hasn't been processed in chain");
+        assertNull(outcome.getException(), "setACL revocation should succeed");
+
+        // 2. The same world client now tries to write while the revocation is still
+        //    outstanding. It must be checked against the new read-only ACL and denied.
+        pLatch = new CountDownLatch(1);
+        SetDataRequest setData = new SetDataRequest("/foo", "evil".getBytes(), -1);
+        processor.pRequest(createRequest(setData, OpCode.setData, false));
+        assertTrue(pLatch.await(5, TimeUnit.SECONDS), "setData hasn't been processed in chain");
+
+        assertEquals(OpCode.error, outcome.getHdr().getType(),
+                "write racing an outstanding ACL revocation must fail");
+        assertEquals(KeeperException.Code.NOAUTH, outcome.getException().code(),
+                "write racing an outstanding ACL revocation must be denied against the new ACL");
+    }
+
     private class MyRequestProcessor implements RequestProcessor {
 
         @Override