Closed Bug 1280339 Opened 8 years ago Closed 6 years ago

Unprivileged content can open resource: URIs via PDF.js

Categories

(Firefox :: PDF Viewer, defect)

Product:
Firefox
Component:
PDF Viewer
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52

People

(Reporter: arminius, Unassigned)

References

Details

(Keywords: csectype-sop, sec-low, Whiteboard: [pdfjs-c-integration]) 

Unprivileged pages can redirect to resource: URIs from the PDF.js viewer without user interaction by specifying a "Refresh" response header.

While the DOM of a PDF.js view is not accessible by untrusted code, a server can still provide HTTP headers that affect the document. If the server sets a "Refresh" header, the redirection URI will be checked against the principal of the loaded document. As with PDF.js, this document can have additional privileges.

https://dxr.mozilla.org/mozilla-central/rev/14c5bf11d37b9e92d27f7089d9392de2ac339bb3/dom/base/nsDocument.cpp#3760
https://dxr.mozilla.org/mozilla-central/rev/14c5bf11d37b9e92d27f7089d9392de2ac339bb3/docshell/base/nsDocShell.cpp#6970

Example header:
Refresh: 0;URL=resource://pdf.js/web/viewer.html

Proof of Concept:
$ echo "HTTP/1.1 200\nContent-type: application/pdf\nRefresh: 0;URL=resource:///chrome.manifest\nContent-length: 0\n\n" | nc -l -p 8080
(Visit 127.0.0.1:8080 to get redrected to resource:///chrome.manifest)
Yury, can pdfjs prevent this redirect from going through netwerk or can we ensure that we provide the correct principal here?
Flags: needinfo?(ydelendik)
"resource:" principal is correct one (mostly to maintain app sandbox). We can remove the Refresh header for the viewer thus prevent from redirecting.

Except ability of redirection to the different "resource://"-hosted file, what other problems can it raise? (This will help to triage the priority of the bug)
Flags: needinfo?(ydelendik)
(In reply to Yury Delendik (:yury) from comment #2)
> "resource:" principal is correct one (mostly to maintain app sandbox). We
> can remove the Refresh header for the viewer thus prevent from redirecting.

I don't think it's the correct principal for evaluating whether the PDF page can link/redirect to something. The correct principal would be that of the PDF file.

> Except ability of redirection to the different "resource://"-hosted file,
> what other problems can it raise? (This will help to triage the priority of
> the bug)

Depends what resource: can link to. We've locked this down quite a bit somewhat recently, but even just 'resource' would let you read pretty much any other files in the app dir. Note that I'm not sure if this issue as reported, by itself, gives you access to the *contents* of the URL in question (e.g. if loaded in a window/frame the attacker has opened).

More generically, this kind of "protocol hopping" has been problematic in the past, so I guess I'm just worried about letting a webpage get to 'resource' to begin with.
(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Yury Delendik (:yury) from comment #2)
> > "resource:" principal is correct one (mostly to maintain app sandbox). We
> > can remove the Refresh header for the viewer thus prevent from redirecting.
> 
> I don't think it's the correct principal for evaluating whether the PDF page
> can link/redirect to something. The correct principal would be that of the
> PDF file.

I agree, but that's the only solution atm that allows us to preserve URL and sandbox PDF viewer. We raised this concern before and we are open to accept better solution if it exists.

Alternative is blocked by implementation of jsplugins (Bug 944929 and bug 1044769) -- this will allow us to remove stream converter and resource principal usage. As I understand this work stopped in favor of flapper.js.
 
> > Except ability of redirection to the different "resource://"-hosted file,
> > what other problems can it raise? (This will help to triage the priority of
> > the bug)
> 
> Depends what resource: can link to. We've locked this down quite a bit
> somewhat recently, but even just 'resource' would let you read pretty much
> any other files in the app dir. Note that I'm not sure if this issue as
> reported, by itself, gives you access to the *contents* of the URL in
> question (e.g. if loaded in a window/frame the attacker has opened).
> 
> More generically, this kind of "protocol hopping" has been problematic in
> the past, so I guess I'm just worried about letting a webpage get to
> 'resource' to begin with.

I understand this and I would like to know more details about if the redirect gives any access to other resources (e.g. file, chrome, data, etc.) without speculating about generic case.
More details for triage: web viewer is opened via stream converter at resource://pdf.js/web/viewer.html and uses resource principal. Looks like HTTP headers that are meta HTML attributes from original request are got applied to the viewer's HTML document. So far there is no evidence that "Refresh" header can redirect to other resources than at "resource://".
(In reply to Yury Delendik (:yury) from comment #4)
> I understand this and I would like to know more details about if the
> redirect gives any access to other resources (e.g. file, chrome, data, etc.)
> without speculating about generic case.

As I see it, the most sensitive protocol you can link to is resource:. You can redirect to data: URIs but the resulting page does not inherit any permissions. Also, javascript: URIs are explicitly forbidden.

So this bug lets you link to but not access the content of resource: pages. It's a stepping stone that an attacker would have to chain with another bug (e.g. an XSS flaw on any document in the resource directory).

(In reply to Yury Delendik (:yury) from comment #2)
> "resource:" principal is correct one (mostly to maintain app sandbox). We
> can remove the Refresh header for the viewer thus prevent from redirecting.

I'd favor checking against the principal of the hosting page over disabling the header explicitly for PDF.js since I'd assume that this way the bug could reappear with implementation of a similar embedded viewer.
Why do you think resource: is sensitive? All it can do is reference files installed with Firefox which are known to everyone (and also add-on resources which is a fingerprinting risk, but that's covered in another bug).
Flags: needinfo?(armin)
(In reply to Daniel Veditz [:dveditz] from comment #7)
> Why do you think resource: is sensitive? All it can do is reference files
> installed with Firefox which are known to everyone (and also add-on
> resources which is a fingerprinting risk, but that's covered in another bug).

Well, we have had a number of escalation bugs from resource:// principals in the past. In theory it's unprivileged, but we should take any escalation that allows untrusted code to be run in resource:// context very seriously.
(In reply to Bobby Holley (busy) from comment #8)
> (In reply to Daniel Veditz [:dveditz] from comment #7)
> > Why do you think resource: is sensitive? All it can do is reference files
> > installed with Firefox which are known to everyone (and also add-on
> > resources which is a fingerprinting risk, but that's covered in another bug).
> 
> Well, we have had a number of escalation bugs from resource:// principals in
> the past. In theory it's unprivileged, but we should take any escalation
> that allows untrusted code to be run in resource:// context very seriously.

I would also take anything that allows fingerprinting of installed add ons as seriously as a preexisting sec-high at least.  From that it only takes profiling the most widely used extensions / add ons to find bugs in them.  That would put a large percent of the end users at risk.

We all know authors in this case tend to do things that aren't thought out well in terms of security.  They also tend to rely on things we would consider bugs as accepted ways to do things.

Just my two cents.
(In reply to Yury Delendik (:yury) from comment #5)
> More details for triage: web viewer is opened via stream converter at
> resource://pdf.js/web/viewer.html and uses resource principal. Looks like
> HTTP headers that are meta HTML attributes from original request are got
> applied to the viewer's HTML document. So far there is no evidence that
> "Refresh" header can redirect to other resources than at "resource://".

This could in turn allow one to possibly create a blob or use a data URI that would then have the same privileges as code running from resource://.

That could allow someone to possibly even launch addons which is worrisome to me(see my previous comment).  I'll try to get testing and confirming this today.
(In reply to Jay Gilbert from comment #9)
> I would also take anything that allows fingerprinting of installed add ons
> as seriously as a preexisting sec-high at least.

That is a known problem being tracked in other bugs. This bug should stick to what is uniquely exposed in this case.
(In reply to Daniel Veditz [:dveditz] from comment #7)
> Why do you think resource: is sensitive? All it can do is reference files
> installed with Firefox which are known to everyone (and also add-on
> resources which is a fingerprinting risk, but that's covered in another bug).

Some resource: documents have additional privileges. For instance, every document in the resource://pdf.js/ path adopts the "pdf.js" domain name and thus is able to read PDF documents from any origin. I am not suggesting this is a severe bug but there are attacks where linking to a resource: URL could come in handy.
Flags: needinfo?(armin)
Can we just release a separate patch (unrelated?) to this bug that will remove "Refresh" header in the PDF viewer's stream converter so it will start riding trains until we decide about severity of this particular threat? If we will find proof that the attack potentially can exist we can uplift the patch.

The patch is one-liner, it will add removal of "Refresh" to the headers removal we already perform for CSP at https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#947
(In reply to Daniel Veditz [:dveditz] from comment #11)
> (In reply to Jay Gilbert from comment #9)
> > I would also take anything that allows fingerprinting of installed add ons
> > as seriously as a preexisting sec-high at least.
> 
> That is a known problem being tracked in other bugs. This bug should stick
> to what is uniquely exposed in this case.

That's just my general opinion, nothing in particular about any bug(s).  BTW Dan can you cc me on the bug that you are talking about?

I agree to keep this separate, and I have a new nightly build with all the release builds ready but just haven't got to digging into this yet.

I do like my pdfjs bugs though, so it wont be long.
Whiteboard: [pdfjs-c-integration]
I'll get to this today if possible I know I'm slacking here, but have had other avenues I've been pursuing(I'll try to file some bugs.)

I once had the idea to try something like this, but never got there.  I built a recent nightly build last night so I'll use that to test this.  I bet it can get much worse with the right small tricks.

More to come.
So far we're not coming up with an actual attack here -- once you redirect to a new file the user sees it, but how does the original document author get a copy sent back? We're not running author scripts in the PDF, are we?
Keywords: csectype-sop, sec-low
(In reply to Yury Delendik (:yury) from comment #13)
> Can we just release a separate patch (unrelated?) to this bug that will
> remove "Refresh" header in the PDF viewer's stream converter so it will
> start riding trains until we decide about severity of this particular
> threat? If we will find proof that the attack potentially can exist we can
> uplift the patch.

That would be great. is there another bug for that so this bug can depend on it?
Flags: needinfo?(ydelendik)
(In reply to Daniel Veditz [:dveditz] from comment #17)
> (In reply to Yury Delendik (:yury) from comment #13)
> > Can we just release a separate patch (unrelated?) to this bug that will
> > remove "Refresh" header in the PDF viewer's stream converter so it will
> > start riding trains until we decide about severity of this particular
> > threat? If we will find proof that the attack potentially can exist we can
> > uplift the patch.
> 
> That would be great. is there another bug for that so this bug can depend on
> it?

Landed with bug 1303228 -- target is FF51
Flags: needinfo?(ydelendik)
Is this bug still relevant?
Flags: needinfo?(ydelendik)
Flags: needinfo?(dveditz)
Looks fixed to me
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Depends on: 1303228
Flags: needinfo?(dveditz)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Group: firefox-core-security → core-security-release
Group: core-security-release
Flags: needinfo?(ydelendik)
You need to log in before you can comment on or make changes to this bug.