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

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect, P1)

Description

[Huzaifa Sidhpurwala]

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.

Comment 1

[Huzaifa Sidhpurwala]

Reproducer available here:
http://huzaifas.fedorapeople.org/ff1.pdf

Comment 2

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

Trevor is this similar to bug 768243?

Comment 3

[Daniel Veditz [:dveditz]]

This is maybe more likely pdfjs misusing Gecko APIs than a bug in the core Gecko code... dunno.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Trevor Saunders (:tbsaunde)]

(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

Comment 6

[Huzaifa Sidhpurwala]

Should this be sec-critical similar to bug 768243 ?

Comment 7

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

(In reply to Huzaifa Sidhpurwala from comment #6)
> Should this be sec-critical similar to bug 768243 ?

I think these two bugs are different.

Comment 8

[Huzaifa Sidhpurwala]

(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?

Comment 10

[Brendan Dahl [:bdahl]]

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

[Raymond Forbes[:rforbes]]

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

[Boris Zbarsky [:bzbarsky]]

> 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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Brendan Dahl [:bdahl]]

Attachment: 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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 675142 [details] [diff] [review]
Forward the original request.

Yep, this is exactly what I was thinking of.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Brendan Dahl [:bdahl]]

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

[Boris Zbarsky [:bzbarsky]]

Yes, that should be fine.

Comment 20

[Daniel Veditz [:dveditz]]

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.

Comment 21

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

Marking sec-moderate per comment 20.

Comment 22

[Brendan Dahl [:bdahl]]

Fixed by bug 810107.

Comment 23

[Daniel Veditz [:dveditz]]

We should land this in Firefox 18

Comment 24

[Alex Keybl [:akeybl]]

(In reply to Daniel Veditz [:dveditz] from comment #23)
> We should land this in Firefox 18

PDF.js isn't riding FF18, so untracking.

Comment 25

[bhavana bajaj [:bajaj]]

(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

[Yury Delendik (:yury)]

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

[bhavana bajaj [:bajaj]]

Spoke offline to :dveditz & upon further investigation b2g is unaffected here.Hence untracking for b2g18.

Comment 28

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

(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?