Обсуждение: Patch for 1-byte buffer overflow in libpq PQencryptPassword

Поиск
Список
Период
Сортировка

Patch for 1-byte buffer overflow in libpq PQencryptPassword

От
ljb
Дата:
A trivial little fix for PostgreSQL-8.4.1.

Calling the libpq function PQencryptPassword(password, "") doesn't make
a lot of sense (empty string for username). But if you do, it results
in a 1-byte buffer overflow in pg_md5_encrypt().  (This is in
backend/libpq/md5.c, but it's client, not backend.)

This is because pg_md5_encrypt(password, salt, salt_len, buf) with
salt_len=0 allocates a buffer crypt_buf of size strlen(password), then
uses strcpy to copy the password in there. The null byte at the end of
the password overruns the end of the allocated buffer.

(Found during pgtclng testing, looking for the cause of an error
on WinXP only, which turned out to have nothing to do with this.)

Two possible suggested fixes to src/backend/libpq/md5.c, pg_md5_crypt():
1) Allocate crypt_buf to (passwd_len + 1 + salt_len)
2) Use memcpy(crypt_buf, passwd, passwd_len) not strcpy(crypt_buf, passwd).

I like fix #2 better, although fix #1 avoids a weirdness with
PQencryptPassword("","") calling malloc(0) with platform-dependent
results (which was the problem I was chasing with pgtclng).

Patch below is for fix #2.

--- postgresql-8.4.1/src/backend/libpq/md5.c.bak    2009-01-01 12:23:42.000000000 -0500
+++ postgresql-8.4.1/src/backend/libpq/md5.c    2009-09-13 11:21:59.000000000 -0400
@@ -324,7 +324,7 @@     * Place salt at the end because it may be known by users trying to crack     * the MD5 output.
  */
 
-    strcpy(crypt_buf, passwd);
+    memcpy(crypt_buf, passwd, passwd_len);    memcpy(crypt_buf + passwd_len, salt, salt_len);    strcpy(buf, "md5");


Re: Patch for 1-byte buffer overflow in libpq PQencryptPassword

От
Tom Lane
Дата:
ljb <ljb1813@pobox.com> writes:
> Two possible suggested fixes to src/backend/libpq/md5.c, pg_md5_crypt():
> 1) Allocate crypt_buf to (passwd_len + 1 + salt_len)
> 2) Use memcpy(crypt_buf, passwd, passwd_len) not strcpy(crypt_buf, passwd).

> I like fix #2 better, although fix #1 avoids a weirdness with
> PQencryptPassword("","") calling malloc(0) with platform-dependent
> results (which was the problem I was chasing with pgtclng).

Hmm ... I'm inclined to do both.  I agree that the memcpy coding is
cleaner than strcpy when we don't actually care about adding a trailing
null.  But malloc(0) is unportable and best avoided.
        regards, tom lane