Обсуждение: 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.
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
chkpass_in()
result memory
crypt_output memory
result memory
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
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
Вложения
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
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.
>
>
> 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] or are you expecting anything more related to this issue?
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? > [1]: https://commitfest.postgresql.org/4/144/ 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
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:
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?
> [1]: https://commitfest.postgresql.org/4/144/
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