956033 - Assertion failure: !ss->firstHsDone, at c:/Users/mozilla/debug-builds/mozilla-central/security/nss/lib/ssl/ssl3con.c:10052

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Carsten Book [:Tomcat]]

Attachment: linux crash stack

found via bughunter while loading https://www.onlinesubmit.in/mha/Account/Login.aspx (some goverment page from india it seems).

Assertion failure: !ss->firstHsDone, at c:/Users/mozilla/debug-builds/mozilla-central/security/nss/lib/ssl/ssl3con.c:10052

Steps to reproduce: 
-> Latest M-c Windows 7 Debug Build
-> Load https://www.onlinesubmit.in/mha/Account/Login.aspx
-> Crash/Assertion Failure in Debug Build


according to bughunter also happens on linux and mac

Comment 1

[Carsten Book [:Tomcat]]

Hey Brian, since you looked at the other NSS/SSL issues i found via bughunter i wonder if you want to look at this too

Comment 2

[Matt Wobensmith [:mwobensmith][:matt:]]

Brian, do you have any advice on how to rate this?

Comment 3

[Wan-Teh Chang]

Attachment: Patch

The assertion is incorrect because an NSS client may call
SSL_AuthCertificateComplete during a renegotiation, in which case
ss->firstHsDone is true.

I also removed an unnecessary assertion and check for !ss->sec.isServer.
This condition is ensured by the "if (ss->sec.isServer)" error return at
the beginning of the function.

Comment 4

[Wan-Teh Chang]

(In reply to Matt Wobensmith from comment #2)
> Brian, do you have any advice on how to rate this?

Based on my understanding of this bug, I think this is not a security bug
and we can make this bug public.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8355354 [details] [diff] [review]
Patch

Review of attachment 8355354 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Wan-Teh Chang from comment #3)
> The assertion is incorrect because an NSS client may call
> SSL_AuthCertificateComplete during a renegotiation, in which case
> ss->firstHsDone is true.

That is true, but the important question is why is restartTarget null in this scenerio?

During a renegotiation, ssl3_SendClientSecondRound will set restartTarget to itself. The assertion in ssl3_AuthCertificateComplete is there primarily to verify that that was done. Perhaps it is the case that SSL_AuthCertificateComplete is getting called between receiving the server's Certificate message ad receiving the server's ServerHelloDone message. AFAICT, this would create the scenerio where restartTarget is null when SSL_AuthCertificateComplete is called.

I just looked at this in the debugger, and IIUC ss->ssl3.hs.ws was wait_cert_request in the debugger when the assertion failed. This is evidence that the explanation in the previous paragraph is correct.

Another interesting question: Why did this just get reported now? We have been using the asynchronous cert verification logic for a very long time, over many releases. I suspect that this may be due to enabling False Start and/or due to the False-Start-related changes we made to the gather logic. Or, perhaps we're just more thoroughly testing debug builds now.

Comment 6

[Wan-Teh Chang]

Comment on attachment 8355354 [details] [diff] [review]
Patch

Review of attachment 8355354 [details] [diff] [review]:
-----------------------------------------------------------------

Patch checked in: https://hg.mozilla.org/projects/nss/rev/677eab0d7f6f

I guess this server does not send the ServerHello ... ServerHelloDone
sequence in one shot. This behavior is uncommon, and few people run
debug builds, which would explain why we didn't hit this assertion
failure earlier.

Comment 7

[Wan-Teh Chang]

The assertion that failed was added in a patch for bug 713933, during
NSS 3.15.4 development.

Comment 8

[Camilo Viecco (:cviecco)]

Comment on attachment 8355354 [details] [diff] [review]
Patch

Review of attachment 8355354 [details] [diff] [review]:
-----------------------------------------------------------------

Closing review as aready checked in.

Comment 9

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

Marking ESR24 affected since it doesn't have NSS 3.15.4.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #7)
> The assertion that failed was added in a patch for bug 713933, during
> NSS 3.15.4 development.

(In reply to Al Billings [:abillings] from comment #9)
> Marking ESR24 affected since it doesn't have NSS 3.15.4.

Based on Wan-Teh's comment, this doesn't affect ESR24 because ESR24 didn't have any version of NSS 3.15.4.

Opening up the bug because it isn't a security issue.

Comment 11

[Kai Engert (:KaiE:)]

> this doesn't affect ESR24 because ESR24 didn't
> have any version of NSS 3.15.4.

I didn't understand when first reading it, so just to clarify, 
not affected because ESR24 never used any of the "beta" versions, which were the only ones producing this assertion.