Assertions [Overwriting an existing document channel!] when opening pdf files

RESOLVED FIXED in Firefox 19

Status

()

P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: huzaifas, Assigned: bdahl)

Tracking

({sec-low})

19 Branch
Firefox 19
x86_64
Linux
sec-low
Points:
---

Firefox Tracking Flags

(firefox17 unaffected, firefox18- disabled, firefox19+ fixed, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [pdfjs-c-integration])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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

6 years ago
Reproducer available here:
http://huzaifas.fedorapeople.org/ff1.pdf
Trevor is this similar to bug 768243?
Flags: needinfo?(trev.saunders)
This is maybe more likely pdfjs misusing Gecko APIs than a bug in the core Gecko code... dunno.
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
(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

6 years ago
Should this be sec-critical similar to bug 768243 ?
(Assignee)

Updated

6 years ago
Priority: -- → P1
Whiteboard: [pdfjs-c-integration]
(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

6 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

6 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?
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.
> 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?
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

6 years ago
Created attachment 675142 [details] [diff] [review]
Forward the original request.

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 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+
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

6 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;
Yes, that should be fine.
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.
Marking sec-moderate per comment 20.
Keywords: sec-moderate
(Assignee)

Comment 22

6 years ago
Fixed by bug 810107.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Assignee: nobody → bdahl
Target Milestone: --- → Firefox 19
status-firefox-esr10: --- → unaffected
status-firefox17: --- → unaffected
status-firefox18: --- → affected
status-firefox19: --- → fixed
status-firefox-esr17: --- → unaffected
tracking-firefox19: --- → +
We should land this in Firefox 18
tracking-firefox18: --- → +
(In reply to Daniel Veditz [:dveditz] from comment #23)
> We should land this in Firefox 18

PDF.js isn't riding FF18, so untracking.
tracking-firefox18: + → -
status-b2g18: --- → affected
status-firefox18: affected → wontfix
tracking-b2g18: --- → 19+
(In reply to Brendan Dahl from comment #22)
> Fixed by bug 810107.

Brendan, can you please get a backport patch ready for b2g18 ?
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
Spoke offline to :dveditz & upon further investigation b2g is unaffected here.Hence untracking for b2g18.

Updated

6 years ago
status-b2g18: affected → unaffected
tracking-b2g18: 19+ → ---
(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?
Group: core-security
status-firefox18: wontfix → disabled
Keywords: sec-moderate → sec-low
You need to log in before you can comment on or make changes to this bug.