Commit bc5449b6 authored by Patrick D. Hunt's avatar Patrick D. Hunt
Browse files

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
parent 1fd0ce83
......@@ -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,
......
......@@ -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,31 +912,33 @@ 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;
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);
}
if (acl == null || acl.size() == 0) {
return false;
for (ACL a: uniqacls) {
LOG.debug("Processing ACL: {}", a);
if (a == null) {
throw new KeeperException.InvalidACLException(path);
}
Iterator<ACL> it = acl.iterator();
LinkedList<ACL> toAdd = null;
while (it.hasNext()) {
ACL a = it.next();
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 =
......@@ -955,28 +948,21 @@ public class PrepRequestProcessor extends ZooKeeperCriticalThread implements
+ 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);
}
if (toAdd != null) {
for (ACL a : toAdd) {
acl.add(a);
rv.add(a);
}
}
return acl.size() > 0;
return rv;
}
public void processRequest(Request request) {
......
/**
* 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");
}
}
\ No newline at end of file
......@@ -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());
if (skipACL) {
try {
zk.getData("/acltest", false, new Stat());
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>();
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment