713410 - Clang Static Analysis: Branch condition evaluates to a garbage value in security/nss/lib/ssl/ssl3con.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Christian Holler (:decoder)]

The following report (in the URL field) has been generated by static analysis using Clang.

It would be good if someone familiar with the particular code could check if

- this is really a bug or a false positive
- and/or if it makes sense to adjust the code (even if there is not a real bug present, e.g. by adding a missing initialization).


In this particular report, the key seems to be the comment in line 4436 which says that this condition shouldn't happen unless "something is wrong in sslsecur.c". However, the code attempts to do a cleanup by executing | goto ec_cleanup; | in line 4439. This will jump to line 4502 and execute | if (privWrapKey) SECKEY_DestroyPrivateKey(privWrapKey); |. However, privWrapKey seems uninitialized at that point.

There is also one other jump to "ec_cleanup" where a variable is uninitialized:

http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-yH9ud8.html#EndPath 

In that case, it's the | ks | variable though (but the very similar problem).

Comment 1

[Kai Engert [:KaiE:]]

> In this particular report, the key seems to be the comment in line 4436
> which says that this condition shouldn't happen unless "something is wrong
> in sslsecur.c". However, the code attempts to do a cleanup by executing |
> goto ec_cleanup; | in line 4439. This will jump to line 4502 and execute |
> if (privWrapKey) SECKEY_DestroyPrivateKey(privWrapKey); |. However,
> privWrapKey seems uninitialized at that point.

I don't understand why you say "privWrapKey seems uninitialized".

The variables are declared and initialized to zero.
    SECKEYPublicKey   *pubWrapKey = NULL;
    SECKEYPrivateKey  *privWrapKey = NULL;

If privWrapKey is zero, the statement 
    if (privWrapKey) SECKEY_DestroyPrivateKey(privWrapKey);
seems fine to me.


> There is also one other jump to "ec_cleanup" where a variable is
> uninitialized:
> 
> In that case, it's the | ks | variable though (but the very similar problem).

Ks is also initialized to zero
	if (Ks) PK11_FreeSymKey(Ks);


To me it seems this bug is invalid, as all vars are zero initialized, and the cleanup functions are called if the variables are nonzero, only.

Comment 2

[Kai Engert [:KaiE:]]

Reopening.

Christian, thanks for making me aware why this code is indeed broken.

I didn't notice that those variables are declared inside a switch statement. I can't remember having seen such a scenario before.

As you pointed out, the variable is declared, but the initialization part is silent ignored by the compiler, and have random content. I wouldn't have expected that! But I can reproduce using a minimal test case and local testing.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 4

[Christian Holler (:decoder)]

Marking this s-s. It's not clear if this situation can occur, but if it would occur then it would certainly lead to a crash during the SSL key negotiation which would be potentially dangerous.

Comment 5

[Kai Engert [:KaiE:]]

This code was added in bug 238051.

Comment 6

[Christian Holler (:decoder)]

I reviewed the code once more to determine the security impact here:

- Both conditions (if possible) would free an uninitialized pointer. It is hard to determine if the pointer content can be influenced or controlled by an attacker because it usually depends on the previous use of that memory area (one possibility would be to control the data through a previous function call on the same stack depth with data that the attacker can influence). Overall not easy to control/exploit.

- As for the first condition (privWrapKey being free'd uninitialized), the comment "something is wrong in sslsecur.c if this isn't an ecKey" should be checked to determine if this can really happen. I'm not familiar with this code but it would be interesting to know why the key type information is available twice here (one is | sid->u.ssl3.exchKeyType | and the other is | svrPubKey->keyType |, do they always have to be the same?).

- The second condition seems to be more likely to happen: If the function call to | SECKEY_CreateECPrivateKey | in line 4442 returns NULL, then | ks | is free'd uninitalized. There seem to be multiple conditions that could lead to this function returning NULL. The most obvious one for me was in function | PK11_MakePrivKey | which returns NULL either when failing to allocate memory or when token authentication fails. That return value is propagated back to where we need it for the failure to happen. Another variant might be if something is wrong with the | param | parameter, which is | &svrPubKey->u.ec.DEREncodedParams |. I assume these are parameters of the remote public key? If so, could these be used to trigger a failure in | SECKEY_CreateECPrivateKey | or its callees' to return NULL?

Comment 7

[Daniel Veditz [:dveditz]]

for now "sg:moderate" unless we find a way to trigger this IRL.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 589676 [details] [diff] [review]
Patch v1

I'm no NSS peer, but I am a mozilla sr, and I'm comfortable saying r+ on this change. r=jst fwiw.

Comment 9

[Kai Engert [:KaiE:]]

We still need a NSS person to r+. Bob, Brian?

Thanks jst!

Comment 10

[Kai Engert [:KaiE:]]

Comment on attachment 589676 [details] [diff] [review]
Patch v1

Bob, the one minute summary is:

Variable inits inside a switch statement are ignored by the compiler.
Move init statements to a place where the compiler will use them.

(We've reported this to the gcc people,
 suggesting the compiler should warn
 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51896
)

Comment 11

[Robert Relyea]

Comment on attachment 589676 [details] [diff] [review]
Patch v1

r+ rrelyea

Comment 12

[Kai Engert [:KaiE:]]

cvs commit: Examining .
Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.164; previous revision: 1.163
done

Comment 13

[Alex Keybl [:akeybl]]

We'll track NSS uplifts to the ESR separately from individual bugs.