Closed
Bug 802050
Opened 12 years ago
Closed 12 years ago
Assertions [Overwriting an existing document channel!] when opening pdf files
Categories
(Firefox :: PDF Viewer, defect, P1)
Tracking
()
RESOLVED
FIXED
Firefox 19
Tracking | Status | |
---|---|---|
firefox17 | --- | unaffected |
firefox18 | - | disabled |
firefox19 | + | fixed |
firefox-esr10 | --- | unaffected |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: huzaifas, Assigned: bdahl)
Details
(Keywords: sec-low, Whiteboard: [pdfjs-c-integration])
Attachments
(1 file)
3.33 KB,
patch
|
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
I am using 19.0a1 (2012-10-15) ASAN debug build. Fuzzing pdfs, when i noticed a couple of assertions when opening a pdf file (pdfjs) ###!!! ASSERTION: must have binding parent when in native anonymous subtree with a parent node: '!IsInNativeAnonymousSubtree() || GetBindingParent() || !GetParent()', file ../../../dist/include/nsIContent.h, line 236 ###!!! ASSERTION: Overwriting an existing document channel!: '(loadFlags & nsIChannel::LOAD_REPLACE) || !(mDocumentRequest.get())', file /builds/slave/try-lnx64-dbg/build/uriloader/base/nsDocLoader.cpp, line 536 ###!!! ASSERTION: Adopting child!: 'Error', file /builds/slave/try-lnx64-dbg/build/accessible/src/generic/Accessible.cpp, line 2463 Not sure if this a security flaw, but i spoke with dveditz and decided it was best to file security, could be opened up later.
Reporter | ||
Comment 1•12 years ago
|
||
Reproducer available here: http://huzaifas.fedorapeople.org/ff1.pdf
Comment 3•12 years ago
|
||
This is maybe more likely pdfjs misusing Gecko APIs than a bug in the core Gecko code... dunno.
Comment 4•12 years ago
|
||
So the JS stack here is: (gdb) jsstack 0 anonymous() ["resource://pdf.js.components/PdfStreamConverter.js":532] this = [object Object] That line is in PdfStreamConverter.prototype.onStartRequest and does this: 532 listener.onStartRequest.apply(listener, arguments); In particular, the way we get there is that nsBaseChannel::OnStartRequest calls the JS-implemented onStartRequest which then calls nsDocumentOpenInfo::OnStartRequest. At this point the mDocumentRequest on the docshell is the HTTP channel, but the request passed to OnStartRequest from JS is a file channel. so yeah, this is bogus code in the stream converter. Stream converters should just be converting the _data_, but passing through the underlying channel, typically. But if you _do_ want to use a different channel, you need to set LOAD_REPLACE on it, like the multipart converter does.
Component: Security → PDF Viewer
Product: Core → Firefox
Comment 5•12 years ago
|
||
(In reply to David Bolter [:davidb] from comment #2) > Trevor is this similar to bug 768243? I'd guess the a11y assert there is bug 768243 yes
Flags: needinfo?(trev.saunders)
Reporter | ||
Comment 6•12 years ago
|
||
Should this be sec-critical similar to bug 768243 ?
Assignee | ||
Updated•12 years ago
|
Priority: -- → P1
Whiteboard: [pdfjs-c-integration]
Comment 7•12 years ago
|
||
(In reply to Huzaifa Sidhpurwala from comment #6) > Should this be sec-critical similar to bug 768243 ? I think these two bugs are different.
Reporter | ||
Comment 8•12 years ago
|
||
(In reply to David Bolter [:davidb] from comment #7) > (In reply to Huzaifa Sidhpurwala from comment #6) > > Should this be sec-critical similar to bug 768243 ? > > I think these two bugs are different. Thanks, should this bug still be security-sensitive?
Assignee | ||
Comment 10•12 years ago
|
||
This may be a misuse of gecko, but as far as I can tell this doesn't need to be security sensitive. When a pdf is opened our stream converter get's notified of channel. We keep that channel open to keep loading the data. We then open our own channel to a resource url that has the pdf.js viewer and use that to replace the channel to show our viewer. We then pipe the data from the original channel into our viewer. (In reply to Boris Zbarsky (:bz) from comment #4) > But if you _do_ want to use a different channel, you > need to set LOAD_REPLACE on it, like the multipart converter does. If we use LOAD_REPLACE the url in the url bar get's replaced with the url of the pdf.js viewer, whereas we want to keep the url of the pdf. Is there some other way that we could 1) keep the url the same, 2) keep the original pdf request alive and not trigger any assertions?
Comment 11•12 years ago
|
||
so, sounds like this isn't a vuln. I will give it a few days. if it seems quiet or we get agreement I will open up the bug and non-qual the bounty nomination.
Comment 12•12 years ago
|
||
> If we use LOAD_REPLACE the url in the url bar get's replaced with the url of the pdf.js
> viewer
Yes, ok.
But why can you not pass the original channel as the request then, with your data?
Basically, I can't guarantee that the way you're violating invariants here is not a security problem. Have you done any sort of code audit to check whether it might be?
Comment 13•12 years ago
|
||
Note that it's quite possible that there is no problem here too; someone just needs to check it and see whether there is. And it not, then we should find a way for you to do what you need to do here without asserting.
Assignee | ||
Comment 14•12 years ago
|
||
bz: The above patch doesn't trigger the assertion anymore. I just need some quick feedback if this is what you had in mind.
Attachment #675142 -
Flags: feedback?(bzbarsky)
Comment 15•12 years ago
|
||
Comment on attachment 675142 [details] [diff] [review] Forward the original request. Yep, this is exactly what I was thinking of.
Attachment #675142 -
Flags: feedback?(bzbarsky) → feedback+
Comment 16•12 years ago
|
||
Note that it might have other benefits too. For example, if the original HTTP channel had a content-disposition filename, I think with this change we might end up using it when you "save as" if we're not doing it already.
Assignee | ||
Comment 17•12 years ago
|
||
bz: We've noticed one issue with the patch I added, it changes the behavior of our viewer so that it runs in the context of the original PDF url instead of our resource url of our viewer. This can be fixed by changing the original channels owner. This doesn't throw any exceptions, but I just wanted to make sure this is okay: - channel.owner = resourcePrincipal; + aRequest.owner = resourcePrincipal;
Comment 18•12 years ago
|
||
Yes, that should be fine.
Comment 20•12 years ago
|
||
Huzaifas: thanks for fuzzing pdfjs and reporting this issue. Although this assertion can indicate a problem it does not appear to be exploitable in this case.
Assignee | ||
Comment 22•12 years ago
|
||
Fixed by bug 810107.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Assignee: nobody → bdahl
Target Milestone: --- → Firefox 19
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox17:
--- → unaffected
status-firefox18:
--- → affected
status-firefox19:
--- → fixed
status-firefox-esr17:
--- → unaffected
tracking-firefox19:
--- → +
Comment 24•12 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #23) > We should land this in Firefox 18 PDF.js isn't riding FF18, so untracking.
Updated•11 years ago
|
Comment 25•11 years ago
|
||
(In reply to Brendan Dahl from comment #22) > Fixed by bug 810107. Brendan, can you please get a backport patch ready for b2g18 ?
Comment 26•11 years ago
|
||
Daniel, Is there a reason why we are tracking it for b2g? I see the code is present in b2g18 [1], but as far as I know it's never executed on b2g device as an extension -- pdfjs gaia app is used instead [2] [1] http://mxr.mozilla.org/mozilla-b2g18/ [2] https://github.com/mozilla-b2g/gaia/tree/master/apps/pdfjs
Comment 27•11 years ago
|
||
Spoke offline to :dveditz & upon further investigation b2g is unaffected here.Hence untracking for b2g18.
Updated•11 years ago
|
tracking-b2g18:
19+ → ---
Comment 28•11 years ago
|
||
(In reply to Alex Keybl [:akeybl] from comment #24) > (In reply to Daniel Veditz [:dveditz] from comment #23) > > We should land this in Firefox 18 > > PDF.js isn't riding FF18, so untracking. Then why is Firefox 18 marked as affected here?
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•