324525 - Memory error in ber_get_stringal/ber_bvfree

Status: RESOLVED FIXED

(Directory Graveyard :: LDAP C SDK, defect)

Description

[Rich Megginson]

If the malloc is done to allocate the *bv, but then an lber error is encountered while doing the ber_skip_tag (due to a bad or malicious client), the bv->bv_val is set to a random value.  If you then try to use ber_bvfree to free the bv, it may call free on this uninitialized value, and badness ensues.  I think the proper fix is for ber_get_stringal to set (*bv)->bv_val to NULL and (*bv)->bv_len to 0 just after the malloc.

Comment 1

[Mark Smith [:mcs]]

I agree this is a bug and that your proposed fix sounds OK.  Another approach would be to use calloc() instead or free the berval before returning an error.  Which do you think is best?

Comment 2

[Rich Megginson]

I figured you used malloc instead of calloc for performance reasons.  Malloc + NULL assignment should be faster than calloc and it's just as safe in this case.

Comment 3

[Rich Megginson]

Attachment: decode.c

Comment 4

[Rich Megginson]

Attachment: diffs for fix

These are also the diffs for bug 324529

Comment 5

[Rich Megginson]

Attachment: diffs for fix

Incorporates comments from Mark.

Comment 6

[Rich Megginson]

Attachment: diffs for fix

I needed to use ber_svecfree to free the char** and ber_bvecfree to free the
berval**, not NSLBERI_FREE.  The arrays may have individual elements that have
been allocated, and those must be freed as well.  See later in the code, the
switch statement that tries to clean up upon error.

Comment 7

[Mark Smith [:mcs]]

Comment on attachment 210533 [details]
diffs for fix

The patch looks good.  Thanks.

Comment 8

[Rich Megginson]

Checking in decode.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/decode.c,v  <--  decode.c
new revision: 5.3; previous revision: 5.2
done