Full_Name: Ryan Tandy Version: master, commit 141f1680 OS: Ubuntu 14.04 URL: ftp://ftp.openldap.org/incoming/rtandy_20140511_fix-passwd-b64-buffer.patch Submission from: (NULL) (24.68.121.206) The password checkers call lutil_b64_pton like this: rc = lutil_b64_pton(passwd->bv_val, orig_pass, passwd->bv_len); However, the size of orig_pass is actually only 3/4 of passwd->bv_len. Normally this isn't a problem; however, if the length of the password hash is not a multiple of 4, then LUTIL_BASE64_DECODE_LEN rounds down, orig_pass gets a shorter buffer allocated than is actually needed, and it's possible for lutil_b64_pton to write past its end. (How can that happen? An admin could mess up copy-pasting from slappasswd to userPassword/olcRootPW, as could a user with write access to their own userPassword who decided to set it manually.) Example: $ slappasswd -s secret -h '{SSHA}' {SSHA}86ks4vJqfruLKer9skuEpTmgPxcaqSLe If we lose the last character ('e') and try to authenticate against the length 31 string, clang's AddressSanitizer notices: ==15335==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000049736 at pc 0xef0720 bp 0x7fa9b38d1c10 sp 0x7fa9b38d1c08 WRITE of size 1 at 0x603000049736 thread T2 #0 0xef071f in lutil_b64_pton (libraries/liblutil/base64.c:225) #1 0xe9547c in chk_ssha1 (libraries/liblutil/passwd.c:508) #2 0xe90a09 in lutil_passwd (libraries/liblutil/passwd.c:327) #3 0x7d5e1c in slap_passwd_check (servers/slapd/passwd.c:529) [snip ...] 0x603000049736 is located 0 bytes to the right of 22-byte region [0x603000049720,0x603000049736) allocated by thread T2 here: #0 0x43c9a4 in malloc (/opt/openldap/libexec/slapd+0x43c9a4) #1 0x112903f in ber_memalloc_x (libraries/liblber/memory.c:228) #2 0x1129396 in ber_memalloc (libraries/liblber/memory.c:244) #3 0xe952ea in chk_ssha1 (libraries/liblutil/passwd.c:503) #4 0xe90a09 in lutil_passwd (libraries/liblutil/passwd.c:327) #5 0x7d5e1c in slap_passwd_check (servers/slapd/passwd.c:529) [snip...] (Built with ./configure CC=clang CFLAGS="-O0 -g -fsanitize=address") Patch in URL. (Apparently the apr1 passwd module has CRLF endings; sorry for the noisy diff.) BTW, I noticed in the same routines that orig_pass is allocated with + 1 to the required length, as if it were to be null-terminated, but that's never done. Is that intentional? (AFAICS ber_memalloc doesn't zero memory, maybe I'm wrong.) thanks, Ryan
Sorry, I missed a line from the patch. Corrected one: ftp://ftp.openldap.org/incoming/rtandy_20140511_fix-passwd-b64-buffer_v2.patch
I checked the new pw-pbkdf2 module. It doesn't appear to be affected by this problem. On 11/05/14 07:56 PM, Ryan Tandy wrote: > ftp://ftp.openldap.org/incoming/rtandy_20140511_fix-passwd-b64-buffer_v2.patch You probably know this, but just in case it helps: "git am --keep-cr" is the way to apply that patch, because of apr1.c's line endings. There's a second bug in slapd-sha2.c, a missing cast causing the return value of lutil_b64_pton to be ignored. The built-in checkers already have the appropriate cast. Patch attached.
changed notes changed state Open to Test moved from Incoming to Contrib
ryan@nardis.ca wrote: > This is a multi-part message in MIME format. > --------------070104060109070008020807 > Content-Type: text/plain; charset=UTF-8; format=flowed > Content-Transfer-Encoding: 7bit > > I checked the new pw-pbkdf2 module. It doesn't appear to be affected by > this problem. Thanks, all committed to master. > > On 11/05/14 07:56 PM, Ryan Tandy wrote: >> ftp://ftp.openldap.org/incoming/rtandy_20140511_fix-passwd-b64-buffer_v2.patch > > You probably know this, but just in case it helps: "git am --keep-cr" is > the way to apply that patch, because of apr1.c's line endings. > > There's a second bug in slapd-sha2.c, a missing cast causing the return > value of lutil_b64_pton to be ignored. The built-in checkers already > have the appropriate cast. Patch attached. > > --------------070104060109070008020807 > Content-Type: text/x-patch; > name="0002-ITS-7851-contrib-pw-sha2-fix-int-size_t-comparison.patch" > Content-Transfer-Encoding: 7bit > Content-Disposition: attachment; > filename*0="0002-ITS-7851-contrib-pw-sha2-fix-int-size_t-comparison.patc"; > filename*1="h" > >>From 0683ded766e51e0521991fc1a5d2303cf95cc475 Mon Sep 17 00:00:00 2001 > From: Ryan Tandy <ryan@nardis.ca> > Date: Thu, 26 Jun 2014 18:33:29 -0700 > Subject: [PATCH 2/2] ITS#7851 contrib pw-sha2 fix int/size_t comparison > > --- > contrib/slapd-modules/passwd/sha2/slapd-sha2.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/contrib/slapd-modules/passwd/sha2/slapd-sha2.c b/contrib/slapd-modules/passwd/sha2/slapd-sha2.c > index 1ec7989..2e4fcb0 100644 > --- a/contrib/slapd-modules/passwd/sha2/slapd-sha2.c > +++ b/contrib/slapd-modules/passwd/sha2/slapd-sha2.c > @@ -244,7 +244,7 @@ static int chk_ssha256( > > rc = lutil_b64_pton(passwd->bv_val, orig_pass, decode_len); > > - if( rc <= sizeof(SHAdigest) ) { > + if( rc <= (int)(sizeof(SHAdigest)) ) { > ber_memfree(orig_pass); > return LUTIL_PASSWD_ERR; > } > @@ -332,7 +332,7 @@ static int chk_ssha384( > > rc = lutil_b64_pton(passwd->bv_val, orig_pass, decode_len); > > - if( rc <= sizeof(SHAdigest) ) { > + if( rc <= (int)(sizeof(SHAdigest)) ) { > ber_memfree(orig_pass); > return LUTIL_PASSWD_ERR; > } > @@ -420,7 +420,7 @@ static int chk_ssha512( > > rc = lutil_b64_pton(passwd->bv_val, orig_pass, decode_len); > > - if( rc <= sizeof(SHAdigest) ) { > + if( rc <= (int)(sizeof(SHAdigest)) ) { > ber_memfree(orig_pass); > return LUTIL_PASSWD_ERR; > } > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
changed notes changed state Test to Release
fixed in master fixed in RE25 fixed in RE24
changed notes changed state Release to Closed