chkpass with RANDOMIZE_ALLOCATED_MEMORY
Hi,
It is been observed on RANDOMIZE_ALLOCATED_MEMORY enabled PG95 build that
chkpass is failing because of uninitialized memory and seems showing false
alarm. I have tried to add code snippets to explain as following i.e.
postgres=# CREATE EXTENSION chkpass;
WARNING: type input function chkpass_in should not be volatile
CREATE EXTENSION
postgres=# CREATE TABLE test_tbl ( name text, pass chkpass );
CREATE TABLE
postgres=# INSERT INTO test_tbl VALUES('foo','foo1');
WARNING: type chkpass has unstable input conversion for "foo1"
LINE 1: INSERT INTO test_tbl VALUES('foo','foo1');
^
INSERT 0 1
It is giving warning "type chkpass has unstable input conversion" because
chkpass structure hold uninitialized memory area’s that are left unused.
When chkpass_in() is called with input “foo1”, it allocate 16 bytes of
randomized memory for chkpass i.e.
contrib/chkpass/chkpass.c
/*
* This is the internal storage format for CHKPASSs.
* 15 is all I need but add a little buffer
*/
typedef struct chkpass
{
char password[16];
} chkpass;
chkpass_in()
result = (chkpass *) palloc(sizeof(chkpass));
*result memory*
0x7ffc93047a68: 0xdd 0xde 0xdf 0xe0 0xe1 0xe2 0xe3 0xe4
0x7ffc93047a70: 0xe5 0xe6 0xe7 0xe8 0xe9 0xea 0xeb 0xec
It copies string (16 bytes max) returned from crypt() function, it copies
until null character reached i.e.
strlcpy(result->password, crypt_output, sizeof(result->password));
*crypt_output memory*
0x7fff7d1577f0: 0x6a 0x6e 0x6d 0x54 0x44 0x53 0x55 0x33
0x7fff7d1577f8: 0x4b 0x48 0x52 0x55 0x6f 0x00 0x00 0x00
*result memory*
0x7ffc93047a68: 0x6a 0x6e 0x6d 0x54 0x44 0x53 0x55 0x33
0x7ffc93047a70: 0x4b 0x48 0x52 0x55 0x6f 0x00 0xeb 0xec
It left remaining last 2 byte left untouched. It is the cause for "unstable
input conversion” warning.
I think these uninitialized memory areas are false alarms. Along with this
there seems another issue that makes chkpass troubling
RANDOMIZE_ALLOCATED_MEMORY comparison logic is that crypt() function is
being passed with random salt value for DES based result. PFA patch, to
generate consistent results, it uses constant salt value.
It seems a minor inconvenience but it will make results better. Please do
let me know if I missed something or more information is required. Thanks.
Regards,
Muhammad Asif Naeem
Attachments:
chkpass_RANDOMIZE_ALLOCATED_MEMORY_v1.patchapplication/octet-stream; name=chkpass_RANDOMIZE_ALLOCATED_MEMORY_v1.patchDownload
diff --git a/contrib/chkpass/chkpass.c b/contrib/chkpass/chkpass.c
index 283ad9a..b361718 100644
--- a/contrib/chkpass/chkpass.c
+++ b/contrib/chkpass/chkpass.c
@@ -75,10 +75,24 @@ chkpass_in(PG_FUNCTION_ARGS)
(errcode(ERRCODE_DATA_EXCEPTION),
errmsg("password \"%s\" is weak", str)));
+#ifndef RANDOMIZE_ALLOCATED_MEMORY
result = (chkpass *) palloc(sizeof(chkpass));
mysalt[0] = salt_chars[random() & 0x3f];
mysalt[1] = salt_chars[random() & 0x3f];
+#else
+ /*
+ * Trailing uninitialized bytes after the null character can lead to
+ * unstable input conversion warnings in RANDOMIZE_ALLOCATED_MEMORY
+ * enable build.
+ */
+ result = (chkpass *) palloc0(sizeof(chkpass));
+
+ /* Instead of random salt use constant value for consistent result */
+ mysalt[0] = salt_chars[1];
+ mysalt[1] = salt_chars[2];
+#endif
+
mysalt[2] = 0; /* technically the terminator is not necessary
* but I like to play safe */
Asif Naeem <anaeem.it@gmail.com> writes:
It is been observed on RANDOMIZE_ALLOCATED_MEMORY enabled PG95 build that
chkpass is failing because of uninitialized memory and seems showing false
alarm.
It's not a false alarm, unfortunately, because chkpass_in actually does
give different results from one call to the next. We could fix the aspect
of that involving failing to zero out unused bytes (which it appears was
introduced by sloppy replacement of strncpy with strlcpy). But we can't
really do anything about the dependency on random(), because that's part
of the fundamental specification of the data type. It was a bad idea,
no doubt, to design the input function to do this; but we're stuck with
it now.
I think these uninitialized memory areas are false alarms. Along with this
there seems another issue that makes chkpass troubling
RANDOMIZE_ALLOCATED_MEMORY comparison logic is that crypt() function is
being passed with random salt value for DES based result. PFA patch, to
generate consistent results, it uses constant salt value.
This is a seriously awful idea. You can't break the fundamental behavior
of a data type (especially not in a way that introduces a major security
hole) just for the convenience of some debugging option or other.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Feb 14, 2015 at 10:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Asif Naeem <anaeem.it@gmail.com> writes:
It is been observed on RANDOMIZE_ALLOCATED_MEMORY enabled PG95 build
that
chkpass is failing because of uninitialized memory and seems showing
false
alarm.
It's not a false alarm, unfortunately, because chkpass_in actually does
give different results from one call to the next. We could fix the aspect
of that involving failing to zero out unused bytes (which it appears was
introduced by sloppy replacement of strncpy with strlcpy). But we can't
really do anything about the dependency on random(), because that's part
of the fundamental specification of the data type. It was a bad idea,
no doubt, to design the input function to do this; but we're stuck with
it now.
It seems to me that fix for this issue has already been committed
(commit-id: 80986e85). So isn't it better to mark as Committed in
CF app [1]https://commitfest.postgresql.org/4/144/ or are you expecting anything more related to this issue?
[1]: https://commitfest.postgresql.org/4/144/
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes:
On Sat, Feb 14, 2015 at 10:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It's not a false alarm, unfortunately, because chkpass_in actually does
give different results from one call to the next. We could fix the aspect
of that involving failing to zero out unused bytes (which it appears was
introduced by sloppy replacement of strncpy with strlcpy). But we can't
really do anything about the dependency on random(), because that's part
of the fundamental specification of the data type. It was a bad idea,
no doubt, to design the input function to do this; but we're stuck with
it now.
It seems to me that fix for this issue has already been committed
(commit-id: 80986e85). So isn't it better to mark as Committed in
CF app [1] or are you expecting anything more related to this issue?
Ah, I didn't realize there was a CF entry for it, I think. Yeah,
I think we committed as much as we should of this, so I marked the
CF entry as committed.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thank you Tom, Thank you Amit.
Regards,
Muhammad Asif Naeem
On Wed, Mar 4, 2015 at 9:30 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Show quoted text
Amit Kapila <amit.kapila16@gmail.com> writes:
On Sat, Feb 14, 2015 at 10:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It's not a false alarm, unfortunately, because chkpass_in actually does
give different results from one call to the next. We could fix theaspect
of that involving failing to zero out unused bytes (which it appears was
introduced by sloppy replacement of strncpy with strlcpy). But we can't
really do anything about the dependency on random(), because that's part
of the fundamental specification of the data type. It was a bad idea,
no doubt, to design the input function to do this; but we're stuck with
it now.It seems to me that fix for this issue has already been committed
(commit-id: 80986e85). So isn't it better to mark as Committed in
CF app [1] or are you expecting anything more related to this issue?Ah, I didn't realize there was a CF entry for it, I think. Yeah,
I think we committed as much as we should of this, so I marked the
CF entry as committed.regards, tom lane