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.

attachment
0001-Ticket-48354-Review-of-default-ACI-in-the-directory-.patch

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.

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

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

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1685

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.