Open Bug 1686200 Opened 4 months ago Updated 2 months ago

Investigate Principal for pdf.js

Categories

(Core :: DOM: Security, task, P2)

task

Tracking

()

ASSIGNED

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 obsolete file)

We should investigate if we are using the right principal in case pdf.js generates a blob. In particular, if you go to a foo.com, open a pdf, then we currently generate a principal of resource://pdf.js but I think we should generate a principal of foo.com.

This happens in URLMainThread::CreateObjectURL.

In turn, the new security machinery we are setting up within Bug 1671166 is also not super happy about receiving a resource principal.

FWIW, this can be reproduced using ./mach test --enable-fission toolkit/components/pdfjs/test/browser_pdfjs_saveas.js

Whiteboard: [domsecurity-active]

Hey Nika, what's your take on the problem explained above - I assume we should not rely on the current aGlobal for generating a principal within nsContentUtils::ObjectPrincipal(aGlobal.Get()). Because in the case we open a pdf in the content process of example.com then in my opinion we should generate a pricnipal of example.com and not resource://pdf.js which is what the current global is generated for.

What's your take? Do we need to loop in someone else?

Flags: needinfo?(nika)

Using example.com's principal to run pdf.js feels wrong. If we somehow exposed example.com to access the browsing context which has pdf.js running, example.com could get access to pdf.js internals. Even if that didn't lead to security issues, it would expose browser internal code to the web.

I chatted with baku, who introduced the principal within CreateObjectURL() - we agreed that we either use the Principal for aGlobal, or in case it's not http or https, then we rather fall back to using a NullPrincipal - which seems like the best path forward to me.

Flags: needinfo?(nika)

Doesn't the patch break stuff in chrome JS? Or if some sandbox JS tries to use the API etc.

Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5cdc1511d8ca
Use NullPrincipal for pdf.js in CreateObjectURL. r=baku

Backed out for causing multiple failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/93e245cd3f8fad4f8002a22306a556ac8bd59469

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunning%2Cpending%2Crunnable&searchStr=os%2Cx%2C10.14%2Cwebrender%2Cdebug%2Cmochitests%2Ctest-macosx1014-64-qr%2Fdebug-mochitest-browser-chrome-e10s%2Cbc2&revision=5cdc1511d8ca59ca1fa2866a0aefa614ad81b249&selectedTaskRun=BC3AVubNQ3W31quXVMZD-Q.0

Failure log mochitest: https://treeherder.mozilla.org/logviewer?job_id=326582755&repo=autoland&lineNumber=3502

Failure log wpt: https://treeherder.mozilla.org/logviewer?job_id=326582770&repo=autoland&lineNumber=2431

Failure log mochitest withou e10s: https://treeherder.mozilla.org/logviewer?job_id=326587546&repo=autoland&lineNumber=25045

Failure log reftest: https://treeherder.mozilla.org/logviewer?job_id=326587603&repo=autoland&lineNumber=7869

"INFO - TEST-PASS | browser/base/content/test/contextMenu/browser_view_image.js | window - checking if popup is closed -
[task 2021-01-13T12:59:38.056Z] 12:59:38 INFO - window - Popup Shown
[task 2021-01-13T12:59:38.056Z] 12:59:38 INFO - Buffered messages finished
[task 2021-01-13T12:59:38.056Z] 12:59:38 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/contextMenu/browser_view_image.js | Uncaught exception - undefined - timed out after 50 tries.
[task 2021-01-13T12:59:38.056Z] 12:59:38 INFO - Leaving test bound test_view_image_canvas_works
[task 2021-01-13T12:59:38.057Z] 12:59:38 INFO - Entering test bound test_view_image_revoked_cached_blob
[task 2021-01-13T12:59:38.057Z] 12:59:38 INFO - GECKO(1570) | [Child 1581: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x122b49800 == 1 [pid = 1581] [id = 0]
[task 2021-01-13T12:59:38.057Z] 12:59:38 INFO - GECKO(1570) | [Child 1581: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 1 (0x10e221e40) [pid = 1581] [serial = 1] [outer = 0x0]
[task 2021-01-13T12:59:38.057Z] 12:59:38 INFO - GECKO(1570) | [Child 1581: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 2 (0x122be2400) [pid = 1581] [serial = 2] [outer = 0x10e221e40]
[task 2021-01-13T12:59:38.126Z] 12:59:38 INFO - GECKO(1570) | [Child 1581, Main Thread] WARNING: NS_ENSURE_TRUE(info) failed: file /builds/worker/checkouts/gecko/extensions/permissions/PermissionDelegateHandler.cpp:348
[task 2021-01-13T12:59:38.126Z] 12:59:38 INFO - GECKO(1570) | [Child 1581: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 3 (0x122c9a400) [pid = 1581] [serial = 3] [outer = 0x10e221e40]
[task 2021-01-13T12:59:38.126Z] 12:59:38 INFO - GECKO(1570) | [1591, MainThread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp:202
[task 2021-01-13T12:59:38.126Z] 12:59:38 INFO - GECKO(1570) | [1591, MainThread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp:202
[task 2021-01-13T12:59:38.127Z] 12:59:38 INFO - GECKO(1570) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /var/folders/_k/_56f7pss1qz0rn7n331b45r0000017/T/tmpJSxf5Q.mozrunner/runtests_leaks_tab_pid1591.log
[task 2021-01-13T12:59:38.127Z] 12:59:38 INFO - GECKO(1570) | [1591, MainThread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp:202
[task 2021-01-13T12:59:38.127Z] 12:59:38 INFO - GECKO(1570) | [1591, MainThread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp:202
[task 2021-01-13T12:59:38.127Z] 12:59:38 INFO - GECKO(1570) | [1591, Main Thread] WARNING: XPCOM_MEM_BLOAT_LOG is set, disabling native allocations.: file /builds/worker/checkouts/gecko/tools/profiler/core/platform.cpp:251
[task 2021-01-13T12:59:38.571Z] 12:59:38 INFO - TEST-PASS | browser/base/content/test/contextMenu/browser_view_image.js | tab - checking if popup is closed - "

Flags: needinfo?(ckerschb)

Sorry, so probably smaug was right - I'll take a look.

Flags: needinfo?(ckerschb)

Chatted with :smaug and the better solution would be to find out what principal we use for pdf.js in general. I'll dig that out!

Summary: Investigate Principal for pdf.js when generating a blob → Investigate Principal for pdf.js

Hey :smaug, as discussed, I investigated if using a NullPrincipal for pdf.js current global would work. Unfortunately not really, if I change the currently created ContentPricnipal of type resource://pdf.js to a NullPrincipal here in PdfStreamConveter, then I am getting security errors a la:

[JavaScript Error: "Security Error: Content at moz-nullprincipal:{e406a142-27d0-4b00-adb0-2f9fbce759a0} may not load or link to resource://pdf.js/build/pdf.js."]
[JavaScript Error: "Security Error: Content at moz-nullprincipal:{e406a142-27d0-4b00-adb0-2f9fbce759a0} may not load or link to resource://pdf.js/web/viewer.css."]
[JavaScript Error: "Security Error: Content at moz-nullprincipal:{e406a142-27d0-4b00-adb0-2f9fbce759a0} may not load or link to
resource://pdf.js/web/viewer.js."]
[JavaScript Error: "Security Error: Content at moz-nullprincipal:{e406a142-27d0-4b00-adb0-2f9fbce759a0} may not load or link to resource://pdf.js/build/pdf.js."]

That obviously makes sense because a NullPrincipal should not be allowed to load internal resources.

Question being, how do we move forward here? I know sub-optimal, but should we go ahead and land the patch attached to this bug? What's your take?

Flags: needinfo?(bugs)

Is isn't still clear to me who is calling URLMainThread::CreateObjectURL and why using null principal in that case would help.
And help with what? Why is resource://* principal problematic?

Flags: needinfo?(bugs)

(In reply to Olli Pettay [:smaug] from comment #13)

Is isn't still clear to me who is calling URLMainThread::CreateObjectURL and why using null principal in that case would help.
And help with what? Why is resource://* principal problematic?

@smaug: Using the resource:// principal would lead to allowing those principals to come from a child process, which is a site-isolation bug.
Leaking those capabilities to could lead to cross-origin leaks in our site-isolation architecture.
Internal message passing we have in PDF.js as well as funky pdf features (embedded PDFs, form submission?) have not been audited for this, so the severity is a bit unclear (see also your very own considerations around this in comment 3).

As a temporary workaround, could we try and figure out an early sign in the parent process that a child process will instantiate pdf.js and use that information in our vetting code to allow exceptions for these processes? That wouldn't resolve the security issue, but would allow us for more granular, iterative process. @ckerschb, wdyt?

Is the idea to ensure resource:// principals aren't ever created in a child process? Created or used?
Naturally just loading any resource url to a tab causes resource:// principals to be used in that tab currently.
And pdf.js itself uses resource:// for running in a child process.
So which case are we exactly trying to fix? Is this only about ObjectURLs?

Attachment #9196798 - Attachment is obsolete: true
See Also: → 1667965
See Also: → 1699658
You need to log in before you can comment on or make changes to this bug.