1201704 - ASan: NULL deref in PK11_SaveContextAlloc

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: call stack

Found using a fuzzing harness:
https://github.com/tysmith/nss/blob/fuzz_support/external_tests/ssl_gtest/ssl_fuzz.cc

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case

Comment 2

[Eric Rescorla (:ekr)]

This should probably be marked as security until indicated otherwise.

Comment 3

[Eric Rescorla (:ekr)]

This doesn't cause a crash on my machine (w/o ASan), which is surprising.

My guess of what's going on here is that the handshake hashes are initialized
in ssl3_Handle{Client,Server}Hello and so if you get a different message first
you have a null context.

Comment 4

[Eric Rescorla (:ekr)]

OK, I have now reproduced the problem by applying this fuzz test to the main
tree. This is a regression introduced by the session hash patch.

The issue here is that the code in ssl3_HandleHandshakeMessage that invokes ssl3_ComputeHandshakeHashes
does not enforce that we are in the right state, and because (as indicated in c3),
we are getting a message of type CertificateVerify before we get a ServerHello
or a ClientHello, we try to invoke ssl3_ComputeHandshakeHashes with a null hash.
Prior to the session hash patch, this generated an error because we checked
for the presence of pwSpec->master_secret, which is nonzero at this stage
in the handshake and so served as an implicit state check. However, we moved that
check out and so now we get further in the function and fail.

There are two possible fixes:

1. Check in ssl3_ComputeHandshakeHashes that the handshake hash contexts
are initialized.
2. Check that we are expecting this message in ssl3_HandleHandshakeMessage.

The second check is probably more robust, but is more of a layering problem.
I suggest instead we just introduce checks in ssl3_ComputeHandshakeHashes
for initialization of the hash contexts, which will bring us back approximately
to where we were before this patch landed.

WTC? MT?

Comment 5

[Tyson Smith [:tsmith]]

Attachment: log_with_backtrace.txt

Comment 6

[Eric Rescorla (:ekr)]

Patch up at:
https://codereview.appspot.com/267750043/

Comment 7

[Eric Rescorla (:ekr)]

Wan-Teh,

I have Martin's r+ on Rietveld and would like to land this  and open the bug
because:

1. It is a null pointer dereference.
2. It only seems to affect the trunk and not released versions.

Tyson, can you please confirm point #2. Wan-Teh, if you object please let me know
by COB today. Otherwise, I will plan to land over the weekend.

Comment 8

[Tyson Smith [:tsmith]]

Attachment: nss_3_20.log.txt

Verified that this crash is not present in NSS 3.20

Comment 9

[Wan-Teh Chang]

Eric: I commented in the Rietveld code review.

Comment 10

[Eric Rescorla (:ekr)]

r+ from MT and WTC

Landed as:
https://hg.mozilla.org/projects/nss/rev/ac6a7b6bc48c

Comment 11

[Eric Rescorla (:ekr)]

Per discussion with WTC, I believe it is fine to unhide this bug.

Comment 12

[Eric Rescorla (:ekr)]

dveditz, actually hold off until I verify with tyson's original test case.

Comment 13

[Eric Rescorla (:ekr)]

It looks good to me w/o ASan. Tyson, please test with ASan against trunk?

Comment 14

[Tyson Smith [:tsmith]]

(In reply to Eric Rescorla (:ekr) from comment #13)
> It looks good to me w/o ASan. Tyson, please test with ASan against trunk?

Passed! Retested with rev ac6a7b6bc48c.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Opening this up as a null deref now fixed.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Can we resolve this as fixed?

Comment 17

[Martin Thomson [:mt:]]

I don't see why not.

Comment 18

[Wan-Teh Chang]

This bug was introduced in NSS 3.21 (pre-release) by the TLS session hash patch
(bug 1117022).