search

389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
211 stars 91 forks source link

Review default installation ACI's #1685

Closed 389-ds-bot closed 4 years ago

389-ds-bot commented 4 years ago

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/48354

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.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2015-12-03 22:55:55

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.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2015-12-04 03:29:38

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.

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2016-01-20 06:02:52

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!=?

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2016-01-20 06:09:14

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.

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2016-01-20 06:14:40

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!!

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2016-01-20 06:28:40

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 ...

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2016-02-04 11:42:34

DECISION: File a doc bug. Move to FUTURE.

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2016-06-17 22:49:08

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";)

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2016-06-20 10:22:36

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";)
389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2016-06-21 07:04:24

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.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2016-06-21 07:16:40

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

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2016-06-21 10:48:09

commit 3c2cd48b7d2cb0579f7de6d460bcd0c9bb1157bd 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

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2017-02-01 04:31:57

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

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2017-02-11 22:52:51

Metadata Update from @nhosoi:

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2017-05-05 01:06:31

Metadata Update from @Firstyear:

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-06-22 18:24:51

Metadata Update from @mreynolds389:

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-06-22 18:29:33

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

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-06-22 18:29:34

Metadata Update from @mreynolds389:

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2019-08-21 21:33:32

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-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2019-08-21 21:33:33

Metadata Update from @mreynolds389: