ZOOKEEPER-1877. Malformed ACL Id can crash server with skipACL=yes (Chris Chen via phunt)

git-svn-id: https://svn.apache.org/repos/asf/zookeeper/trunk@1613328 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -738,6 +738,9 @@ BUGFIXES:
   ZOOKEEPER-1937. init script needs fixing for ZOOKEEPER-1719
   (Marshall McMullen via phunt)
 
+  ZOOKEEPER-1877. Malformed ACL Id can crash server with skipACL=yes
+  (Chris Chen via phunt)
+
 IMPROVEMENTS:
 
   ZOOKEEPER-1170. Fix compiler (eclipse) warnings: unused imports,

src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java

@@ -376,10 +376,7 @@ public class PrepRequestProcessor extends ZooKeeperCriticalThread implements
                 String path = createRequest.getPath();
                 String parentPath = validatePathForCreate(path, request.sessionId);
 
-                List<ACL> listACL = removeDuplicates(createRequest.getAcl());
-                if (!fixupACL(request.authInfo, listACL)) {
-                    throw new KeeperException.InvalidACLException(path);
-                }
+                List<ACL> listACL = fixupACL(path, request.authInfo, createRequest.getAcl());
                 ChangeRecord parentRecord = getRecordForPath(parentPath);
 
                 checkACL(zks, parentRecord.acl, ZooDefs.Perms.CREATE, request.authInfo);
@@ -429,10 +426,7 @@ public class PrepRequestProcessor extends ZooKeeperCriticalThread implements
                 String path = createRequest.getPath();
                 String parentPath = validatePathForCreate(path, request.sessionId);
 
-                List<ACL> listACL = removeDuplicates(createRequest.getAcl());
-                if (!fixupACL(request.authInfo, listACL)) {
-                    throw new KeeperException.InvalidACLException(path);
-                }
+                List<ACL> listACL = fixupACL(path, request.authInfo, createRequest.getAcl());
                 ChangeRecord parentRecord = getRecordForPath(parentPath);
 
                 checkACL(zks, parentRecord.acl, ZooDefs.Perms.CREATE, request.authInfo);
@@ -633,10 +627,7 @@ public class PrepRequestProcessor extends ZooKeeperCriticalThread implements
                 if(deserialize)
                     ByteBufferInputStream.byteBuffer2Record(request.request, setAclRequest);
                 path = setAclRequest.getPath();
-                List<ACL> listACL = removeDuplicates(setAclRequest.getAcl());
-                if (!fixupACL(request.authInfo, listACL)) {
-                    throw new KeeperException.InvalidACLException(path);
-                }
+                List<ACL> listACL = fixupACL(path, request.authInfo, setAclRequest.getAcl());
                 nodeRecord = getRecordForPath(path);
                 checkACL(zks, nodeRecord.acl, ZooDefs.Perms.ADMIN, request.authInfo);
                 newVersion = checkAndIncVersion(nodeRecord.stat.getAversion(), setAclRequest.getVersion(), path);
@@ -921,62 +912,57 @@ public class PrepRequestProcessor extends ZooKeeperCriticalThread implements
      * depend on the requestor's authentication information.
      *
      * @param authInfo list of ACL IDs associated with the client connection
-     * @param acl list of ACLs being assigned to the node (create or setACL operation)
-     * @return
+     * @param acls list of ACLs being assigned to the node (create or setACL operation)
+     * @return verified and expanded ACLs
+     * @throws KeeperException.InvalidACLException
      */
-    private boolean fixupACL(List<Id> authInfo, List<ACL> acl) {
-        if (skipACL) {
-            return true;
-        }
-        if (acl == null || acl.size() == 0) {
-            return false;
+    private List<ACL> fixupACL(String path, List<Id> authInfo, List<ACL> acls)
+        throws KeeperException.InvalidACLException {
+        // check for well formed ACLs
+        // This resolves https://issues.apache.org/jira/browse/ZOOKEEPER-1877
+        List<ACL> uniqacls = removeDuplicates(acls);
+        LinkedList<ACL> rv = new LinkedList<ACL>();
+        if (uniqacls == null || uniqacls.size() == 0) {
+            throw new KeeperException.InvalidACLException(path);
         }
-
-        Iterator<ACL> it = acl.iterator();
-        LinkedList<ACL> toAdd = null;
-        while (it.hasNext()) {
-            ACL a = it.next();
+        for (ACL a: uniqacls) {
+            LOG.debug("Processing ACL: {}", a);
+            if (a == null) {
+                throw new KeeperException.InvalidACLException(path);
+            }
             Id id = a.getId();
+            if (id == null || id.getScheme() == null) {
+                throw new KeeperException.InvalidACLException(path);
+            }
             if (id.getScheme().equals("world") && id.getId().equals("anyone")) {
-                // wide open
+                rv.add(a);
             } else if (id.getScheme().equals("auth")) {
                 // This is the "auth" id, so we have to expand it to the
                 // authenticated ids of the requestor
-                it.remove();
-                if (toAdd == null) {
-                    toAdd = new LinkedList<ACL>();
-                }
                 boolean authIdValid = false;
                 for (Id cid : authInfo) {
                     AuthenticationProvider ap =
                         ProviderRegistry.getProvider(cid.getScheme());
                     if (ap == null) {
                         LOG.error("Missing AuthenticationProvider for "
-                                + cid.getScheme());
+                            + cid.getScheme());
                     } else if (ap.isAuthenticated()) {
                         authIdValid = true;
-                        toAdd.add(new ACL(a.getPerms(), cid));
+                        rv.add(new ACL(a.getPerms(), cid));
                     }
                 }
                 if (!authIdValid) {
-                    return false;
+                    throw new KeeperException.InvalidACLException(path);
                 }
             } else {
                 AuthenticationProvider ap = ProviderRegistry.getProvider(id.getScheme());
-                if (ap == null) {
-                    return false;
-                }
-                if (!ap.isValid(id.getId())) {
-                    return false;
+                if (ap == null || !ap.isValid(id.getId())) {
+                    throw new KeeperException.InvalidACLException(path);
                 }
+                rv.add(a);
             }
         }
-        if (toAdd != null) {
-            for (ACL a : toAdd) {
-                acl.add(a);
-            }
-        }
-        return acl.size() > 0;
+        return rv;
     }
 
     public void processRequest(Request request) {

src/java/test/org/apache/zookeeper/test/ClientSkipACLTest.java

@@ -0,0 +1,34 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+
+public class ClientSkipACLTest extends ClientTest {
+    @BeforeClass
+    static public void setup() {
+        System.setProperty("zookeeper.skipACL", "yes");
+    }
+
+    @AfterClass
+    static public void teardown() {
+        System.clearProperty("zookeeper.skipACL");
+    }
+}

src/java/test/org/apache/zookeeper/test/ClientTest.java

@@ -54,6 +54,7 @@ import org.slf4j.LoggerFactory;
 
 public class ClientTest extends ClientBase {
     protected static final Logger LOG = LoggerFactory.getLogger(ClientTest.class);
+    private boolean skipACL = System.getProperty("zookeeper.skipACL", "no").equals("yes");
 
     /** Verify that pings are sent, keeping the "idle" client alive */
     @Test
@@ -142,23 +143,43 @@ public class ClientTest extends ClientBase {
                 LOG.info("Test successful, invalid acl received : "
                         + e.getMessage());
             }
+            try {
+                ArrayList<ACL> testACL = new ArrayList<ACL>();
+                testACL.add(new ACL(Perms.ALL | Perms.ADMIN, new Id()));
+                zk.create("/nullidtest", new byte[0], testACL, CreateMode.PERSISTENT);
+                Assert.fail("Should have received an invalid acl error");
+            } catch(InvalidACLException e) {
+                LOG.info("Test successful, invalid acl received : "
+                        + e.getMessage());
+            }
             zk.addAuthInfo("digest", "ben:passwd".getBytes());
-            zk.create("/acltest", new byte[0], Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
+            ArrayList<ACL> testACL = new ArrayList<ACL>();
+            testACL.add(new ACL(Perms.ALL, new Id("auth","")));
+            testACL.add(new ACL(Perms.WRITE, new Id("ip", "127.0.0.1")));
+            zk.create("/acltest", new byte[0], testACL, CreateMode.PERSISTENT);
             zk.close();
             zk = createClient();
             zk.addAuthInfo("digest", "ben:passwd2".getBytes());
-            try {
-                zk.getData("/acltest", false, new Stat());
-                Assert.fail("Should have received a permission error");
-            } catch (KeeperException e) {
-                Assert.assertEquals(Code.NOAUTH, e.code());
+            if (skipACL) {
+                try {
+                    zk.getData("/acltest", false, null);
+                } catch (KeeperException e) {
+                    Assert.fail("Badauth reads should succeed with skipACL.");
+                }
+            } else {
+                try {
+                    zk.getData("/acltest", false, null);
+                    Assert.fail("Should have received a permission error");
+                } catch (KeeperException e) {
+                    Assert.assertEquals(Code.NOAUTH, e.code());
+                }
             }
             zk.addAuthInfo("digest", "ben:passwd".getBytes());
-            zk.getData("/acltest", false, new Stat());
+            zk.getData("/acltest", false, null);
             zk.setACL("/acltest", Ids.OPEN_ACL_UNSAFE, -1);
             zk.close();
             zk = createClient();
-            zk.getData("/acltest", false, new Stat());
+            zk.getData("/acltest", false, null);
             List<ACL> acls = zk.getACL("/acltest", new Stat());
             Assert.assertEquals(1, acls.size());
             Assert.assertEquals(Ids.OPEN_ACL_UNSAFE, acls);
@@ -176,6 +197,48 @@ public class ClientTest extends ClientBase {
         }
     }
 
+    @Test
+    public void testNullAuthId() throws Exception {
+        ZooKeeper zk = null;
+        try {
+            zk = createClient();
+            zk.addAuthInfo("digest", "ben:passwd".getBytes());
+            ArrayList<ACL> testACL = new ArrayList<ACL>();
+            testACL.add(new ACL(Perms.ALL, new Id("auth", null)));
+            zk.create("/acltest", new byte[0], testACL, CreateMode.PERSISTENT);
+            zk.close();
+            zk = createClient();
+            zk.addAuthInfo("digest", "ben:passwd2".getBytes());
+            if (skipACL) {
+                try {
+                    zk.getData("/acltest", false, null);
+                } catch (KeeperException e) {
+                    Assert.fail("Badauth reads should succeed with skipACL.");
+                }
+            } else {
+                try {
+                    zk.getData("/acltest", false, null);
+                    Assert.fail("Should have received a permission error");
+                } catch (KeeperException e) {
+                    Assert.assertEquals(Code.NOAUTH, e.code());
+                }
+            }
+            zk.addAuthInfo("digest", "ben:passwd".getBytes());
+            zk.getData("/acltest", false, null);
+            zk.setACL("/acltest", Ids.OPEN_ACL_UNSAFE, -1);
+            zk.close();
+            zk = createClient();
+            zk.getData("/acltest", false, null);
+            List<ACL> acls = zk.getACL("/acltest", new Stat());
+            Assert.assertEquals(1, acls.size());
+            Assert.assertEquals(Ids.OPEN_ACL_UNSAFE, acls);
+        } finally {
+            if (zk != null) {
+                zk.close();
+            }
+        }
+    }
+
     private class MyWatcher extends CountdownWatcher {
         LinkedBlockingQueue<WatchedEvent> events =
             new LinkedBlockingQueue<WatchedEvent>();