[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160716162719.GA18982@openwall.com>
Date: Sat, 16 Jul 2016 19:27:19 +0300
From: Solar Designer <solar@...nwall.com>
To: owl-dev@...ts.openwall.com
Subject: Re: passwdqc code quality
On Sat, Jul 16, 2016 at 06:40:21PM +0300, Solar Designer wrote:
> https://bugs.debian.org/831356
>
> The problem is that when we modified passwdqc_check() to use pw_dir in
> 2009, we didn't similarly update pam_passwdqc.c to set pw_dir in fake_pw
> for when the "non-unix" option is used. Looking at it another way, a
> problem was that we didn't fully initialize fake_pw right away, just in
> case of future code changes like this. I'll fix both of these shortly.
As a test, I added just:
memset(pw, 0, sizeof(*pw));
right after "pw = &fake_pw;"
Now I am finally able to reproduce the reported issue (with "non-unix"
in passwdqc.conf):
---
Enter new password:
ASAN:DEADLYSIGNAL
=================================================================
==498503==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x2b854b02b510 bp 0x7fffe42b7120 sp 0x7fffe42b68a8 T0)
#0 0x2b854b02b50f in __GI_strlen (/lib64/libc.so.6+0x7150f)
#1 0x2b8548d335ac in __interceptor_strlen ../../../../libsanitizer/asan/asan_interceptors.cc:577
#2 0x2b854ef8067f in unify (/lib64/libpasswdqc.so.0+0x167f)
#3 0x2b854ef80f25 in passwdqc_check (/lib64/libpasswdqc.so.0+0x1f25)
#4 0x2b854ed739fe in pam_sm_chauthtok (/lib64/security/pam_passwdqc.so+0x59fe)
#5 0x2b854a9a70fc in _pam_dispatch (/lib64/libpam.so.0+0x30fc)
#6 0x2b854a9ab350 in pam_chauthtok (/lib64/libpam.so.0+0x7350)
#7 0x400ea0 (/usr/bin/passwd+0x400ea0)
#8 0x2b854afd78f0 in __libc_start_main ../sysdeps/generic/libc-start.c:209
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib64/libc.so.6+0x7150f) in __GI_strlen
==498503==ABORTING
---
Without the memset(), the issue was not detected by ASan. So it looks
like ASan is of little help for issues like this as the segfault would
be immediately visible without it anyway, when the segfault does occur,
whereas in our default build on Owl it did not.
This memset() reminds me: we also have many memset() calls trying to
zeroize things. This always made little sense, and it makes even less
sense with modern compilers, which tend to optimize such calls away.
So maybe one of the code quality aspects is to add a source file with a
slightly less unreliable memory zeroization function, and use that.
Maybe like Colin Percival's insecure_memzero():
https://github.com/Tarsnap/libcperciva/blob/master/util/insecure_memzero.c
http://www.daemonology.net/blog/2014-09-04-how-to-zero-a-buffer.html
http://www.daemonology.net/blog/2014-09-06-zeroing-buffers-is-insufficient.html
Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
