ACL: Implement a custom AclImpl doesn't make sense

[skram]

When an ACE entry is retrieved from the database, the mask value is compared inside the isGranted(List<Permission> permission, List<Sid> sids, boolean administrativeMode) method under the org.springframework.security.acls.domain.AclImpl implementation.

Here's the code:

if ((ace.getPermission().getMask() == p.getMask()) && ace.getSid().equals(sid)) {
...
}

The problem with this comparison is it compares the actual values. If in the database the mask is stored as int 3 (which corresponds to the binary value 101) with meaning Write and Read access, and you mark your service method for example:

@PreAuthorize("hasPermission(#object, 'READ')")
public AnyType get(MyObject object) {
...
}

The equivalent mask for this by default is int 1 (which corresponds to the binary value of 001).

When these two values (3 == 1) are compared by ace.getPermission().getMask() == p.getMask(), the result is of course, false.

By setting the mask in the database as int 3, we assumed that if you mark your method with READ permission, it will be granted because int 3 means READ and WRITE access.

As a workaround, I have to implement a custom AclImpl and modify the line with the following:

if (( (ace.getPermission().getMask() | p.getMask()) == p.getMask()) && ace.getSid().equals(sid)) {
...
}

This is a bitwise OR comparison. It works. But the issue here is why do I need to implement my own logic. Shouldn't this be the default logic.

Another workaround is in the ACE entry in the database is to add two entries for the same object. The first entry has mask 1. The second entry has mask 2. This gives the effect of having a READ and WRITE access. This also allows me to use the default AclImpl. But at the expense of creating multiple entries in the database.

Actually there's another workaround. Extend the org.springframework.security.acls.domain.BasePermi ssion and add another Permission. For example:

public static final Permission READWRITE = new CustomBasePermission(3, 'Z'); // 3

I would like to use WR instead of Z but the constructor only allows a char. This is a custom permission that can be used on hasPermission expression. For example:

@PreAuthorize("hasPermission(#post, 'READWRITE')")

The advantage of this workaround is I don't need to declare a custom AclImpl. The disadvantage of this implementation is now I have to mark my methods specifically with READWRITE instead of just READ or WRITE.

I know my question isn't easy to digest. Well, security is a complex beast

[Luke Taylor]

You'll find this kind of thing has been discussed quite extensively in Jira. Start with
SEC-1166 and its related issues.

[ko0kie]

As a workaround, I have to implement a custom AclImpl and modify the line with the following:

if (( (ace.getPermission().getMask() | p.getMask()) == p.getMask()) && ace.getSid().equals(sid)) {
...
}

Are you sure that OR is adequate operator? 0011 (ACE: READ, WRITE) | 0001 (Required Right: READ) => 0011 != 0001 Authorization failed.

Instead I would recommend AND & operator: 0011 & 0001 => 0001 == 0001 Authorization successful.