ASan: NULL deref in PK11_SaveContextAlloc

RESOLVED FIXED in 3.21

Status

P1
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tsmith, Assigned: ekr)

Tracking

(Blocks: 1 bug, 4 keywords)

3.21
3.21
crash, csectype-nullptr, regression, testcase

Firefox Tracking Flags

(firefox43 affected)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Comment 1

3 years ago
Created attachment 8656824 [details]
test_case
(Assignee)

Updated

3 years ago
Group: crypto-core-security
(Assignee)

Comment 2

3 years ago
This should probably be marked as security until indicated otherwise.
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
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?
(Assignee)

Updated

3 years ago
Flags: needinfo?(wtc)
Flags: needinfo?(martin.thomson)
(Reporter)

Comment 5

3 years ago
Created attachment 8656858 [details]
log_with_backtrace.txt
Attachment #8656823 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
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.
Flags: needinfo?(martin.thomson) → needinfo?(twsmith)
(Reporter)

Comment 8

3 years ago
Created attachment 8657278 [details]
nss_3_20.log.txt

Verified that this crash is not present in NSS 3.20
Flags: needinfo?(twsmith)

Comment 9

3 years ago
Eric: I commented in the Rietveld code review.
Flags: needinfo?(wtc)
Keywords: regression
(Assignee)

Comment 10

3 years ago
r+ from MT and WTC

Landed as:
https://hg.mozilla.org/projects/nss/rev/ac6a7b6bc48c
Flags: needinfo?(dveditz)
(Assignee)

Comment 11

3 years ago
Per discussion with WTC, I believe it is fine to unhide this bug.
(Assignee)

Comment 12

3 years ago
dveditz, actually hold off until I verify with tyson's original test case.
Flags: needinfo?(dveditz)
(Assignee)

Comment 13

3 years ago
It looks good to me w/o ASan. Tyson, please test with ASan against trunk?
Flags: needinfo?(twsmith)
(Reporter)

Comment 14

3 years ago
(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.
Flags: needinfo?(twsmith)
Opening this up as a null deref now fixed.
Group: crypto-core-security
Can we resolve this as fixed?
I don't see why not.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.21

Comment 18

3 years ago
This bug was introduced in NSS 3.21 (pre-release) by the TLS session hash patch
(bug 1117022).
Assignee: nobody → ekr
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Version: trunk → 3.21
You need to log in before you can comment on or make changes to this bug.