#48354 Review default installation ACI's
Closed: fixed a month ago by mreynolds. Opened 3 years ago by firstyear.

The default ACI's we provide use rules like:

(targetattr !="cn || sn || uid")

These rules aren't best practice, as it's very easy to create "targetattr=*" by accident through some simple mistakes with != rules in aci. This will create rules that look good on review, but don't function as expected.

The default ACI's in DS should be altered to all be "targetattr =", to help encourage good ACI practice.


I disagree that this is bad practice. There are cases where you just want to prevent access to a few attributes like "aci" or "userpassword" - without using "!=" all attributes would have to be listed, this can also be error prone, forgetting some attribute, or any new addition of an objectclass to an entry would mean to change the acis. Maintaining acis in a dynamic database could become difficult.

The flip side of this is that if you have the following rules:

targetAttr!=userPassword self write.

targetAttr!=objectClass self write.

You have actually now allowed * self write on an object. Anywhere where two != rules overlap in scope at all, in a recipe to open up access to many attributes beyond the intent of the administrator. Despite appearing to be correct upon inspection, these rules are a security disaster.

This comes down to the fact that != rules are inverted to "=" rules internally, and the resultant set of attributes is a union.

Additionally, even one of these rules on its own actually allows an object to self write or access many internal attributes including:

nsSizeLimit
nsAccountLock
aci

This is why I do not think we should be encouraging the use of !=. There are too many dangers. Yes, it means we have to add extra attrs if we change object class. But it prevents issues where people "think" they are secure, but really aren't. I dread to think how many installations of 389ds are vulnerable to these: My previous workplace was affected for more than 10 years and no one ever realised.

If you want to block access to a few attrs, you should use targetAttr="foo" deny in my view.

Indeed, (targetattr!=userpassword) and (targetattr!=uid) makes (targetattr=).
{{{
aci: (target=ldap:///dc=example,dc=com)(targetattr!=userpassword)(version 3.0; acl "acl1"; allow(read,search,compare,write) userdn = "ldap:///self";)
aci: (target=ldap:///dc=example,dc=com)(targetattr!=uid)(version 3.0; acl "acl11"; allow(read,search,compare,write) userdn = "ldap:///self";)
aci: (target=ldap:///dc=example,dc=com)(targetattr=
)(version 3.0; acl "acl2"; allow(write) groupdn = "ldap:///cn=Directory Administrators, dc=example,dc=com";)

/usr/lib64/mozldap/ldapsearch -h localhost -p 389 -D "uid=tuser0,dc=example,dc=com" -w tuser0 -b "dc=example,dc=com" -J "1.3.6.1.4.1.42.2.27.9.5.2:true:dn: uid=tuser0,dc=example,dc=com" "(uid=)"
[...]
entryLevelRights: v
attributeLevelRights: objectClass:rscwo, cn:rscwo, sn:rscwo, uid:rscwo, givenN
ame:rscwo, userPassword:rscwo, secretary:rscwo
}}}
FYI, one (targetattr!=userpassword) is correctly interpreted.
{{{
aci: (target=ldap:///dc=example,dc=com)(targetattr!=userpassword)(version 3.0; acl "acl1"; allow(read,search,compare,write) userdn = "ldap:///self";)
aci: (target=ldap:///dc=example,dc=com)(targetattr=
)(version 3.0; acl "acl2"; allow(write) groupdn = "ldap:///cn=Directory Administrators, dc=example,dc=com";)

/usr/lib64/mozldap/ldapsearch -h localhost -p 389 -D "uid=tuser0,dc=example,dc=com" -w tuser0 -b "dc=example,dc=com" -J "1.3.6.1.4.1.42.2.27.9.5.2:true:dn: uid=tuser0,dc=example,dc=com" "(uid=*)"
[...]
entryLevelRights: v
attributeLevelRights: objectClass:rscwo, cn:rscwo, sn:rscwo, uid:rscwo, givenN
ame:rscwo, userPassword:none, secretary:rscwo
}}}
But I'd rather think this is a bug in the acl code, which should treat (targetattr!=userpassword) and (targetattr!=uid) as (&(targetattr!=userpassword)(targetattr!=uid)) instead of current (|(targetattr!=userpassword)(targetattr!=uid))?

I also wonder IPA uses targetattr!=<type>?

I agree with that, that it's a bug and we should treat it as: (&(targetattr!=userpassword)(targetattr!=uid))

However, this is probably more invasive than changing default aci's. We are changing fundamentals of aci evaluation, and this could really break user setups where they may rely on this behaviour without realising.

IPA does not use the != type, I checked this at the time. It would be worth re-confirming this though.

Replying to [comment:4 firstyear]:

I agree with that, that it's a bug and we should treat it as: (&(targetattr!=userpassword)(targetattr!=uid))

However, this is probably more invasive than changing default aci's. We are changing fundamentals of aci evaluation, and this could really break user setups where they may rely on this behaviour without realising.

Well, but I doubt the author of the ACLs of (targetattr!=userpassword) and (targetattr!=uid) intends to specify (targetattr=*)... I'd think it is a sort of security issue...

IPA does not use the != type, I checked this at the time. It would be worth re-confirming this though.

Good news! Thanks!!

Replying to [comment:5 nhosoi]:

Replying to [comment:4 firstyear]:

I agree with that, that it's a bug and we should treat it as: (&(targetattr!=userpassword)(targetattr!=uid))

However, this is probably more invasive than changing default aci's. We are changing fundamentals of aci evaluation, and this could really break user setups where they may rely on this behaviour without realising.

Well, but I doubt the author of the ACLs of (targetattr!=userpassword) and (targetattr!=uid) intends to specify (targetattr=*)... I'd think it is a sort of security issue...

Yes, I agree it's a security issue.

I mean they may rely on it by:

{{{
(targetattr != a || b) read
(targetattr != c || d) read
}}}

And without realising, they have an application that depends on attr a, b, c or d to read. They don't have an aci to allow the read, but it "just works" so they never thought to question ....

I think that this change, despite being security related, will break some customer setups, so if we do this it should have a big warning, documentation, perhaps even notify support teams to be ready to expect calls about it ...

DECISION: File a doc bug. Move to FUTURE.

Can we do something like this for now???

aci: ((targetattr!="userPassword") && (targetattr!="aci"))(version 3.0; acl "Enable anonymous access"; allow (read, search, compare) userdn="ldap:///anyone";)

We could, but this doesn't fix say entrywsi, nsSizeLimit, createdBy, etc .....

Also, wouldn't this be equivalent:

{{{
aci: (targetattr!="userPassword || aci")(version 3.0; acl "Enable anonymous access"; allow (read, search, compare) userdn="ldap:///anyone";)
}}}

I think is a candidate list to not display out of box:

{{{
creatorsName:
modifiersName:
createTimestamp:
modifyTimestamp:
parentid:
entryid:
entrydn:
numSubordinates:
}}}

There may be some others I have missed off the top of my head.

commit 3c2cd48
Writing objects: 100% (9/9), 1.90 KiB | 0 bytes/s, done.
Total 9 (delta 7), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
4c2656d..3c2cd48 master -> master

I'm going to push this back to 1.3.7 backlog, because I think it depends a bit on some python work.

Metadata Update from @nhosoi:
- Issue assigned to firstyear
- Issue set to the milestone: 1.3.7 backlog

2 years ago

Metadata Update from @firstyear:
- Issue close_status updated to: None
- Issue set to the milestone: 1.4 backlog (was: 1.3.7 backlog)

2 years ago

Metadata Update from @mreynolds:
- Issue assigned to mreynolds (was: firstyear)

a year ago

We should pursue changing this in 1.4.0, and officially adding @firstyear's aci lint tool:

https://fy.blackhats.net.au/blog/html/2016/04/01/389_ds_aci_linting_tool.html

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to None

a year ago

dscreate now adds these new kind of revised aci's in it's sample ldifs. Fixed in 389-ds-base-1.4.x

Metadata Update from @mreynolds:
- Issue close_status updated to: fixed
- Issue set to the milestone: 1.4.0 (was: 1.4 backlog)
- Issue status updated to: Closed (was: Open)

a month ago

Login to comment on this ticket.

Metadata