Server control requests get eaten after a NULL character in the berval

[phillipod]

Hi,

I was looking into the syncrepl issue reported by Nat Sincheler, which I guess is the syncrepl issue you mentioned in #14. I don't think I've identified his issue, but we're definitely not handling BER correctly - in what appears to be a mirror of #28

net-ldapapi/LDAPapi.pm: 

sub create_control {
# SNIP
    my $status = ldap_create_control($oid, $berval, length($berval), $critical, $ctrl);
# SNIP
}

net-ldapapi/LDAPapi.xs:

int ldap_create_control(oid, bv_val, bv_len, iscritical, ctrlp) {
/* SNIP */
        ctrl->ldctl_value.bv_val = ber_strdup(bv_val);
        ctrl->ldctl_value.bv_len = bv_len;
/* SNIP */
}

openldap/libraries/liblber/memory.c:

char * ber_strdup( LDAP_CONST char *s ) {
        return ber_strdup_x( s, NULL );
}

char *ber_strdup_x( LDAP_CONST char *s, void *ctx ) {
        char    *p;
        size_t  len;
/* SNIP */
        len = strlen( s ) + 1;
        if ( (p = ber_memalloc_x( len, ctx )) != NULL ) {
                AC_MEMCPY( p, s, len );
        }

        return p;
}

If a berval containing an otherwise valid '\x00' byte is attempted to be passed, all data after the '\x00' byte will be discarded due to strlen() stopping at '\x00', say for instance the 'beforeCount' on a VLV control.

Using the sample code in #28, but changing 'beforeCount' to 0, I get this:

===== Server Side Sort and Virtual List View
 + Loading ASN for SortKeyList, SortResult, VirtualListViewRequest, VirtualListViewResponse
 + Constructing Server Side Sort control
sss ctrl berval =
[0000]   30 13 30 11  04 02 73 6E  80 08 32 2E  35 2E 31 33   0.0. ..sn ..2. 5.13
[0010]   2E 33 81 01  FF                                      .3.. .
 + Constructing Virtual List View control
vlv ctrl berval =
[0000]   30 0E 02 01  00 02 01 03  A0 06 02 01  01 02 01 00   0... .... .... ....
 + Searching for results
 - Fail
Protocol error
 + Parsing first entry
perl: error.c:255: ldap_parse_result: Assertion `r != ((void *)0)' failed.
Aborted

However, with the following change:

--- LDAPapi.xs.old      2015-06-14 04:54:23.000000000 +1200
+++ LDAPapi.xs  2015-12-01 00:53:17.000000000 +1300
@@ -1863,8 +1863,7 @@
         LDAPControl *ctrl = malloc(sizeof(LDAPControl));

         ctrl->ldctl_oid          = ber_strdup(oid);
-        ctrl->ldctl_value.bv_val = ber_strdup(bv_val);
-        ctrl->ldctl_value.bv_len = bv_len;
+       ber_str2bv(bv_val, bv_len, 1, &ctrl->ldctl_value);
         ctrl->ldctl_iscritical   = iscritical;

         ctrlp = ctrl;

I get this output:

===== Server Side Sort and Virtual List View
 + Loading ASN for SortKeyList, SortResult, VirtualListViewRequest, VirtualListViewResponse
 + Constructing Server Side Sort control
sss ctrl berval =
[0000]   30 13 30 11  04 02 73 6E  80 08 32 2E  35 2E 31 33   0.0. ..sn ..2. 5.13
[0010]   2E 33 81 01  FF                                      .3.. .
 + Constructing Virtual List View control
vlv ctrl berval =
[0000]   30 0E 02 01  00 02 01 03  A0 06 02 01  01 02 01 00   0... .... .... ....
 + Searching for results
 + Parsing first entry
result = $VAR1 = {
          'matcheddn' => undef,
          'referrals' => [],
          'errmsg' => undef,
          'serverctrls' => [
                             32738608,
                             27748368
                           ],
          'errcode' => 0
        };
 + Reading result-scope server response controls
 + Response control 32738608 [1.2.840.113556.1.4.474]
berval =
[0000]   30 03 0A 01  00                                      0... .
 + Parsing SSS response berval
$VAR1 = {
          'sortResult' => 0
        };
 + Response control 27748368 [2.16.840.1.113730.3.4.10]
berval =
[0000]   30 14 02 01  01 02 02 7E  B0 0A 01 00  04 08 50 0E   0... ...~ .... ..P.
[0010]   97 02 00 00  00 00                                   .... ..
 + Parsing VLV response berval
$VAR1 = {
          'contextID' => 'P',
          'contentCount' => 32432,
          'virtualListViewResult' => 0,
          'targetPosition' => 1
        };
 + Search results
dn: cn=Test User,ou=Users,o=Test Data,c=nz
dn: cn=Ani Odey,ou=Users,o=Test Data,c=nz
dn: cn=Vevelyn Odette,ou=Users,o=Test Data,c=nz
dn: cn=Donyetta Odett,ou=Users,o=Test Data,c=nz

To sum up - does it make sense to replace the ber_strdup() with ber_str2bv() in the XS wrapper ldap_create_control() ?

As I say, I don't think this is related to the syncrepl issue - but only because I don't see how a '\x00' character could be inserted into the BER of 'mode' on the syncRequestValue....

Perhaps there is another way we should be reading the user-provided BER? Otherwise, I'm happy to prepare a pull request for this, targeting a 3.0.5 or 3.1.x release

Cheers,
Phillip

[phillipod]

As an aside, should 'ber_strdup_x' in liblber really be using strlen()? :)

[hyc]

Yes, ber_strdup is meant for C strings and C strings are NUL-terminated, by definition. But obviously you should not be using it on values that are not C strings. Likewise you cannot use ber_str2bv, for the same reason.

You should be using ber_dupbv, which is meant for bervals.

[quanah]

@phillipod Want to hold 3.0.4 for this to be fixed?

[phillipod]

@hyc okay, makes sense about ber_strdup and ber_str2bv. But ber_dupbv seems to indicate that it is duplicating an existing struct berval to another struct berval - which seems kind of redundant when the code is trying to build a struct berval from just a chunk of memory?

What about ber_mem2bv? It seems to have the right semantics of not ever using strlen unlike ber_strdup and ber_str2bv, and it doesn't require the source to be a struct berval. Keep in mind that the underlying perl representation of the input should be a SVpv with a defined length instead of a NULL-terminated string.

[phillipod]

@quanah actually, yeah, might as well.

[hyc]

phillipod wrote:

@hyc https://github.com/hyc okay, makes sense about /ber_strdup/ and
/ber_str2bv/. But /ber_dupbv/ seems to indicate that it is duplicating an
existing /struct berval/ to another /struct berval/ - which seems kind of
redundant when the code is trying to build a /struct berval/ from just a chunk
of memory?

What about /ber_mem2bv/? It seems to have the right semantics of not ever
using /strlen/ unlike /ber_strdup/ and /ber_str2bv/, and it doesn't require
the source to be a /struct berval/. Keep in mind that the underlying perl
representation of the input should be a SVpv with a defined length instead of
a NULL-terminated string.

Yeah, mem2bv should do fine.

-- Howard Chu
CTO, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc/
Chief Architect, OpenLDAP http://www.openldap.org/project/