openssh bugs - Mar 2014 - [Bug 2207] New: Potential NULL deference, found using coverity

https://bugzilla.mindrot.org/show_bug.cgi?id=2207

            Bug ID: 2207
           Summary: Potential NULL deference, found using coverity
           Product: Portable OpenSSH
           Version: -current
          Hardware: Other
                OS: FreeBSD
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: arthurmesh at gmail.com

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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2207

Arthur Mesh <arthurmesh at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|enhancement                 |security

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2207

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
           Severity|security                    |trivial

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2207

--- Comment #2 from Arthur Mesh <arthurmesh at gmail.com> ---
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 at 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

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2207

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
nothing calls this function with a NULL passphrase

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2207

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Blocks|                            |2226
             Status|NEW                         |RESOLVED

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
I committed a big refactoring of the key code a couple of weeks ago. It
adds a check for passphrase==NULL

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2207

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Close all bugs left open from 6.6 and 6.7 releases.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.