https://fedorahosted.org/389/ticket/47571https://fedorahosted.org/389/attachment/ticket/47571/0001-Ticket-47571-targ…
Description: commit 85a78741dfeb636a1cf7cced1576278e65f5bb58
<https://fedorahosted.org/389/changeset/85a78741dfeb636a1cf7cced1576278e65f5…>
introduced 2 coverity issues:
12423: Explicit null dereferenced
do_search (slapd/search.c)
If attribute list contains multiple "*"s and "aci"s in ldapsearch,
the previous code attempted to add "aci" once for "*" and replacing
"aci" with normalized aci (if any) once and eliminating the duplicated
"aci"s. The code contained a null dereferenced bug. Even if duplicated
attributes are included in the attribute list, they are removed later
in send_specific_attrs. Thus, this patch simplifies the logic to avoid
the null dereference.
12422: Logically dead code
comp_cmp (slapd/attr.c)
Eliminated the dead code (case s1 == NULL AND s2 == NULL).
On Thu, 2014-01-09 at 17:37 -0500, Simo Sorce wrote:
> On Thu, 2014-01-09 at 16:32 -0500, Nathaniel McCallum wrote:
> > This patch is independent from my patches 0028-0031 and can be merged in
> > any order.
> >
> > This patch has a bug, but I can't figure it out. We need to set
> > nsslapd-access-userattr-strict on cn=config to "off".
>
> Uhmm what is the effect on ACL evaluation of changing this boolean ?
> I can;t figure out from your commit not from 389ds commit what exactly
> changes and how it impacts the security of the directory.
>
> I ask because I was planning on using userattr to protect some
> operations in the password plugin but was waiting due to bug:
> https://fedorahosted.org/389/ticket/47571 which is beeing resolved.
>
> I want to make sure your change won't change what this ACIs would allow.
>
> Is this option simply allowing the use of add/delete ACIs to be
> specified in conjunction with userattr, so that a user can add an attr
> only if it contains its own DN ?
>
> Will it allow the user to add multiple values to the same attr as long
> as one of the is the userDN ? O will it restrict that case ?
>
> (I know that ipaTokenOwner is a single-value attribute, but the
> mechanism you are enabling here is general, and I want to be sure of
> what the semantics are)
After testing, it was determined that the 389DS patch #47653 does in
fact permit addition if any of the multi-valued attributes match the
condition. This is definitely problematic.
After discussion today with nkinder, simo, nhosoi, we agreed to
roll-back patch #47653 and find an alternate approach. This also
invalidates patch freeipa-npmccallum-0032. Simo will follow up this
email with an alternate proposal.
Nathaniel
https://fedorahosted.org/389/ticket/47571https://fedorahosted.org/389/attachment/ticket/47571/0001-Ticket-47571-targ…
Description:
Subtypes in targetattr, userattr in aci as well as filter and attribute list
in the search are supported.
* If targetattr contains subtypes, the base type only as well as other
subtypes
are not allowed to access (or denied to access).
* If userattr contains subtypes, the base type as well as other subtypes in
entries do not match the userattr value.
* If attribute list in search has a base type attribute, and a
targetattr has
a type with subtypes, then only the subtyped value is returned. E.g.,
attribute list: sn
targetattr: sn;en
==>
sn;en: <sn-en-value> and
sn;en;phonetic: <sn-en-phonetic-value> are returned
but
sn or sn;fr is not.
If attribute list has a type with subtype, then if the targetattr
allows the
subtype, the value is returned. E.g.,
attribute list: sn;en
targetattr: sn;en
==>
sn;en: <sn-en-value> and
sn;en;phonetic: <sn-en-phonetic-value> are returned
but
sn or sn;fr is not.
1) slapd/attr.c
* slapi_attr_type_cmp assumed the subtype order in 2 args are identical,
but it is not always guaranteed. Removed the assumption.
* Added another compare type SLAPI_TYPE_CMP_SUBTYPES to comp_cmp
which is
called by slapi_attr_type_cmp to support full subtypes comparison.
2) plugin/acl.c:
* Changed to call slapi_attr_type_cmp with human readable macros, e.g.,
SLAPI_TYPE_CMP_BASE, SLAPI_TYPE_CMP_SUBTYPE, etc.
* Replaced strcasecmp with slapi_attr_type_cmp for attribute type
comparison.
* Changed to call slapi_attr_type_cmp with SLAPI_TYPE_CMP_SUBTYPES (full
subtype comparison) in acl__get_attrEval, where the next attribute to
compare is determined.
3) slapd/search.c,result.c
send_all_attrs/send_specific_attrs use a dontsendattr array to
control the
duplicate attribute types. Replaced the logic with a simpler one by
creating
an charray with no duplicates.
I'm working on this project: http://www.freeipa.org/page/V3/OTP
Users need to be able to create, edit and delete their own tokens. Each
token has an attribute: ipatokenOwner.
I attempted creating this ACL: (target =
"ldap:///ipatokenuniqueid=*,cn=otp,dc=example,dc=com")(targetfilter =
"(objectClass=ipaToken)")(version 3.0; acl "token-add-delete"; allow
(add, delete) userattr = "ipatokenOwner#USERDN";)
After much debugging I found out this is impossible because of this:
https://git.fedorahosted.org/cgit/389/ds.git/tree/ldap/servers/plugins/acl/…
Now, in the general case, I can very much understand why this shouldn't
be allowed by default. What alternatives are there with the current
code? Would 389DS be willing to accept a patch to enable this (with a
I_KNOW_WHAT_I_AM_DOING flag)?
The general reason why this feature works in my case is that each object
created restricts the user, rather than granting new privileges. This
seems like a valid use case.
Nathaniel
I just posted this patch:
https://www.redhat.com/archives/freeipa-devel/2013-December/msg00229.html
389ds may be interested in the ISO 8601 parser contained in the patch.
It offers two main advantages to the one already contained in the 389ds
tree:
1. It is *far* more flexible in what it can parse.
2. It is thoroughly tested (currently ~15k tests).
Nathaniel