Bug 2207 - Potential NULL deference, found using coverity
Summary: Potential NULL deference, found using coverity
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: -current
Hardware: Other FreeBSD
: P5 trivial
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_6_7
  Show dependency tree / graph
 
Reported: 2014-03-04 05:24 AEDT by Arthur Mesh
Modified: 2014-10-08 08:00 AEDT (History)
1 user (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Mesh 2014-03-04 05:24:33 AEDT 
This defect was found on OpenSSH 6.5; however, it appears that code in question has not changed between 6.5 and openssh-SNAP-20140204.tar.gz.

Thoughts:

while reading the code -- this caught attention:

authfile.c:
279         kdfname = buffer_get_cstring_ret(&copy, NULL);
280         if (kdfname == NULL ||
281             (!strcmp(kdfname, "none") && !strcmp(kdfname, "bcrypt"))) {
282                 error("%s: unknown kdf name", __func__);
283                 goto out;
284         }

'!strcmp(kdfname, "none") && !strcmp(kdfname, "bcrypt")' can never be true.

(I appologize that line numbers are offset by -3.)

Error: FORWARD_NULL:
path:/c/amesh/142/src/crypto/openssh/authfile.c:224:
cond_false: Condition "len < m1len", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:224:
cond_false: Condition "memcmp(cp, "-----BEGIN OPENSSH PRIVATE KEY-----\n", m1len)", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:227:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:230:
cond_true: Condition "len", taking true branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:231:
cond_true: Condition "*cp != 10", taking true branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:231:
cond_true: Condition "*cp != 13", taking true branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:236:
cond_false: Condition "last == 10", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:241:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:242:
loop: Jumping back to the beginning of the loop
path:/c/amesh/142/src/crypto/openssh/authfile.c:230:
loop_begin: Jumped back to beginning of loop
path:/c/amesh/142/src/crypto/openssh/authfile.c:230:
cond_true: Condition "len", taking true branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:231:
cond_false: Condition "*cp != 10", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:232:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:236:
cond_true: Condition "last == 10", taking true branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:237:
cond_true: Condition "len >= m2len", taking true branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:237:
cond_true: Condition "!memcmp(cp, "-----END OPENSSH PRIVATE KEY-----\n", m2len)", taking true branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:239:
break: Breaking from loop
path:/c/amesh/142/src/crypto/openssh/authfile.c:242:
loop_end: Reached end of loop
path:/c/amesh/142/src/crypto/openssh/authfile.c:243:
cond_false: Condition "!len", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:246:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:248:
cond_false: Condition "(cp = buffer_append_space(&copy, len)) == NULL", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:251:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:252:
cond_false: Condition "(dlen = uudecode(buffer_ptr(&encoded), cp, len)) < 0", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:255:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:256:
cond_false: Condition "(u_int)dlen > len", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:259:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:261:
cond_false: Condition "buffer_len(&copy) < 15U /* sizeof ("openssh-key-v1") */", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:261:
cond_false: Condition "memcmp(buffer_ptr(&copy), "openssh-key-v1", 15U /* sizeof ("openssh-key-v1") */)", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:265:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:269:
cond_false: Condition "ciphername == NULL", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:269:
cond_false: Condition "(c = cipher_by_name(ciphername)) == NULL", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:273:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:274:
cond_true: Condition "passphrase == NULL", taking true branch
/c/amesh/142/src/crypto/openssh/authfile.c:274:
var_compare_op: Comparing "passphrase" to null implies that "passphrase" might be null.
path:/c/amesh/142/src/crypto/openssh/authfile.c:274:
cond_false: Condition "strcmp(ciphername, "none") != 0", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:278:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:280:
cond_false: Condition "kdfname == NULL", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:280:
cond_false: Condition "!strcmp(kdfname, "none")", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:284:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:285:
cond_false: Condition "!strcmp(kdfname, "none")", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:288:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:291:
cond_false: Condition "kdfp == NULL", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:294:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:295:
cond_true: Condition "klen > 0", taking true branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:296:
cond_false: Condition "(cp = buffer_append_space(&kdf, klen)) == NULL", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:299:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:303:
cond_false: Condition "buffer_get_int_ret(&nkeys, &copy) < 0", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:306:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:307:
cond_false: Condition "nkeys != 1", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:310:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:312:
cond_false: Condition "(cp = buffer_get_string_ret(&copy, &len)) == NULL", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:315:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:322:
cond_false: Condition "len < blocksize", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:325:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:326:
cond_false: Condition "len % blocksize", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:329:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:335:
cond_true: Condition "!strcmp(kdfname, "bcrypt")", taking true branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:336:
cond_false: Condition "(salt = buffer_get_string_ret(&kdf, &slen)) == NULL", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:339:
if_end: End of if statement
path:/c/amesh/142/src/crypto/openssh/authfile.c:340:
cond_false: Condition "buffer_get_int_ret(&rounds, &kdf) < 0", taking false branch
path:/c/amesh/142/src/crypto/openssh/authfile.c:343:
if_end: End of if statement
/c/amesh/142/src/crypto/openssh/authfile.c:344:
var_deref_model: Passing null pointer "passphrase" to function "strlen(char const *)", which dereferences it.
Comment 1 Damien Miller 2014-03-04 08:27:38 AEDT 
We don't normally mark crashers as security bugs unless they take down the master sshd process.

That being said, there is no NULL dereference here anyway. See the "kdfname == NULL"

You are right about the logic error in testing the KDF name, but the impact of this is failure to read keys that have a KDF that is other than 'bcrypt' or 'none', which we would not be able to do anyway.
Comment 2 Arthur Mesh 2014-03-05 02:40:54 AEDT 
Not sure why e-mail correspondence didn't make it here -- hence reposting:
On Mon, Mar 03, 2014 at 09:27:38PM +0000, bugzilla-daemon@mindrot.org wrote:
> We don't normally mark crashers as security bugs unless they take down
> the master sshd process.

Noted.

> That being said, there is no NULL dereference here anyway. See the
> "kdfname == NULL"

Disagree. Let me try to be more specific:

Let's for the sake of argument assume:
kdfname = "bcrypt"
passphrase = NULL
ciphername = "none"

Please ignore this bug if such assumptions are invalid.

 277         if ((passphrase == NULL || !strlen(passphrase)) &&
 278             strcmp(ciphername, "none") != 0) {
 279                 /* passphrase required */
 280                 goto out;
 281         }

Given the assumption, condition in line 277 is false.

 283         if (kdfname == NULL ||
 284             (!strcmp(kdfname, "none") && !strcmp(kdfname, "bcrypt"))) {
 285                 error("%s: unknown kdf name", __func__);
 286                 goto out;
 287         }

Given the assumption, condition in line 283 is false.

 288         if (!strcmp(kdfname, "none") && strcmp(ciphername, "none") != 0) {
 289                 error("%s: cipher %s requires kdf", __func__, ciphername);
 290                 goto out;
 291         }

Furthermore, condition in 288 is false as well.

 338         if (!strcmp(kdfname, "bcrypt")) {
 339                 if ((salt = buffer_get_string_ret(&kdf, &slen)) == NULL) {
 340                         error("%s: salt not set", __func__);
 341                         goto out;
 342                 }
 343                 if (buffer_get_int_ret(&rounds, &kdf) < 0) {
 344                         error("%s: rounds not set", __func__);
 345                         goto out;
 346                 }
 347                 if (bcrypt_pbkdf(passphrase, strlen(passphrase), salt, slen,
 348                     key, keylen + ivlen, rounds) < 0) {
 349                         error("%s: bcrypt_pbkdf failed", __func__);
 350                         goto out;
 351                 }

Condition in 338 is true, and line 347 could produce a NULL dereference (strlen(NULL)).

Condition in 338 is true, and line 347 could produce a NULL dereference (strlen(NULL)).

(Again, assuming that lines 339 and 343 do not fail).

Perhaps I am missing something obvious here..

Thanks
Comment 3 Damien Miller 2014-03-05 03:16:10 AEDT 
nothing calls this function with a NULL passphrase
Comment 4 Damien Miller 2014-07-03 13:51:56 AEST 
I committed a big refactoring of the key code a couple of weeks ago. It adds a check for passphrase==NULL
Comment 5 Damien Miller 2014-10-08 08:00:29 AEDT 
Close all bugs left open from 6.6 and 6.7 releases.