AV crash in mozilla::net::nsHttpChannel::SetWarningReporter

RESOLVED FIXED in Firefox 60

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: qab, Assigned: mayhemer)

Tracking

60 Branch
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment, 1 obsolete attachment)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.167 Safari/537.36

Steps to reproduce:

1. Open 'view-source:https://example.org'
2. Inject the following JS using devtools:

	var qReq = new XMLHttpRequest();
	qReq.addEventListener("load", function(e){console.dir(e)});
	qReq.addEventListener("error", function(e){console.dir(e)});
	qReq.open('DELETE', 'view-source:https://bing.com', true,'test','test');
	qReq.send();


Actual results:

Browser crashes with AV crash

https://crash-stats.mozilla.com/report/index/80ff0a43-57b2-4aae-a731-424ba0180223#tab-details
Looks like something under our safebrowsing code, but normally we don't expect view-source: to make network connections!

Normal pages can't load view-source: urls either!
Group: firefox-core-security → network-core-security
Status: UNCONFIRMED → NEW
Component: Untriaged → Networking
Ever confirmed: true
Keywords: sec-low
Product: Firefox → Core
(In reply to Daniel Veditz [:dveditz] from comment #1)
> Looks like something under our safebrowsing code, but normally we don't
> expect view-source: to make network connections!

I don't think it's true.  I can see only LOAD_FROM_CACHE being set on the inner channel of a view-source channel at the time of asyncOpen'ing it.  It means, look into the cache, don't validate at all, but if it's not there, go to network.

I think if we enforce loading only from cache (add LOAD_ONLY_FROM_CACHE) we may break view source under some circumstances.

Anyway, in this case, seeing an http channel being created means we are using it to fetch the content, not necessarily we are doing a network (going out) request.

> 
> Normal pages can't load view-source: urls either!

not sure view-source: top level load is a "normal page".


I think what we are missing here is simply a null check.  preHttp is nsViewSourceChannel, which QIs to nsIHttpChannel.  I had to see this during the review :/
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Priority: -- → P2
Posted patch v1 (obsolete) — Splinter Review
note that with this patch we don't crash and the XHR fails with an error.

what I'm seing in a log: there are two http channels created for https://bing.com/

1. coming from debugger eval code:5:1 (=qReq.send();), expected
2. created on the parent process by nsCORSListenerProxy::StartCORSPreflight as a preflight channel (OPTIONS) ; it gets 301 response to a different origin (https://www.bing.com:443/) and gets canceled with NS_ERROR_DOM_BAD_URI 

I think this all is OK.  do you believe that a 'view-source:' context shouldn't be able to open other 'view-source:' requests?  I believe it should, since when you click a link in the source view, you go to view-source: of if again (modulo bug 1442784).
Attachment #8956142 - Flags: review?(dveditz)
(In reply to Honza Bambas (:mayhemer) from comment #2)
> It means, look into the cache, don't validate at all, but if it's not there, go
> to network.

Oh, I was thinking about the crash in the testcase and forgetting about the initial view-source page load. Yes of course you're right, in that case if it's not in the cache (disabled?) there could be a network load.

(In reply to Honza Bambas (:mayhemer) from comment #3)
> do you believe that a 'view-source:' context shouldn't be able to
> open other 'view-source:' requests?  I believe it should, since
> when you click a link in the source view, you go to view-source: [again]

You're right, I wasn't considering that case either. Navigation, sure, that's going to happen. The original testcase injected JS to do an XHR load from a view-source context so I was thinking this crash might be limited to sub-resource loads which I didn't think could normally happen in view-source and might not have been accounted for when the code was written.
Comment on attachment 8956142 [details] [diff] [review]
v1

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

r=dveditz
Attachment #8956142 - Flags: review?(dveditz) → review+
Should we unhide this bug then?  (if so, I have to update the commit message first)
Flags: needinfo?(dveditz)
Keywords: checkin-needed
slippery bi*ch :)
Keywords: checkin-needed
Yup, sounds good.
Group: network-core-security
Flags: needinfo?(dveditz)
Keywords: sec-low
Attachment #8956142 - Attachment is obsolete: true
Attachment #8956852 - Flags: review+
Keywords: checkin-needed
Whiteboard: [necko-triaged]
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/857cd5aebd4b
Add non-null checks around assignment of warning report to preflight HTTP channel in nsCORSListenerProxy::StartCORSPreflight. r=dveditz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/857cd5aebd4b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.