Hello
Right now, slapd ignore attribute ACL when performing an add operation.
If you have privilegied users that can add entries, it means that you can prevent them from modifying attributes in existing entries, but you cannot prevent them from adding an entry with a read-only attribute.
The problem can be interesting with an attribute such as authzTo, where the whole access control can be circumvented by any user that can create an entry in the tree. IMO this behavior was not intended, but if it was, then it should be clearly documented.
Below is a patch that cause attribute ACL to be checked for add operations. It is done in the backend, so if it is acceptable, then I will have to do it for other backends. I wonder if the modrdn operation shoulnd't be subject to the same sanity checks.
Any thought? Does it look right?
diff -U2 -r1.174 add.c --- servers/slapd/back-bdb/add.c 26 Aug 2008 23:45:35 -0000 1.174 +++ servers/slapd/back-bdb/add.c 27 Sep 2008 15:54:58 -0000 @@ -300,4 +300,22 @@ }
+ /* + * Check ACL for attribute write access + */ + if (!acl_check_modlist(op, oe, op->ora_modlist)) { + switch( opinfo.boi_err ) { + case DB_LOCK_DEADLOCK: + case DB_LOCK_NOTGRANTED: + goto retry; + } + + Debug( LDAP_DEBUG_TRACE, + LDAP_XSTRING(bdb_add) ": no write access to attribute\n", + 0, 0, 0 ); + rs->sr_err = LDAP_INSUFFICIENT_ACCESS; + rs->sr_text = "no write access to attribute"; + goto return_results;; + } + if ( eid == NOID ) { rs->sr_err = bdb_next_id( op->o_bd, &eid );
----- "Emmanuel Dreyfus" manu@netbsd.org ha scritto:
Hello
Right now, slapd ignore attribute ACL when performing an add operation.
If you have privilegied users that can add entries, it means that you can prevent them from modifying attributes in existing entries, but you cannot prevent them from adding an entry with a read-only attribute.
The problem can be interesting with an attribute such as authzTo, where the whole access control can be circumvented by any user that can create an entry in the tree. IMO this behavior was not intended, but if it was, then it should be clearly documented.
Below is a patch that cause attribute ACL to be checked for add operations. It is done in the backend, so if it is acceptable, then I will have to do it for other backends. I wonder if the modrdn operation shoulnd't be subject to the same sanity checks.
Any thought? Does it look right?
diff -U2 -r1.174 add.c --- servers/slapd/back-bdb/add.c 26 Aug 2008 23:45:35 -0000 1.174 +++ servers/slapd/back-bdb/add.c 27 Sep 2008 15:54:58 -0000 @@ -300,4 +300,22 @@ }
/** Check ACL for attribute write access*/if (!acl_check_modlist(op, oe, op->ora_modlist)) {switch( opinfo.boi_err ) {case DB_LOCK_DEADLOCK:case DB_LOCK_NOTGRANTED:goto retry;}Debug( LDAP_DEBUG_TRACE,LDAP_XSTRING(bdb_add) ": no write access toattribute\n",
0, 0, 0 );rs->sr_err = LDAP_INSUFFICIENT_ACCESS;rs->sr_text = "no write access to attribute";goto return_results;;}if ( eid == NOID ) { rs->sr_err = bdb_next_id( op->o_bd, &eid );-- Emmanuel Dreyfus http://hcpnet.free.fr/pubz manu@netbsd.org
See ITS#4556 for discussion.
p.
Ing. Pierangelo Masarati OpenLDAP Core Team
SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ----------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Fax: +39 0382 476497 Email: ando@sys-net.it -----------------------------------
Pierangelo Masarati ando@sys-net.it wrote:
See ITS#4556 for discussion.
So this is not considered a security hole. But as far as I understand, anyone that is allowed to add an entry anywhere can add a user with random privileges. Did I miss something here? Can that be avoided?
----- Emmanuel Dreyfus manu@netbsd.org ha scritto:
Pierangelo Masarati ando@sys-net.it wrote:
See ITS#4556 for discussion.
So this is not considered a security hole. But as far as I understand, anyone that is allowed to add an entry anywhere can add a user with random privileges. Did I miss something here? Can that be avoided?
I'm not necessarily saying that. For the problem you highlight, setting
authz-policy from
would cure it (one could only add an entry and authorize as that entry, but not add an entry and use it to authorize as some other existing one).
What I'm saying that apparently the "right" manner to handle the problem you pose consists in implementing DIT content rules.
p.
Ing. Pierangelo Masarati OpenLDAP Core Team
SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ----------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Fax: +39 0382 476497 Email: ando@sys-net.it -----------------------------------
Pierangelo Masarati wrote:
----- Emmanuel Dreyfusmanu@netbsd.org ha scritto:
Pierangelo Masaratiando@sys-net.it wrote:
See ITS#4556 for discussion.
So this is not considered a security hole. But as far as I understand, anyone that is allowed to add an entry anywhere can add a user with random privileges. Did I miss something here? Can that be avoided?
I'm not necessarily saying that. For the problem you highlight, setting
authz-policy from
would cure it (one could only add an entry and authorize as that entry, but not add an entry and use it to authorize as some other existing one).
What I'm saying that apparently the "right" manner to handle the problem you
pose consists in implementing DIT content rules.
I disagree. http://www.openldap.org/lists/openldap-devel/200709/msg00079.html
ditContentRules and ditStructutreRules don't give you the fine-grained control that's required here.
I think Emmanuel's patch looks correct, and the corresponding patch needs to be made for a lot of other backends.
Howard Chu hyc@symas.com wrote:
I think Emmanuel's patch looks correct, and the corresponding patch needs to be made for a lot of other backends.
Cool, I can do that. Two other questions:
1) do we want an option to enable this behavior? The change could affect existing setups that rely on this "feature"
2) should modrdn be fixed the same way? Other operations?
Emmanuel Dreyfus wrote:
Howard Chuhyc@symas.com wrote:
I think Emmanuel's patch looks correct, and the corresponding patch needs to be made for a lot of other backends.
Cool, I can do that. Two other questions:
- do we want an option to enable this behavior? The change could affect
existing setups that rely on this "feature"
I'm inclined not to have a particular option for this. It's simply plugging a long-standing hole.
- should modrdn be fixed the same way? Other operations?
I'm not yet convinced. What's the scenario you see here?
Howard Chu hyc@symas.com wrote:
- should modrdn be fixed the same way? Other operations?
I'm not yet convinced. What's the scenario you see here?
I have the right to move users from a branch to another, but ACL restrict some atttributes (e.g.: gidNumber) depending on the branch. A modrdn allows me to circunvent the ACL.
That's a bit far fetched, but I wonder if some setups could benefit from such a check.
----- Howard Chu hyc@symas.com ha scritto:
Emmanuel Dreyfus wrote:
Howard Chuhyc@symas.com wrote:
I think Emmanuel's patch looks correct, and the corresponding patch needs to be made for a lot of other backends.
Cool, I can do that. Two other questions:
- do we want an option to enable this behavior? The change could affect
existing setups that rely on this "feature"
I'm inclined not to have a particular option for this. It's simply plugging a long-standing hole.
As I said, I agrfee about the hole; however, I remember raising this issue myself earlier and receiving a satisfactory response about the fact that the current software complies with specs. I need to dig this out.
- should modrdn be fixed the same way? Other operations?
I'm not yet convinced. What's the scenario you see here?
Unless one uses authzTo/authzFrom as a naming attribute, I don't see any issue. I haven't checked, but I believe modrdn already needs to comply with ACLs in a manner that allows finge-grain enough control. In fact, modrdn needs to pass access control both for the old and the new (r)dn, and the use of filters, sets and so allows to condition access on the entry's content.
p.
Ing. Pierangelo Masarati OpenLDAP Core Team
SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ----------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Fax: +39 0382 476497 Email: ando@sys-net.it -----------------------------------
Pierangelo Masarati ando@sys-net.it wrote:
Unless one uses authzTo/authzFrom as a naming attribute, I don't see any issue. I haven't checked, but I believe modrdn already needs to comply with ACLs in a manner that allows finge-grain enough control. In fact, modrdn needs to pass access control both for the old and the new (r)dn, and the use of filters, sets and so allows to condition access on the entry's content.
Looking at the code for back-bdb, it requires you have: - write access to old parent - write access to new superior parent - write access to old entry - and there is a call to bdb_modify_internal() that will check for attribute ACL, and it this seems to be for the old location in the tree, not the new one.
So if you have an attribute ACL that applies only to the new location, it can be circunvented by a modrdn. I am not sure this is really a bug, though, perhaps just an unspecfied area.
Howard Chu hyc@symas.com wrote:
I think Emmanuel's patch looks correct, and the corresponding patch needs to be made for a lot of other backends.
I did a backend tour, and here am I:
I have a patch to fix the problem: back-bdb back-hdb (is a link to back-bdb) back-ndb
backend has no add operation, and therefore does not need a fix: back-dnssrv back-null back-passwd back-monitor (add operation implemented in plugins)
backend does not implement access control: back-ldap back-ldif back-meta back-perl back-relay back-shell back-sock back-sql
Emmanuel Dreyfus wrote:
Howard Chuhyc@symas.com wrote:
I think Emmanuel's patch looks correct, and the corresponding patch needs to be made for a lot of other backends.
I did a backend tour, and here am I:
Sounds good.
bconfig.c (cn=config) needs it as well.
I have a patch to fix the problem: back-bdb back-hdb (is a link to back-bdb) back-ndb
backend has no add operation, and therefore does not need a fix: back-dnssrv back-null back-passwd back-monitor (add operation implemented in plugins)
backend does not implement access control: back-ldap back-ldif back-meta back-perl back-relay back-shell back-sock back-sql
Howard Chu hyc@symas.com wrote:
bconfig.c (cn=config) needs it as well.
Right, I missed that one, but it was easy to fix it too.
Pierangelo Masarati ando@sys-net.it wrote:
I'm not necessarily saying that. For the problem you highlight, setting authz-policy from would cure it
Unfortuately, authz-policy to seems to be mandatory if you want to configure slapo-chain: in that scenario, the replica connects to the master as a pseudo-user that must have authTo: * if you want it to perform update on behalf of the user that did the modification.
On Sep 27, 2008, at 8:59 AM, Emmanuel Dreyfus wrote:
Hello
Right now, slapd ignore attribute ACL when performing an add operation.
I note that this is the expected behavior, been so for many, many years.
If you have privilegied users that can add entries, it means that you can prevent them from modifying attributes in existing entries, but you cannot prevent them from adding an entry with a read-only attribute.
The problem can be interesting with an attribute such as authzTo, where the whole access control can be circumvented by any user that can create an entry in the tree. IMO this behavior was not intended, but if it was,
It was. Likewise the behavior of rename.
then it should be clearly documented.
I recall it being noted somewhere in the documents, but likely not as clear as it should be. I recall discussing this ACL/authzTo issue long ago.
Below is a patch that cause attribute ACL to be checked for add operations. It is done in the backend, so if it is acceptable, then I will have to do it for other backends. I wonder if the modrdn operation shoulnd't be subject to the same sanity checks.
Any thought? Does it look right?
I have no opinion as whether this change should be made or not. I'm merely providing some background information....
-- Kurt
diff -U2 -r1.174 add.c --- servers/slapd/back-bdb/add.c 26 Aug 2008 23:45:35 -0000 1.174 +++ servers/slapd/back-bdb/add.c 27 Sep 2008 15:54:58 -0000 @@ -300,4 +300,22 @@ }
/** Check ACL for attribute write access*/if (!acl_check_modlist(op, oe, op->ora_modlist)) {switch( opinfo.boi_err ) {case DB_LOCK_DEADLOCK:case DB_LOCK_NOTGRANTED:goto retry;}Debug( LDAP_DEBUG_TRACE,LDAP_XSTRING(bdb_add) ": no write access toattribute\n",
0, 0, 0 );rs->sr_err = LDAP_INSUFFICIENT_ACCESS;rs->sr_text = "no write access to attribute";goto return_results;;}if ( eid == NOID ) { rs->sr_err = bdb_next_id( op->o_bd, &eid );-- Emmanuel Dreyfus http://hcpnet.free.fr/pubz manu@netbsd.org
Kurt Zeilenga wrote:
On Sep 27, 2008, at 8:59 AM, Emmanuel Dreyfus wrote:
Hello
Right now, slapd ignore attribute ACL when performing an add operation.
I note that this is the expected behavior, been so for many, many years.
Yes, but it never really made much sense - it means you can be forbidden from modifying an existing record to contain certain privileged data, but not forbidden from creating records with privileged data. It makes sense to me that your ability to create particular data values should not depend on whether you're creating it for the very first time, or some subsequent time.
(Coming at it from the opposite direction, Delete of course requires you to permit the Delete even if certain attributes are read-only; since every entry contains read-only operational attributes, deletes would be impossible without this provision.)
If you have privilegied users that can add entries, it means that you can prevent them from modifying attributes in existing entries, but you cannot prevent them from adding an entry with a read-only attribute.
The problem can be interesting with an attribute such as authzTo, where the whole access control can be circumvented by any user that can create an entry in the tree. IMO this behavior was not intended, but if it was,
It was. Likewise the behavior of rename.
then it should be clearly documented.
I recall it being noted somewhere in the documents, but likely not as clear as it should be. I recall discussing this ACL/authzTo issue long ago.
Below is a patch that cause attribute ACL to be checked for add operations. It is done in the backend, so if it is acceptable, then I will have to do it for other backends. I wonder if the modrdn operation shoulnd't be subject to the same sanity checks.
Any thought? Does it look right?
I have no opinion as whether this change should be made or not. I'm merely providing some background information....
-- Kurt
diff -U2 -r1.174 add.c --- servers/slapd/back-bdb/add.c 26 Aug 2008 23:45:35 -0000 1.174 +++ servers/slapd/back-bdb/add.c 27 Sep 2008 15:54:58 -0000 @@ -300,4 +300,22 @@ }
/** Check ACL for attribute write access*/if (!acl_check_modlist(op, oe, op->ora_modlist)) {switch( opinfo.boi_err ) {case DB_LOCK_DEADLOCK:case DB_LOCK_NOTGRANTED:goto retry;}Debug( LDAP_DEBUG_TRACE,LDAP_XSTRING(bdb_add) ": no write access toattribute\n",
0, 0, 0 );rs->sr_err = LDAP_INSUFFICIENT_ACCESS;rs->sr_text = "no write access to attribute";goto return_results;;}if ( eid == NOID ) { rs->sr_err = bdb_next_id( op->o_bd,&eid );-- Emmanuel Dreyfus http://hcpnet.free.fr/pubz manu@netbsd.org
Howard Chu wrote:
Kurt Zeilenga wrote:
On Sep 27, 2008, at 8:59 AM, Emmanuel Dreyfus wrote:
Hello
Right now, slapd ignore attribute ACL when performing an add operation.
I note that this is the expected behavior, been so for many, many years.
Yes, but it never really made much sense - it means you can be forbidden from modifying an existing record to contain certain privileged data, but not forbidden from creating records with privileged data. It makes sense to me that your ability to create particular data values should not depend on whether you're creating it for the very first time, or some subsequent time.
(Coming at it from the opposite direction, Delete of course requires you to permit the Delete even if certain attributes are read-only; since every entry contains read-only operational attributes, deletes would be impossible without this provision.)
Well, the user doesn't add operational attrs either, so it is perfectly symmetric that read-only attributes are automatically destroyed by users with "delete" permission on "entry".
In any case, I note that fixing this issue broke test006 (at least).
p.
Ing. Pierangelo Masarati OpenLDAP Core Team
SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ----------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Fax: +39 0382 476497 Email: ando@sys-net.it -----------------------------------
Pierangelo Masarati ando@sys-net.it wrote:
In any case, I note that fixing this issue broke test006 (at least).
I think this is going to break many setups that had a security hole but nobody was aware of it.
A database option can make everyone happy, but is there anyone complaining?
Emmanuel Dreyfus wrote:
Pierangelo Masarati ando@sys-net.it wrote:
In any case, I note that fixing this issue broke test006 (at least).
I think this is going to break many setups that had a security hole but nobody was aware of it.
I mean: test006 is broken now, we can no longer make test. You should check why the test is broken and try to fix it :) Probably, according to the old access rule, a user with "add" permission for entries is adding an entry without having "add" permission on all the attributes.
A database option can make everyone happy, but is there anyone complaining?
I'm not particularly in favor of a config option as soon as we're happy with the fix.
p.
Ing. Pierangelo Masarati OpenLDAP Core Team
SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ----------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Fax: +39 0382 476497 Email: ando@sys-net.it -----------------------------------
Pierangelo Masarati ando@sys-net.it wrote:
I mean: test006 is broken now, we can no longer make test. You should check why the test is broken and try to fix it :) Probably, according to the old access rule, a user with "add" permission for entries is adding an entry without having "add" permission on all the attributes.
The culprit is the ACL on attrs=objectclass at the top of the file: access to attrs=objectclass by * =rsc stop
If I change it that way, test006 passes: access to attrs=objectclass by dn.exact="cn=Bjorn Jensen,ou=Information Technology Division,ou=People,dc=example,dc=com" add by * =rsc stop
Not sure it is a correct fix, through.
Emmanuel Dreyfus wrote:
Pierangelo Masarati ando@sys-net.it wrote:
I mean: test006 is broken now, we can no longer make test. You should check why the test is broken and try to fix it :) Probably, according to the old access rule, a user with "add" permission for entries is adding an entry without having "add" permission on all the attributes.
The culprit is the ACL on attrs=objectclass at the top of the file: access to attrs=objectclass by * =rsc stop
If I change it that way, test006 passes: access to attrs=objectclass by dn.exact="cn=Bjorn Jensen,ou=Information Technology Division,ou=People,dc=example,dc=com" add by * =rsc stop
Not sure it is a correct fix, through.
Sounds correct. I mean: since no objectClass modification was performed in the test, given the expected behavior of access control for add operations, there was no need to give anyone add permission on objectClass. What you suggest seems to be the minimal add permission to let the test pass, and I think it's fine to re-enable that test right now. Should the test change (more add operations) acls will be tweaked further.
Go ahead and commit :)
p.
Ing. Pierangelo Masarati OpenLDAP Core Team
SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ----------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Fax: +39 0382 476497 Email: ando@sys-net.it -----------------------------------
Pierangelo Masarati ando@sys-net.it wrote:
Go ahead and commit :)
Done.
Kurt Zeilenga Kurt@OpenLDAP.org wrote:
Right now, slapd ignore attribute ACL when performing an add operation.
I note that this is the expected behavior, been so for many, many years.
(...)
I have no opinion as whether this change should be made or not. I'm merely providing some background information....
I read your message just after I did commit the change. Perhaps we should have a backend option to enable it?
-
Howard Chu -
Kurt Zeilenga -
manu＠netbsd.org -
Pierangelo Masarati