diff --git a/jspwiki-main/src/main/java/org/apache/wiki/auth/permissions/PermissionFactory.java b/jspwiki-main/src/main/java/org/apache/wiki/auth/permissions/PermissionFactory.java index 760c879f4b..eb546ec850 100644 --- a/jspwiki-main/src/main/java/org/apache/wiki/auth/permissions/PermissionFactory.java +++ b/jspwiki-main/src/main/java/org/apache/wiki/auth/permissions/PermissionFactory.java @@ -38,10 +38,10 @@ public final class PermissionFactory private PermissionFactory() {} /** - * This is a WeakHashMap, which stores the + * This is a WeakHashMap, which stores the * cached page permissions. */ - private static final WeakHashMap c_cache = new WeakHashMap<>(); + private static final WeakHashMap c_cache = new WeakHashMap<>(); /** * Get a permission object for a WikiPage and a set of actions. @@ -79,15 +79,13 @@ private static PagePermission getPagePermission( final String wiki, String page, { PagePermission perm; // - // Since this is pretty speed-critical, we try to avoid the StringBuffer creation - // overhead by XORring the hashcodes. However, if page name length > 32 characters, - // this might result in two same hashCodes. - // FIXME: Make this work for page-name lengths > 32 characters (use the alt implementation - // if page.length() > 32?) - // Alternative implementation below, but it does create an extra StringBuffer. - //String key = wiki+":"+page+":"+actions; - - final Integer key = wiki.hashCode() ^ page.hashCode() ^ actions.hashCode(); + // The cache key must uniquely identify the (wiki, page, actions) triple. The previous + // XOR-of-hashCodes key allowed two different pages to collide onto the same 32-bit + // value, so an access check for one page could silently be evaluated against another + // page's cached permission (and hence the wrong ACL). The concatenated string key is + // collision-free for any attacker-choosable page name. + // + final String key = wiki + ":" + page + ":" + actions; // // It's fine if two threads update the cache, since the objects mean the same diff --git a/jspwiki-main/src/test/java/org/apache/wiki/auth/permissions/PermissionFactoryTest.java b/jspwiki-main/src/test/java/org/apache/wiki/auth/permissions/PermissionFactoryTest.java new file mode 100644 index 0000000000..6bcbccc65f --- /dev/null +++ b/jspwiki-main/src/test/java/org/apache/wiki/auth/permissions/PermissionFactoryTest.java @@ -0,0 +1,43 @@ +/* + 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.wiki.auth.permissions; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class PermissionFactoryTest +{ + + @Test + public void testHashCollidingPageNamesGetDistinctPermissions() + { + // "Aa" and "BB" have identical String hashCodes; the old XOR-of-hashCodes cache key + // returned the first page's cached permission for the second page, so ACL checks + // could be evaluated against the wrong page. + Assertions.assertEquals( "Aa".hashCode(), "BB".hashCode() ); + + final PagePermission p1 = PermissionFactory.getPagePermission( "Aa", "view" ); + final PagePermission p2 = PermissionFactory.getPagePermission( "BB", "view" ); + + Assertions.assertEquals( "Aa", p1.getPage() ); + Assertions.assertEquals( "BB", p2.getPage() ); + Assertions.assertNotSame( p1, p2 ); + } + +}