Issue 7995 - of-by-one error in schema
Summary: of-by-one error in schema
Status: VERIFIED FIXED
Alias: None
Product: OpenLDAP
Classification: Unclassified
Component: slapd (show other issues)
Version: 2.4.40
Hardware: All All
: --- normal
Target Milestone: ---
Assignee: OpenLDAP project
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-09 14:27 UTC by Leonid Yuriev
Modified: 2015-07-02 17:45 UTC (History)
0 users

See Also:


Attachments
its7995.patch (2.23 KB, patch)
2014-12-09 21:01 UTC, Leonid Yuriev
Details

Note You need to log in before you can comment on or make changes to this issue.
Description Leonid Yuriev 2014-12-09 14:27:46 UTC
Full_Name: Leonid Yuriev
Version: 2.4.40
OS: RHEL7
URL: ftp://ftp.openldap.org/incoming/
Submission from: (NULL) (31.130.36.33)


In some cases (presumably when a database contains more attributes than defined
in the scheme) a heap error may be detected at stop of slapd.

Below is the result of attempts to find a bug(s) with Valgrind.
It is enough to corrupt a malloc's heap!

==29701== Invalid write of size 1
==29701== at 0x4A089AF: strcpy (vg_replace_strmem.c:458)
==29701== by 0x45ECC6: slap_bv2undef_ad (ad.c:772)
==29701== by 0x4C3649: mdb_ad_read (attr.c:575)
==29701== by 0x4949D7: mdb_db_open (init.c:278)
==29701== by 0x482D86: over_db_open (backover.c:149)
==29701== by 0x42DA58: backend_startup_one (backend.c:224)
==29701== by 0x42DD22: backend_startup (backend.c:325)
==29701== by 0x44ABB0: slap_startup (init.c:219)
==29701== by 0x406C55: main (main.c:988)
==29701== Address 0x57d9187 is 0 bytes after a block of size 71 alloc'd
==29701== at 0x4A0720A: malloc (vg_replace_malloc.c:296)
==29701== by 0x549A18: ber_memalloc_x (memory.c:228)
==29701== by 0x43901A: ch_malloc (ch_malloc.c:54)
=979701== by 0x45EC93: slap_bv2undef_ad (ad.c:764)
==29701== by 0x4C3649: mdb_ad_read (attr.c:575)
==29701== by 0x4949D7: mdb_db_open (init.c:278)
==29701== by 0x482D86: over_db_open (backover.c:149)
==29701== by 0x42DA58: backend_startup_one (backend.c:224)
==29701== by 0x42DD22: backend_startup (backend.c:325)
==29701== by 0x44ABB0: slap_startup (init.c:219)
==29701== by 0x406C55: main (main.c:988)
==29701==
==29701== Invalid read of size 1
==29701== at 0x53BE9F: ldap_pvt_str2upper (string.c:116)
==29701== by 0x45ECCF: slap_bv2undef_ad (ad.c:775)
==29701== by 0x4C3649: mdb_ad_read (attr.c:575)
==29701== by 0x4949D7: mdb_db_open (init.c:278)
==29701== by 0x482D86: over_db_open (backover.c:149)
==29701== by 0x42DA58: backend_startup_one (backend.c:224)
==291%1== by 0x42DD22: backend_startup (backend.c:325)
==29701== by 0x44ABB0: slap_startup (init.c:219)
==29701== by 0x406C55: main (main.c:988)
==29701== Address 0x57d9187 is 0 bytes after a block of size 71 alloc'd
==29701== at 0x4A0720A: malloc (vg_replace_malloc.c:296)
==29701== by 0x549A18: ber_memalloc_x (memory.c:228)
==29701== by 0x43901A: ch_malloc (ch_malloc.c:54)
==29701== by 0x45EC93: slap_bv2undef_ad (ad.c:764)
==29701== by 0x4C3649: mdb_ad_read (attr.c:575)
==29701== by 0x4949D7: mdb_db_open (init.c:278)
==29701== by 0x482D86: over_db_open (backover.c:149)
==29701== by 0x42DA58: backend_startup_one (backend.c:224)
==29701== by 0x42DD22: backend_startup (backend.c:325)
==29701== by 0x44ABB0: slap_startup (init.c:219)
==29703D%3= by 0x406C55: main (main.c:988)

==29701== Invalid read of size 1
==29701== at 0x30E184812C: vfprintf (in /lib64/libc-2.12.so)
==29701== by 0x30E186FA51: vsnprintf (in /lib64/libc-2.12.so)
==29701== by 0x5498DA: lutil_debug (debug.c:67)
==29701== by 0x45ED34: slap_bv2undef_a(a8ad.c:785)
==29701== by 0x4C3649: mdb_ad_read (attr.c:575)
==29701== by 0x4949D7: mdb_db_open (init.c:278)
==29701== by 0x482D86: over_db_open (backover.c:149)
==29701== by 0x42DA58: backend_startup_one (backend.c:224)
==29701== by 0x42DD22: backend_startup (backend.c:325)
==29701== by 0x44ABB0: slap_startup (init.c:219)
==29701== by 0x406C55: main (main.c:988)
==29701== Address 0x57d9187 is 0 bytes after a block of size 71 alloc'd
==29701== at 0x4A0720A: malloc (vg_replace_malloc.c:296)
==29701== by 0x549A18: ber_memalloc_x (memory.c:228)
==29701== by 0x43901A: ch_malloc (ch_malloc.c:54)
==29701== by 0x45EC93: slap_bv2undef_ad (ad.c:764)
==29701== by 0x4C3649: mdb_ad_read (attr.c:575)
==29701== by 0x4949D7: mdb_db_open (init.c:278)
==29701== by 0x482D86: over_db_open (backover.c:149)
==29701== by 0x42DA58: backend_startup_one (backend.c:224)
==29701== by 0x42DD22: backend_startup (backend.c:325)
==29701== by 0x44ABB0: slap_startup (init.c:219)
==29701== by 0x406C55: main (main.c:988)

Comment 1 Howard Chu 2014-12-09 15:57:38 UTC
leo@yuriev.ru wrote:
> Full_Name: Leonid Yuriev
> Version: 2.4.40
> OS: RHEL7
> URL: ftp://ftp.openldap.org/incoming/
> Submission from: (NULL) (31.130.36.33)
>
>
> In some cases (presumably when a database contains more attributes than defined
> in the scheme) a heap error may be detected at stop of slapd.
>
> Below is the result of attempts to find a bug(s) with Valgrind.
> It is enough to corrupt a malloc's heap!

Please provide a test case. Unable to reproduce this on a local database 
with commented out schema. I get:

violino:~/OD/o24/tests> valgrind ../servers/slapd/slapd -Tc -f 
/tmp/testr/slapd.1.conf > /tmp/out
==25499== Memcheck, a memory error detector
==25499== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==25499== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for 
copyright info
==25499== Command: ../servers/slapd/slapd -Tc -f /tmp/testr/slapd.1.conf
==25499==
54871b4f UNKNOWN attributeDescription "HOMEPOSTALADDRESS" inserted.
54871b4f UNKNOWN attributeDescription "DRINK" inserted.
54871b4f UNKNOWN attributeDescription "HOMEPHONE" inserted.
54871b4f UNKNOWN attributeDescription "PAGER" inserted.
==25499==
==25499== HEAP SUMMARY:
==25499==     in use at exit: 2,746 bytes in 78 blocks
==25499==   total heap usage: 10,335 allocs, 10,257 frees, 1,649,999 
bytes allocated
==25499==
==25499== LEAK SUMMARY:
==25499==    definitely lost: 0 bytes in 0 blocks
==25499==    indirectly lost: 0 bytes in 0 blocks
==25499==      possibly lost: 0 bytes in 0 blocks
==25499==    still reachable: 2,746 bytes in 78 blocks
==25499==         suppressed: 0 bytes in 0 blocks
==25499== Rerun with --leak-check=full to see details of leaked memory
==25499==
==25499== For counts of detected and suppressed errors, rerun with: -v
==25499== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)



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

Comment 2 Leonid Yuriev 2014-12-09 21:01:29 UTC
2014-12-09 18:57 GMT+03:00 Howard Chu <hyc@symas.com>:
> leo@yuriev.ru wrote:
>>
>> Full_Name: Leonid Yuriev
>> Version: 2.4.40
>> OS: RHEL7
>> URL: ftp://ftp.openldap.org/incoming/
>> Submission from: (NULL) (31.130.36.33)
>>
>>
>> In some cases (presumably when a database contains more attributes than
>> defined
>> in the scheme) a heap error may be detected at stop of slapd.
>>
>> Below is the result of attempts to find a bug(s) with Valgrind.
>> It is enough to corrupt a malloc's heap!
>
>
> Please provide a test case. Unable to reproduce this on a local database
> with commented out schema. I get:
>
> violino:~/OD/o24/tests> valgrind ../servers/slapd/slapd -Tc -f
> /tmp/testr/slapd.1.conf > /tmp/out
> ==25499== Memcheck, a memory error detector
> ==25499== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
> ==25499== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright
> info
> ==25499== Command: ../servers/slapd/slapd -Tc -f /tmp/testr/slapd.1.conf
> ==25499==
> 54871b4f UNKNOWN attributeDescription "HOMEPOSTALADDRESS" inserted.
> 54871b4f UNKNOWN attributeDescription "DRINK" inserted.
> 54871b4f UNKNOWN attributeDescription "HOMEPHONE" inserted.
> 54871b4f UNKNOWN attributeDescription "PAGER" inserted.
> ==25499==
> ==25499== HEAP SUMMARY:
> ==25499==     in use at exit: 2,746 bytes in 78 blocks
> ==25499==   total heap usage: 10,335 allocs, 10,257 frees, 1,649,999 bytes
> allocated
> ==25499==
> ==25499== LEAK SUMMARY:
> ==25499==    definitely lost: 0 bytes in 0 blocks
> ==25499==    indirectly lost: 0 bytes in 0 blocks
> ==25499==      possibly lost: 0 bytes in 0 blocks
> ==25499==    still reachable: 2,746 bytes in 78 blocks
> ==25499==         suppressed: 0 bytes in 0 blocks
> ==25499== Rerun with --leak-check=full to see details of leaked memory
> ==25499==
> ==25499== For counts of detected and suppressed errors, rerun with: -v
> ==25499== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
>
> --
>   -- Howard Chu
>   CTO, Symas Corp.           http://www.symas.com
>   Director, Highland Sun     http://highlandsun.com/hyc/
>   Chief Architect, OpenLDAP  http://www.openldap.org/project/

(1)
Unfortunately it was reproduced only in production-like environment,
therefore a testcase or complete info is not available yet.

(2)
But let see to lines 570-575 of mdb-back/attr.c and lines 764-772 of slapd/ad.c:
- does the mdb-value include a NUL byte =
https://github.com/leo-yuriev/openldap-lmdb-challenge/blob/2.4-devel/servers/slapd/back-mdb/attr.c#L570?
- if "YES" then "+1" at slapd/ad.c:764 is wrong =
https://github.com/leo-yuriev/openldap-lmdb-challenge/blob/2.4-devel/servers/slapd/ad.c#L764
- but if "NO" then "strcpy()" at slapd/ad.c:772 (and more) is wrong =
https://github.com/leo-yuriev/openldap-lmdb-challenge/blob/2.4-devel/servers/slapd/ad.c#L772

(3)
Next, I had added a couple of assert-check into mdb-back/attr.c
and right away got "slapcat: attr.c:573: mdb_ad_read: Assertion
`bdata.bv_val[bdata.bv_len-1] == 0' failed"

diff --git a/servers/slapd/back-mdb/attr.c b/servers/slapd/back-mdb/attr.c
index 07b64a6..7979b9d 100644
--- a/servers/slapd/back-mdb/attr.c
+++ b/servers/slapd/back-mdb/attr.c
@@ -569,6 +569,8 @@ int mdb_ad_read( struct mdb_info *mdb, MDB_txn *txn )
        while ( rc == MDB_SUCCESS ) {
                bdata.bv_len = data.mv_size;
                bdata.bv_val = data.mv_data;
+               assert(bdata.bv_len > 0);
+               assert(bdata.bv_val[bdata.bv_len-1] == 0);
                ad = NULL;
                rc = slap_bv2ad( &bdata, &ad, &text );
                if ( rc ) {

(4)
Ok, this is a off-by-one bug.
Then I looked for "sizeof(AttributeDescription)" and has found only 3
places in slapd/ad.c
... and fix a few places of str-calls to n-form.

Patch is attached, please review it.

Leonid.

---

The attached files is derived from OpenLDAP Software. All of the modifications
to OpenLDAP Software represented in the following patch(es) were developed by
Peter-Service LLC, Moscow, Russia. Peter-Service LLC has not assigned rights
and/or interest in this work to any party. I, Leonid Yuriev am authorized by
Peter-Service LLC, my employer, to release this work under the following terms.

Peter-Service LLC hereby places the following modifications to OpenLDAP Software
(and only these modifications) into the public domain. Hence, these
modifications may be freely used and/or redistributed for any purpose
with or without attribution and/or other notice.
Comment 3 Howard Chu 2014-12-10 02:05:06 UTC
Леонид Юрьев wrote:
> (2)
> But let see to lines 570-575 of mdb-back/attr.c and lines 764-772 of slapd/ad.c:
> - does the mdb-value include a NUL byte =
> https://github.com/leo-yuriev/openldap-lmdb-challenge/blob/2.4-devel/servers/slapd/back-mdb/attr.c#L570?
> - if "YES" then "+1" at slapd/ad.c:764 is wrong =
> https://github.com/leo-yuriev/openldap-lmdb-challenge/blob/2.4-devel/servers/slapd/ad.c#L764
> - but if "NO" then "strcpy()" at slapd/ad.c:772 (and more) is wrong =
> https://github.com/leo-yuriev/openldap-lmdb-challenge/blob/2.4-devel/servers/slapd/ad.c#L772

And the answer is no, back-mdb/attr.c mdb_ad_get() doesn't store the 
trailing NUL byte,

> diff --git a/servers/slapd/ad.c b/servers/slapd/ad.c
> index 246b900..fd60483 100644
> --- a/servers/slapd/ad.c
> +++ b/servers/slapd/ad.c
> @@ -118,7 +118,7 @@ AttributeDescription * ad_find_tags(
>  	for (ad = type->sat_ad; ad; ad=ad->ad_next)
>  	{
>  		if (ad->ad_tags.bv_len == tags->bv_len &&
> -			!strcasecmp(ad->ad_tags.bv_val, tags->bv_val))
> +			!strncasecmp( ad->ad_tags.bv_val, tags->bv_val, ad->ad_tags.bv_len ))
>  			break;
>  	}

Unnecessary.

>  	ldap_pvt_thread_mutex_unlock( &type->sat_ad_mutex );
> @@ -742,14 +742,13 @@ int slap_bv2undef_ad(
>  	/* use the appropriate type */
>  	if ( flags & SLAP_AD_PROXIED ) {
>  		at = slap_schema.si_at_proxied;
> -
>  	} else {
>  		at = slap_schema.si_at_undefined;
>  	}
>
>  	for( desc = at->sat_ad; desc; desc=desc->ad_next ) {
>  		if( desc->ad_cname.bv_len == bv->bv_len &&
> -		    !strcasecmp( desc->ad_cname.bv_val, bv->bv_val ) )
> +		    !strncasecmp( desc->ad_cname.bv_val, bv->bv_val, desc->ad_cname.bv_len) )

Unnecessary. We've already checked that the two lengths are equal; even 
if bv->bv_val is non-terminated the compare will stop because 
desc->ad_cname is terminated.

>  		{
>  		    	break;
>  		}
> @@ -769,7 +768,8 @@ int slap_bv2undef_ad(
>
>  		desc->ad_cname.bv_len = bv->bv_len;
>  		desc->ad_cname.bv_val = (char *)(desc+1);
> -		strcpy(desc->ad_cname.bv_val, bv->bv_val);
> +		strncpy( desc->ad_cname.bv_val, bv->bv_val, desc->ad_cname.bv_len )
> +			[desc->ad_cname.bv_len] = '\0';

This is a valid fix.
>
>  		/* canonical to upper case */
>  		ldap_pvt_str2upper( desc->ad_cname.bv_val );
> @@ -806,9 +806,10 @@ slap_bv2tmp_ad(
>  		 slap_sl_mfuncs.bmf_malloc( sizeof(AttributeDescription) +
>  			bv->bv_len + 1, memctx );
>
> -	ad->ad_cname.bv_val = (char *)(ad+1);
> -	strncpy( ad->ad_cname.bv_val, bv->bv_val, bv->bv_len+1 );
>  	ad->ad_cname.bv_len = bv->bv_len;
> +	ad->ad_cname.bv_val = (char *)(ad+1);
> +	strncpy( ad->ad_cname.bv_val, bv->bv_val, ad->ad_cname.bv_len)
> +		[ad->ad_cname.bv_len] = '\0';
>  	ad->ad_flags = SLAP_DESC_TEMPORARY;
>  	ad->ad_type = slap_schema.si_at_undefined;
>
Unnecessary.

> @@ -887,7 +888,7 @@ an_find(
>
>  	for ( ; a->an_name.bv_val; a++ ) {
>  		if ( a->an_name.bv_len != s->bv_len) continue;
> -		if ( strcasecmp( s->bv_val, a->an_name.bv_val ) == 0 ) {
> +		if ( strncasecmp( s->bv_val, a->an_name.bv_val, s->bv_len ) == 0 ) {
>  			return( 1 );
>  		}
>  	}

Unnecessary.

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

Comment 4 Howard Chu 2014-12-10 23:01:54 UTC
changed notes
changed state Open to Test
moved from Incoming to Software Bugs
Comment 5 Quanah Gibson-Mount 2014-12-11 00:39:55 UTC
changed notes
changed state Test to Release
Comment 6 OpenLDAP project 2015-07-02 17:45:53 UTC
fixed in master
fixed in RE25
fixed in RE24
Comment 7 Quanah Gibson-Mount 2015-07-02 17:45:53 UTC
changed notes
changed state Release to Closed