Spring Security Spring ACL permissions are not bitmask-based

Describe the bug If I grant a user the .....W permission (mask 3) and check it against a document I annotated with @PreAuthorize("hasPermission(id, typ, 1)") (or READ instead of 1) it doesn't have the permission.

Additionally, if I guard a resource with @PreAuthorize("hasPermission(id, typ, 3)"), I wouldn't expect a user with just R or W (1 or 2) to be able to access this resource.

This is due to this code - which even has a comment above, that states that you could implement bitmask matching with (given&expected)!=0.

    protected boolean isGranted(AccessControlEntry ace, Permission p) {
        return ace.getPermission().getMask() == p.getMask();
    }

While the documentation states this

Finally, ACL_ENTRY stores the individual permissions assigned to each recipient. Columns include a foreign key to the ACL_OBJECT_IDENTITY, the recipient (i.e. a foreign key to ACL_SID), whether we’ll be auditing or not, and the integer bit mask that represents the actual permission being granted or denied. We have a single row for every recipient that receives a permission to work with a domain object.

As mentioned in the last paragraph, the ACL system uses integer bit masking. However, you need not be aware of the finer points of bit shifting to use the ACL system. Suffice it to say that we have 32 bits we can switch on or off. Each of these bits represents a permission. By default, the permissions are read (bit 0), write (bit 1), create (bit 2), delete (bit 3), and administer (bit 4). You can implement your own Permission instance if you wish to use other permissions, and the remainder of the ACL framework operates without knowledge of your extensions.

You should understand that the number of domain objects in your system has absolutely no bearing on the fact that we have chosen to use integer bit masking. While you have 32 bits available for permissions, you could have billions of domain object instances (which means billions of rows in ACL_OBJECT_IDENTITY and, probably, ACL_ENTRY). We make this point because we have found that people sometimes mistakenly that believe they need a bit for each potential domain object, which is not the case.

Which for me clearly identifies, that an ACL entry with RW- should give permission to a R-guarded entry and giving just an R-permission shouldn't give permission to an RW-guarded resource.

To Reproduce First, insert for a user+object (in a class extending JdbcMutableAclService)

        MutableAcl acl = (MutableAcl) this.readAclById(new ObjectIdentityImpl('id', 'typ'));
        Permission perm = permissionFactory.buildFromMask(permissionMask);
        acl.insertAce(atIndexPosition, perm, new PrincipalSid(sid), true);

Then, put a "@PreAuthorize("hasPermission('id', 'typ', READ)") on a method and send a request as that user.

Expected behavior The behavior stated in the documentation: The user (SID) must have exactly the same or more bits set in his aquired ACEs than the required bits.

I'd also say, that for the multi-bit-checking-case, it should only work if a user has exactly the required bits after masking (== p.getMask() not !=0) but I guess that's somewhat of a niche, unintended corner case.

Another option would be to fully rework the whole documentation that's talking about bitmasks, as the DefaultPermissionGrantingStrategy is effectively not bitmask-logic-compliant, and the suggested alternative isGranted (in the comment) could even be a real security vulnerability, as it would grant the moment one bit in the bitmask is true, whereas the current version is just too restrictive to my understanding.

Sample

I built a minimal reproducible sample - including tests for the original, the suggested !=0 and a better ==r.getMask() here and respective tests in the test directory.

I also built a test for the case of checking bitmasks (shouldReadOnAddedAcePermissions) that would require a full rewrite of public boolean isGranted(Acl acl, List<Permission> permission, List<Sid> sids, boolean administrativeMode) as the expected behaviour (for me) would be that granting read and write in a separate ACE would pass when doing a isGranted when checking for read+write.