Thread: Fix for bug in ldapServiceLookup in libpq

I have found a small but annoying bug in libpq where
connection parameters are resolved via LDAP.

There is a write past the end of a malloc'ed string which causes
memory corruption. The code and the bug are originally by me :^(

The attached patch fixes the problem in HEAD.
This should be backpatched to 8.2 where the code was introduced.

Yours,
Laurenz Albe

"Albe Laurenz" <laurenz.albe@wien.gv.at> writes:
> I have found a small but annoying bug in libpq where
> connection parameters are resolved via LDAP.

> There is a write past the end of a malloc'ed string which causes
> memory corruption. The code and the bug are originally by me :^(

Hmm ... that's a bug all right, but why have the null-termination
inside the loop at all?  Seems like it should look like
for (p = result, i = 0; values[i] != NULL; ++i){    strncpy(p, values[i]->bv_val, values[i]->bv_len);    p +=
values[i]->bv_len;   *(p++) = '\n';}*p = '\0';
 

> This should be backpatched to 8.2 where the code was introduced.

Yes, will do.
        regards, tom lane

... btw, shouldn't this function free the "result" string when it's done
with it?  AFAICS that string is not returned to the caller, it's just
being leaked.

(I'll refrain from asking why it's creating the string in the first
place rather than parsing ldap_get_values_len's output as-is ...)
        regards, tom lane

Tom Lane wrote:
>> I have found a small but annoying bug in libpq where
>> connection parameters are resolved via LDAP.

> Hmm ... that's a bug all right, but why have the null-termination
> inside the loop at all?  Seems like it should look like
>
>    for (p = result, i = 0; values[i] != NULL; ++i)
>    {
>        strncpy(p, values[i]->bv_val, values[i]->bv_len);
>        p += values[i]->bv_len;
>        *(p++) = '\n';
>    }
>    *p = '\0';

Yes, that's better.

> ... btw, shouldn't this function free the "result" string when it's
done
> with it?  AFAICS that string is not returned to the caller, it's just
> being leaked.

Oops, yes it should.

> (I'll refrain from asking why it's creating the string in the first
> place rather than parsing ldap_get_values_len's output as-is ...)

So that I can close the LDAP connection as soon as feasible, but of
course
that's not absolutely necessary.

I have attached a new version of the patch that should address all known
problems.

Yours,
Laurenz Albe

I wrote:
>>> I have found a small but annoying bug in libpq where
>>> connection parameters are resolved via LDAP.
>
> I have attached a new version of the patch that should address all
known
> problems.

FWIW, I ran valgrind on psql establishing an SSL connection, and I found
some messages like this:

==26437== Conditional jump or move depends on uninitialised value(s)
==26437==    at 0x423DDC8: BN_mod_inverse (in /lib/libcrypto.so.0.9.7a)
==26437==    by 0x4241EDC: BN_MONT_CTX_set (in /lib/libcrypto.so.0.9.7a)
==26437==    by 0x4243E28: ??? (in /lib/libcrypto.so.0.9.7a)
==26437==    by 0x424553D: RSA_public_decrypt (in
/lib/libcrypto.so.0.9.7a)
==26437==    by 0x4245F15: RSA_verify (in /lib/libcrypto.so.0.9.7a)
==26437==    by 0x41D1192: ??? (in /lib/libssl.so.0.9.7a)
==26437==    by 0x41CFC2A: ssl3_connect (in /lib/libssl.so.0.9.7a)
==26437==    by 0x41DC939: SSL_connect (in /lib/libssl.so.0.9.7a)
==26437==    by 0x403DF47: open_client_SSL (fe-secure.c:1161)
==26437==    by 0x403C903: pqsecure_open_client (fe-secure.c:284)
==26437==    by 0x402908F: PQconnectPoll (fe-connect.c:2113)
==26437==    by 0x4028301: connectDBComplete (fe-connect.c:1463)

and

==26437== Use of uninitialised value of size 4
==26437==    at 0x42387A5: BN_num_bits_word (in
/lib/libcrypto.so.0.9.7a)
==26437==    by 0x4238833: BN_num_bits (in /lib/libcrypto.so.0.9.7a)
==26437==    by 0x423788A: BN_mod_exp_mont_consttime (in
/lib/libcrypto.so.0.9.7a)
==26437==    by 0x4237657: BN_mod_exp_mont (in /lib/libcrypto.so.0.9.7a)
==26437==    by 0x424A51D: ??? (in /lib/libcrypto.so.0.9.7a)
==26437==    by 0x424A1DB: ??? (in /lib/libcrypto.so.0.9.7a)
==26437==    by 0x424A061: DH_generate_key (in /lib/libcrypto.so.0.9.7a)
==26437==    by 0x41D1EF9: ??? (in /lib/libssl.so.0.9.7a)
==26437==    by 0x41CFD1E: ssl3_connect (in /lib/libssl.so.0.9.7a)
==26437==    by 0x41DC939: SSL_connect (in /lib/libssl.so.0.9.7a)
==26437==    by 0x403DF47: open_client_SSL (fe-secure.c:1161)
==26437==    by 0x403C903: pqsecure_open_client (fe-secure.c:284)

All in the same call of SSL_connect in open_client_SSL.

I've never worked with SSL, but it looks to me like something in the SSL
data structure is uninitialized. Don't know if that's a serious problem
or not.

Yours,
Laurenz Albe

"Albe Laurenz" <laurenz.albe@wien.gv.at> writes:
> FWIW, I ran valgrind on psql establishing an SSL connection, and I found
> some messages like this:

> ==26437== Conditional jump or move depends on uninitialised value(s)
> ==26437== Use of uninitialised value of size 4

Yeah, this has been mentioned before IIRC.  It looks to me like the
issue is entirely within the SSL library and has nothing to do with
Postgres.  Dunno if it's worth filing a bug report with the OpenSSL
folk ...
        regards, tom lane

"Albe Laurenz" <laurenz.albe@wien.gv.at> writes:
> I have attached a new version of the patch that should address all known
> problems.

You missed one "return" where the string needed to be freed.  I've
applied this patch with that fix and a couple of cosmetic changes.
Thanks for the report and patch!
        regards, tom lane

Tom Lane wrote:

> You missed one "return" where the string needed to be freed.  I've
> applied this patch with that fix and a couple of cosmetic changes.
> Thanks for the report and patch!

Thanks for the work and the keen eye!

Yours,
Laurenz Albe