Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Sat, 16 Jul 2016 18:40:21 +0300
From: Solar Designer <>
Subject: passwdqc code quality


We were reported this bug a couple of days ago:

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:

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        }
184        return 0;

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.


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.