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
Attachments:
ldapServiceLookup.patchapplication/octet-stream; name=ldapServiceLookup.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 1b409d1..ae13944
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** ldapServiceLookup(const char *purl, PQco
*** 3613,3619 ****
p += values[i]->bv_len;
*(p++) = '\n';
if (values[i + 1] == NULL)
! *(p + 1) = '\0';
}
ldap_value_free_len(values);
--- 3613,3619 ----
p += values[i]->bv_len;
*(p++) = '\n';
if (values[i + 1] == NULL)
! *p = '\0';
}
ldap_value_free_len(values);
"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 likefor (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
Attachments:
ldapServiceLookup.patchapplication/octet-stream; name=ldapServiceLookup.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 1b409d1..e4b152a
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** ldapServiceLookup(const char *purl, PQco
*** 3612,3620 ****
strncpy(p, values[i]->bv_val, values[i]->bv_len);
p += values[i]->bv_len;
*(p++) = '\n';
- if (values[i + 1] == NULL)
- *(p + 1) = '\0';
}
ldap_value_free_len(values);
ldap_unbind(ld);
--- 3612,3619 ----
strncpy(p, values[i]->bv_val, values[i]->bv_len);
p += values[i]->bv_len;
*(p++) = '\n';
}
+ *p = '\0';
ldap_value_free_len(values);
ldap_unbind(ld);
*************** ldapServiceLookup(const char *purl, PQco
*** 3643,3648 ****
--- 3642,3648 ----
printfPQExpBuffer(errorMessage, libpq_gettext(
"missing \"=\" after \"%s\" in connection info string\n"),
optname);
+ free(result);
return 3;
}
else if (*p == '=')
*************** ldapServiceLookup(const char *purl, PQco
*** 3661,3666 ****
--- 3661,3667 ----
printfPQExpBuffer(errorMessage, libpq_gettext(
"missing \"=\" after \"%s\" in connection info string\n"),
optname);
+ free(result);
return 3;
}
break;
*************** ldapServiceLookup(const char *purl, PQco
*** 3731,3736 ****
--- 3732,3738 ----
}
oldstate = state;
}
+ free(result);
if (state == 5 || state == 6)
{
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