[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Sat, 16 Jul 2016 18:40:21 +0300
From: Solar Designer <solar@...nwall.com>
To: owl-dev@...ts.openwall.com
Subject: passwdqc code quality
Hi,
We were reported this bug a couple of days ago:
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.
This prompted me to check passwdqc code quality in other ways.
Yesterday, I entered it into Coverity:
https://scan.coverity.com/projects/passwdqc
It found exactly 1 defect, classified as a "medium impact" "error
handling issue", in pam_passwdqc.c. The issue is that we usually check
the return value from say(), but not always. Exactly one of those
places where we don't check the return value from say() looks wrong to
Coverity, presumably because in the rest of them the calling function is
about to return an error (or so it looks to Coverity) to the caller
anyway. The one place is:
173static int check_max(passwdqc_params_qc_t *qc, pam_handle_t *pamh,
174 const char *newpass)
175{ 1. Condition (int)strlen(newpass) > qc->max, taking true branch.
176 if ((int)strlen(newpass) > qc->max) { 2. Condition qc->max != 8, taking false branch.
177 if (qc->max != 8) {
178 say(pamh, PAM_ERROR_MSG, MESSAGE_TOOLONG);
179 return -1;
180 } CID 133745 (#1 of 1): Unchecked return value (CHECKED_RETURN)
3. check_return: Calling say without checking return value (as is done elsewhere 9 out of 10 times).
181 say(pamh, PAM_TEXT_INFO, MESSAGE_TRUNCATED);
182 }
183
184 return 0;
185}
In other words, if the warning message about the password being
truncated at 8 characters (when using the legacy descrypt) is not fully
delivered to the user, the current code will not report this as a
failure to the PAM library. I am not eager to fix this (too minor to be
worth complicating the code and require re-testing of this rarely used
code path, and an extra risk if we make changes without re-testing), and
if we do fix it perhaps there are a few more say() calls (not deemed
problematic by Coverity) where a similar "issue" exists.
Coverity did not find the uninitialized pw_dir issue.
I also built gcc-7-20160710 and built passwdqc with it with added
options "-fsanitize=address -fsanitize=leak -fsanitize=undefined".
Running the command-line programs (with "LD_LIBRARY_PATH=." to test the
corresponding build of the library as well) found only one issue: the
intentional memory leak in pwqgen.c, where it does not free() the random
passphrase strdup()'ed by passwdqc_random(). This does not matter since
pwqgen is about to exit anyway. If we do add the free(), this brings up
the question whether we also want to zeroize this generated passphrase
first, and then the question whether we want to avoid the passphrase
being in an stdio buffer (where we can't zeroize it as easily), such as
by setting stdout to non-buffered first. With the program about to
exit, all of this is moot, but zeroization makes slightly more sense
than a mere free().
I could not easily test pam_passwdqc with ASan, because doing so
requires(?) building a PAM application, like the passwd program, with
ASan as well, and I just didn't get around to this yet. Trying to
LD_PRELOAD the ASan library to our existing build of the passwd program
and running it as root didn't help. Oh, I just realized I should have
removed the SGID bit for this test.
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.
