heap-buffer-overflow in check_pw_syntax_ext() via ldap_utf8prev()

[xhon-pelushi]

While looking at #7758 I checked the other ldap_utf8prev() call sites. The one in check_pw_syntax_ext() has the same problem, and this one is reached with a user-supplied password.

ldap/servers/slapd/pw.c, the character-class loop:

/* check for repeating characters. If this is the
   first char of the password, no need to check */
if (pwd != p) {
    int len = ldap_utf8len(p);
    char *prev_p = ldap_utf8prev(p);

The pwd != p test stops the call being made on the first character, but it does not stop the walk itself from leaving the buffer. ldap_utf8prev() reads the byte before its argument and then keeps walking back while it sees continuation bytes:

register unsigned char *prev = (unsigned char *)s;
unsigned char *limit = prev - 6;
while (((*--prev & 0xC0) == 0x80) && (prev != limit)) {
    ;
}

So a password whose first byte is a stray continuation byte is enough. ldap_utf8next() treats 0xA0 as one byte, so after the first iteration p is pwd + 1, pwd != p passes, and ldap_utf8prev(pwd + 1) reads pwd[0], sees 0xA0, and keeps going below the start of the value.

Password "\xA0a":

ERROR: AddressSanitizer: heap-buffer-overflow on address 0x50200000002f
READ of size 1 at 0x50200000002f thread T0
    #0 ldap_utf8prev
    #1 check_pw_syntax_ext character-class loop
0x50200000002f is located 1 bytes before 3-byte region [0x502000000030,0x502000000033)

I reproduced that by extracting the loop together with the real UTF8len, ldap_utf8next, ldap_utf8len and ldap_utf8prev bodies, since I cannot build the server here. The is*() helpers were stubbed out because they do not move any pointer.

Severity

Low, as far as I can tell, and I would rather state that plainly than overstate it. It is a read of at most 6 bytes before the value, the bytes only feed ldap_utf8len() and a memcmp() that adjust a repeated-character counter, and nothing is returned to the client. The realistic worst case is a crash if the allocation happens to start a page. Happy to be told it deserves different handling.

Suggested fix

The same bounded walk proposed in #7847 — a helper that takes the start of the value and never reads below it.

I also tried the alternative of remembering the previous position while iterating forward, which removes the backward walk entirely. Over 22620 inputs built from ASCII, blanks, lead bytes and continuation bytes:

variant	differs from current
bounded walk	0
track previous position	405, all of them malformed UTF-8 (0 on well-formed input)

So the bounded walk is the behaviour-preserving option, and that is what I would send unless you prefer the forward-tracking version — it is arguably the more correct reading of "the previous character", but it does change the repeat count for malformed input.