792811 - Crash in ASan-ized unit tests in ssl_ConfigSecureServer

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Eric Rescorla (:ekr)]

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x000000013b8e0d60
[Switching to process 37410 thread 0x3d03]
0x000000013730fbec in definite_length_decoder (buf=0x1373fec80 "definite_length_decoder 9 32 8 6 retval 96 8 8 buf.addr 160 4 11 length.addr 224 8 16 data_length.addr 288 4 15 includeTag.addr 352 1 3 tag 416 4 11 used_length 480 4 8 data_len 544 4 9 len_count ", length=926936192, data_length=0x1373fec80, includeTag=926936192) at quickder.c:22
22	{
(gdb) up
#1  0x0000000137305eb8 in GetItem (src=<value temporarily unavailable, due to optimizations>, dest=<value temporarily unavailable, due to optimizations>, includeTag=<value temporarily unavailable, due to optimizations>) at quickder.c:87
87	    dest->data = definite_length_decoder(src->data,  src->len, &dest->len,
(gdb) up
#2  0x0000000137302909 in DecodeItem (dest=<value temporarily unavailable, due to optimizations>, templateEntry=<value temporarily unavailable, due to optimizations>, src=<value temporarily unavailable, due to optimizations>, arena=<value temporarily unavailable, due to optimizations>, checkTag=<value temporarily unavailable, due to optimizations>) at quickder.c:678
678	        rv = GetItem(src, &temp, PR_TRUE);
(gdb) up
#3  0x000000013730e925 in DecodeSequence (dest=<value temporarily unavailable, due to optimizations>, templateEntry=<value temporarily unavailable, due to optimizations>, src=<value temporarily unavailable, due to optimizations>, arena=<value temporarily unavailable, due to optimizations>) at quickder.c:370
370	            rv = DecodeItem(dest, sequenceEntry, &sequence, arena, PR_TRUE);
(gdb) up
#4  0x0000000137303f54 in DecodeItem (dest=<value temporarily unavailable, due to optimizations>, templateEntry=<value temporarily unavailable, due to optimizations>, src=<value temporarily unavailable, due to optimizations>, arena=<value temporarily unavailable, due to optimizations>, checkTag=<value temporarily unavailable, due to optimizations>) at quickder.c:777
777	            rv = DecodeSequence(dest, templateEntry, &temp , arena);
(gdb) up
#5  0x0000000137309363 in DecodeInline (dest=<value temporarily unavailable, due to optimizations>, templateEntry=<value temporarily unavailable, due to optimizations>, src=<value temporarily unavailable, due to optimizations>, arena=<value temporarily unavailable, due to optimizations>, checkTag=<value temporarily unavailable, due to optimizations>) at quickder.c:396
396	    return DecodeItem((void*)((char*)dest + templateEntry->offset),
(gdb) up
#6  0x0000000137303083 in DecodeItem (dest=<value temporarily unavailable, due to optimizations>, templateEntry=<value temporarily unavailable, due to optimizations>, src=<value temporarily unavailable, due to optimizations>, arena=<value temporarily unavailable, due to optimizations>, checkTag=<value temporarily unavailable, due to optimizations>) at quickder.c:719
719	            rv = DecodeInline(dest, templateEntry, &temp , arena, PR_TRUE);
(gdb) up
#7  0x000000013730e925 in DecodeSequence (dest=<value temporarily unavailable, due to optimizations>, templateEntry=<value temporarily unavailable, due to optimizations>, src=<value temporarily unavailable, due to optimizations>, arena=<value temporarily unavailable, due to optimizations>) at quickder.c:370
370	            rv = DecodeItem(dest, sequenceEntry, &sequence, arena, PR_TRUE);
(gdb) up
#8  0x0000000137303f54 in DecodeItem (dest=<value temporarily unavailable, due to optimizations>, templateEntry=<value temporarily unavailable, due to optimizations>, src=<value temporarily unavailable, due to optimizations>, arena=<value temporarily unavailable, due to optimizations>, checkTag=<value temporarily unavailable, due to optimizations>) at quickder.c:777
777	            rv = DecodeSequence(dest, templateEntry, &temp , arena);
(gdb) up
#9  0x0000000137301518 in SEC_QuickDERDecodeItem_Util (arena=<value temporarily unavailable, due to optimizations>, dest=<value temporarily unavailable, due to optimizations>, templateEntry=<value temporarily unavailable, due to optimizations>, src=<value temporarily unavailable, due to optimizations>) at quickder.c:887
887	        rv = DecodeItem(dest, templateEntry, &newsrc, arena, PR_TRUE);
(gdb) up
#10 0x0000000136895a10 in CERT_SerialNumberFromDERCert (derCert=<value temporarily unavailable, due to optimizations>, derName=<value temporarily unavailable, due to optimizations>) at certdb.c:347
347	    rv = SEC_QuickDERDecodeItem(arena, &sd, CERT_SignedDataTemplate, derCert);
(gdb) up
#11 0x00000001369e5f34 in STAN_GetNSSCertificate (cc=<value temporarily unavailable, due to optimizations>) at pki3hack.c:1002
1002		secrv = CERT_SerialNumberFromDERCert(&cc->derCert, &derSerial);
(gdb) up
#12 0x00000001368a4f42 in CERT_DupCertificate (c=<value temporarily unavailable, due to optimizations>) at certdb.c:1275
1275		NSSCertificate *tmp = STAN_GetNSSCertificate(c);
(gdb) up
#13 0x00000001363dd6b3 in ssl_ConfigSecureServer (ss=<value temporarily unavailable, due to optimizations>, cert=<value temporarily unavailable, due to optimizations>, certChain=<value temporarily unavailable, due to optimizations>, keyPair=<value temporarily unavailable, due to optimizations>, kea=<value temporarily unavailable, due to optimizations>) at sslsecur.c:680
680	        sc->serverCert = CERT_DupCertificate(cert);
(gdb) down
#12 0x00000001368a4f42 in CERT_DupCertificate (c=<value temporarily unavailable, due to optimizations>) at certdb.c:1275
1275		NSSCertificate *tmp = STAN_GetNSSCertificate(c);
(gdb) up
#13 0x00000001363dd6b3 in ssl_ConfigSecureServer (ss=<value temporarily unavailable, due to optimizations>, cert=<value temporarily unavailable, due to optimizations>, certChain=<value temporarily unavailable, due to optimizations>, keyPair=<value temporarily unavailable, due to optimizations>, kea=<value temporarily unavailable, due to optimizations>) at sslsecur.c:680
680	        sc->serverCert = CERT_DupCertificate(cert);
(gdb) up
#14 0x00000001363e014d in SSL_ConfigSecureServerWithCertChain (fd=<value temporarily unavailable, due to optimizations>, cert=<value temporarily unavailable, due to optimizations>, certChainOpt=<value temporarily unavailable, due to optimizations>, key=<value temporarily unavailable, due to optimizations>, kea=<value temporarily unavailable, due to optimizations>) at sslsecur.c:819
819	    if (ssl_ConfigSecureServer(ss, cert, certChainOpt,
(gdb) up
#15 0x00000001363deb8d in SSL_ConfigSecureServer (fd=<value temporarily unavailable, due to optimizations>, cert=<value temporarily unavailable, due to optimizations>, key=<value temporarily unavailable, due to optimizations>, kea=<value temporarily unavailable, due to optimizations>) at sslsecur.c:741
741	    return SSL_ConfigSecureServerWithCertChain(fd, cert, NULL, key, kea);
(gdb) up
#16 0x000000010015c05e in TransportLayerDtls::Setup (this=<value temporarily unavailable, due to optimizations>) at transportlayerdtls.cpp:468
468	                                identity_->privkey(),
Current language:  auto; currently c++

Comment 2

[Daniel Veditz [:dveditz]]

Removing [blocking-webrtc-] in hopes that's the right way to request reconsideration. This bug pops up enough that it's blocking security fuzz testing of SPC, and that really ought to block webrtc.

This is a case where it would have made more sense to dupe forward since the other bug has a minimal testcase in attachment 664555 [details]

Comment 3

[Christoph Diehl [:posidron]]

Attachment: testcase

Adding testcase which reproduces with an ASan enabled build and a normal debug build.

Comment 4

[Christoph Diehl [:posidron]]

Attachment: callstack

The testcase is the same like in 794139; the crash location/callstack differs in ASan and non-ASan-opt build.

Comment 5

[Christoph Diehl [:posidron]]

Attachment: debuglog

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Randell can you take this or find an owner?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

I'll take it.  I may ask ekr to help

Comment 8

[Eric Rescorla (:ekr)]

Jesup,

I don't really have any good guesses.... I would have thought this was a free memory read, but I don't see any evidence of htat...

Comment 9

[Christoph Diehl [:posidron]]

Attachment: callstack-assertion-rtp_transport_

changeset:   110172:7f1694a507dd (tip)

Testcase is producing a different assertion failure here.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Updated testcase for current m-c and alder

creating fake media streams changed...

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

fingerprint:sha-1 61:57:a2:6f:91:8f:8a:5a:5b:28:a9:55:c1:ce:e5:83:02:cf:17:3f

RFC 4752:
"The hash value is represented as a sequence of uppercase hexadecimal bytes, separated by colons."

So this is a valid failure to parse.  The question is why that causes a crash.

MediaPipeline::MediaPipeline() seems to allow for muxed rtp/rtcp  (initializer has "muxed_((rtcp_transport_ == NULL) || (rtp_transport_ == rtcp_transport_)") (though we haven't implemented it yet), but inside it does a  MOZ_ASSERT(rtp_transport_) which is where we fail.

The real issue is that in vcmRxStartICE() we don't check for failures to create rtp/rtcp flows.

-> ekr

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Note: don't land this patch (once it's made) without first asking for sec-approval? in whiteboard here and getting approval.

Impact to security of this bug is minimal since webrtc is preffed-off by default.

Comment 13

[Eric Rescorla (:ekr)]

Attachment: Don't crash on failure to create WebRTC transport;

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 669691 [details] [diff] [review]
Don't crash on failure to create WebRTC transport;

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Unsure, as the main failure is a null-deref.  It's unclear how the asan failure is caused by this; in debug builds this is caught by the MOZ_ASSERT.  I'll assume moderately easy.  This is strongly mitigated by it being preffed-off by default.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It clearly shows an asan failure.

Which older supported branches are affected by this flaw?
m-c and Aurora are affected

If not all supported branches, which bug introduced the flaw?
Landing of WebRTC and bug 790517

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply directly to Aurora

How likely is this patch to cause regressions; how much testing does it need?
Minimal; clear fix.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

>Do comments in the patch, the check-in comment, or tests included in the patch >paint a bulls-eye on the security problem?
>It clearly shows an asan failure.

I should revise that: the *bug* clearly shows an ASAN failure; the patches would probably lead one to expect a null-deref failure.

Comment 16

[Eric Rescorla (:ekr)]

Yes. I'm a little confused about what was triggering ASAN. Note that the ASAN failure is deep within NSS.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Yes, and looking at MediaPipeline::Init() (which fired the ASSERT we fixed here) would immediately deref the null pointer if you don't have the ASSERT - so either it was kicking out earlier, or there's an independent problem.

Looking more closely, I think the ASSERT cdiehl found is a separate bug from the initial report here by ekr.  I think the ASSERT is a straightforward NULL-deref bug.  The deep access violation in NSS looks to come from the second PushLayer() in vcmCreateTransportFlow(), after it has successfully passed the fingerprint parsing that cdiehl's test failed (and thus ended up triggering the assert). Unfortunately, ekr's traceback doesn't go quite far enough back to be sure, but I can't see anywhere else likely tracing back from ::Setup().

So I'd suggest spinning the null-deref to a new non-security bug (with patch), and try again to find how we could (with a good fingerprint) cause this problem.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeba6e117081

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 667116 [details]
testcase

Original testcase.  Note that I think this will now fail out before even getting to trying to do ::Setup() due to lower-case fingerprints; lowercase may have worked when this was originally generated (ekr made changes to fingerprint parsing in response to bsmith).

Also will fail due to fakemediastream code needing to be updated (easy)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eeba6e117081

Comment 21

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

Why is this still open?

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Al Billings [:abillings] from comment #21)
> Why is this still open?

See comment 18 (and comment 14).  This bug should be retested to see if the ASAN failure still exists; if not then this can probably be closed, but my analysis leads me to believe the patch committed is for an independent bug from the reported ASAN failure.  cdiehl, can you still make this bug happen?

Comment 23

[Christoph Diehl [:posidron]]

Testcase produces:

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Error: createAnswer passed invalid constraints' when calling method: [nsIDOMRTCPeerConnection::createAnswer]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "JS frame :: file:///Users/cdiehl/Downloads/bug792811.html :: step3 :: line 53"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Error' when calling method: [RTCPeerConnectionCallback::onCallback]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "JS frame :: file:///Users/cdiehl/Code/Mozilla/mc-asan-opt/debug_build/dist/NightlyDebug.app/Contents/MacOS/components/PeerConnection.js :: <TOP_LEVEL> :: line 540"  data: no]

No crash.

It also did not show up in my latest unit-test runs.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

The testcase has bitrotted if we're getting invalid constraints; we changed the createOffer/createAnswer API.  Anant, can you help?

Comment 25

[Anant Narayanan [:anant]]

Attachment: Updated testcase

createAnswer no longer takes an offer as an argument (since it is provided by setRemoteDescription).

Comment 26

[Christoph Diehl [:posidron]]

Fixed.

Comment 27

[bhavana bajaj [:bajaj]]

(In reply to Randell Jesup [:jesup] from comment #22)
> (In reply to Al Billings [:abillings] from comment #21)
> > Why is this still open?
> 
> See comment 18 (and comment 14).  This bug should be retested to see if the
> ASAN failure still exists; if not then this can probably be closed, but my
> analysis leads me to believe the patch committed is for an independent bug
> from the reported ASAN failure.  cdiehl, can you still make this bug happen?

This seems to be fixed as per comment 26.Are we ready for aurora uplift here ?

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 669691 [details] [diff] [review]
Don't crash on failure to create WebRTC transport;

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Initial checkin

User impact if declined: 
  Not large, as PeerConnection is preffed off by default on FF18; the only risk is for crashes among those who pref it on.

Testing completed (on m-c, etc.): 
  Retested by cdiehl using ASAN, passes.  On m-c for a month with no regressions.

Risk to taking this patch (and alternatives if risky): 
  Very low (as it's preffed off by default) and the patch is a trivial failure check.

String or UUID changes made by this patch: 
  None

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cfc44602b21a