#47591 entries with empty objectclass attribute value can be hidden
Closed: Fixed None Opened 5 years ago by lkrispen.

The following entry is not returned by a search:

dn: ....
objectClass;adcsn-5283b8e0000000c80000;vucsn-5283b8e0000000c80000: top
objectClass;vucsn-5283b8e0000000c80000: person
objectClass;vucsn-5283b8e0000000c80000: organizationalPerson
objectClass;vucsn-5283b8e0000000c80000: inetOrgPerson
objectClass;vdcsn-5283b8e0000000c80000;deleted:


The problem is that for the empty value

objectClass;vdcsn-5283b8e0000000c80000;deleted

it is compared to "ldapsubentry" and "nstombstone"

if (PL_strncasecmp(type.bv_val,"objectclass",type.bv_len) == 0) {
454 if (PL_strncasecmp(value.bv_val,"ldapsubentry",value.bv_len) == 0)
455 e->e_flags |= SLAPI_ENTRY_LDAPSUBENTRY;
456 if (PL_strncasecmp(value.bv_val, SLAPI_ATTR_VALUE_TOMBSTONE,value.bv_len) == 0)

There are two bugs here:
- the value is deleted, so it should not be considered as tombstone or subentry, even if it would match
- the strings are only compared up to the value length (0 in this case).

And the problem is not only for this specific value comparison.
Eg if the schema is extended to define an attribute: "nsu"

Then the test:
if ( PL_strncasecmp (type.bv_val, SLAPI_ATTR_UNIQUEID, type.bv_len) == 0 )

is true and takes the duplicate nsuniqueid branch.

In general all the calls
PL_strncasecmp (bv_val, <string>, bv_len) == 0

need to be checked and replaced.
The reason for using strNcasecmp is probably that bv_vals are not guaranteed to be NULL terminated, although it seems to be the case inside str2entry_fast()

Alternative 1: PL_strcasecmp (bv_val, <string>, bv_len) == 0

Alternative 2: (bv_len >= strlen(<string>) && PL_strncasecmp (bv_val, <string>, bv_len) == 0 )
It has to be ">=" and not "==" because we have types like:

(gdb) p type
$11 = {bv_len = 38, bv_val = 0x7f07f800155a "objectClass"}
(gdb) x/s 0x7f07f800155a
0x7f07f800155a: "objectClass"
(gdb) x/s 0x7f07f800155a+12
0x7f07f8001566: "vucsn-5284a364000000c80000"

Alternative 1:

Replying to [comment:2 lkrispen]:

And the problem is not only for this specific value comparison.
Eg if the schema is extended to define an attribute: "nsu"

Then the test:
if ( PL_strncasecmp (type.bv_val, SLAPI_ATTR_UNIQUEID, type.bv_len) == 0 )

is true and takes the duplicate nsuniqueid branch.

In general all the calls
PL_strncasecmp (bv_val, <string>, bv_len) == 0

need to be checked and replaced.
The reason for using strNcasecmp is probably that bv_vals are not guaranteed to be NULL terminated, although it seems to be the case inside str2entry_fast()

Alternative 1: PL_strcasecmp (bv_val, <string>, bv_len) == 0

Alternative 2: (bv_len >= strlen(<string>) && PL_strncasecmp (bv_val, <string>, bv_len) == 0 )
It has to be ">=" and not "==" because we have types like:

(gdb) p type
$11 = {bv_len = 38, bv_val = 0x7f07f800155a "objectClass"}
(gdb) x/s 0x7f07f800155a
0x7f07f800155a: "objectClass"
(gdb) x/s 0x7f07f800155a+12
0x7f07f8001566: "vucsn-5284a364000000c80000"

What if the user has an attribute type like
(gdb) p type
$11 = {bv_len = 14, bv_val = 0x7f07f800155a "objectClassExt"}
?

Alternative 1:

It should be handled correctly by both versions.
V2: strcasecmp returns != 0
V1: strncasecmp says it compares up to len or a string termonator, whatever comes first, so
strncasecmp ("objectclass", objectclassext",14) should also return != 0

I was wrong.

strncasecmp stops at len or the first null terminator and compares only the strings before,

so
strncasecmp ("objectclass", objectclassext",4)
strncasecmp ("objectclass", objectclassext",11)
strncasecmp ("objectclass", objectclassext",14)
strncasecmp ("objectclass", objectclassext",17)
all return equal

I'm sorry, I had written a small testprogramm and made a typo to always compare "objectclass","objectclass".

After fixing, the result is:
strncasecmp ("objectclass", objectclassext",4) EQUAL
strncasecmp ("objectclass", objectclassext",11) EQUAL
strncasecmp ("objectclass", objectclassext",14) NOT EQUAL
strncasecmp ("objectclass", objectclassext",17) NOT EQUAL

In general, I prefer the explicit length checking (v1). In these cases:
{{{

  • if ( PL_strncasecmp( type.bv_val, "dn", type.bv_len ) == 0 ) {
  • if ( type.bv_len >= 2 && PL_strncasecmp( type.bv_val, "dn", type.bv_len ) == 0 ) {
    }}}
    If type > 2, then PL_strncasecmp( "dna", "dn", 3 ) != 0
    I don't think you need the >= for this case, or rdn, entrydn, or uniqueid, since none of these will ever be of the form entrydn;vucsn-xxxxxx.

Do not call strlen on the constant strings "objectclass" or "ldapsubentry" or SLAPI_ATTR_VALUE_TOMBSTONE. I don't know if the compiler will optimize this into a sizeof or other constant, but best to define this outside of str2entry and use a real constant value for the length.

I think I'm missing something
{{{
(gdb) p type
$11 = {bv_len = 38, bv_val = 0x7f07f800155a "objectClass"}
(gdb) x/s 0x7f07f800155a
0x7f07f800155a: "objectClass"
(gdb) x/s 0x7f07f800155a+12
0x7f07f8001566: "vucsn-5284a364000000c80000"
}}}
Does the code find the first ';' in the string and write a '\0' there? And that's how we are able to do PL_strncasecmp(type.bv_val,"objectclass",type.bv_len) even though type.bv_len is > strlen("objectclass")?

I think I'm missing something
{{{
(gdb) p type
$11 = {bv_len = 38, bv_val = 0x7f07f800155a "objectClass"}
(gdb) x/s 0x7f07f800155a
0x7f07f800155a: "objectClass"
(gdb) x/s 0x7f07f800155a+12
0x7f07f8001566: "vucsn-5284a364000000c80000"
}}}
Does the code find the first ';' in the string and write a '\0' there? And that's how we are able to do PL_strncasecmp(type.bv_val,"objectclass",type.bv_len) even though type.bv_len is > strlen("objectclass")?

yes. In
str2entry_state_information_from_type(type.bv_val,&valuecsnset,&attributedeletioncsn,&maxcsn,&value_state,&attr_state);

it replaces the first ";" by "\0" and parses the rest into csnset,......

Replying to [comment:7 rmeggins]:

If type > 2, then PL_strncasecmp( "dna", "dn", 3 ) != 0
I don't think you need the >= for this case, or rdn, entrydn, or uniqueid, since none of these will ever be of the form entrydn;vucsn-xxxxxx.

for these types it is teh other way round, if you define a custom schema attr "nsu", then you can have:
$11 = {bv_len = 3, bv_val = 0x7f07f800155a "nsu"} and get
PL_strncasecmp (type.bv_val, SLAPI_ATTR_UNIQUEID, type.bv_len) == 0

Replying to [comment:9 lkrispen]:

Replying to [comment:7 rmeggins]:

If type > 2, then PL_strncasecmp( "dna", "dn", 3 ) != 0
I don't think you need the >= for this case, or rdn, entrydn, or uniqueid, since none of these will ever be of the form entrydn;vucsn-xxxxxx.

for these types it is teh other way round, if you define a custom schema attr "nsu", then you can have:
$11 = {bv_len = 3, bv_val = 0x7f07f800155a "nsu"} and get
PL_strncasecmp (type.bv_val, SLAPI_ATTR_UNIQUEID, type.bv_len) == 0

Right. Sorry, I wasn't clear. You don't need "type.bv_len >= N" you just need "type.bv_len == N".

This version is still using ">=" instead of "==" for things like dn, rdn, etc. Unless they really need to be >=, I would prefer ==.

sorry, new patch attached

$git merge ticket47591-1
Updating 6b83caf..3a6ce92
Fast-forward
ldap/servers/slapd/entry.c | 15 ++++++++-------
ldap/servers/slapd/slapi-plugin.h | 15 ++++++++++++++-
2 files changed, 22 insertions(+), 8 deletions(-)
$ git push origin master
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 1.28 KiB, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
6b83caf..3a6ce92 master -> master

Metadata Update from @lkrispen:
- Issue assigned to lkrispen
- Issue set to the milestone: 1.2.11.26

2 years ago

Login to comment on this ticket.

Metadata