1242730 - Convert callsites to use channel.async0pen2() within browser/extensions/pdfjs

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1242730_asyncopen2_pdfjs.patch

Yury, similar as in Bug 1242731, we should convert callsites within pdfjs to use asyncOpen2. Similar questions:

1) Can we land that change on inbound, or do we need to file a github request?
2) The attached patch passes all tests within browser/extensions/pdfjs/test/, any other tests I need to run?
3) We could use SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS but also SEC_ALLOW_CROSS_ORIGIN_DATA_IS NULL. Usually when we use the systemPrincipal as the loadingPrincipal we rather lean towards SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL. Maybe we should do that in this case as well.

Let me know how you would like me to move forward! Thanks!

Comment 2

[Yury Delendik (:yury)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8711887 [details] [diff] [review]
> bug_1242730_asyncopen2_pdfjs.patch
> 
> Yury, similar as in Bug 1242731, we should convert callsites within pdfjs to
> use asyncOpen2. Similar questions:
> 
> 1) Can we land that change on inbound, or do we need to file a github
> request?

We need to file a PR and also provide a polyfill and workaround for older versions of FF. See e.g. https://github.com/mozilla/pdf.js/blob/master/extensions/firefox/content/PdfStreamConverter.jsm#L209


> 2) The attached patch passes all tests within
> browser/extensions/pdfjs/test/, any other tests I need to run?

mozcentral has integration tests, so no additional tests needed there. However when PR at github will be reviewed, reviewer will test the extension with release version of FF. (See above)

> 3) We could use SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS but also
> SEC_ALLOW_CROSS_ORIGIN_DATA_IS NULL. Usually when we use the systemPrincipal
> as the loadingPrincipal we rather lean towards
> SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL. Maybe we should do that in this case as
> well.

There are two usages of createNewChannel -- they might require different restrictions. We use data: to load @font-face, so I'm not quite sure it will affect the presentation. Could you explain the differences and how these restrictions will be applied for resource://pdf.js/web/viewer.html loaded page?

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1242730_asyncopen2_pdfjs.patch

Yury, you are right, there are two different use cases, so I suggest we are eliminating 'createNewChannel()' and inline the two newChannel functions.

1) When loading PDF_VIEWER_WEB_PAGE, that URI can not be modified by the web (it's defined as const above) hence using the systemPrincipal (loadUsingSystemPrincipal) is the right thing to do.
Just to make sure, we are loading a FONT here, right? In regards of using the correct contentPolicyType for newChannel().

2) I am not sure what we are loading when using 'blobUri' - can you provide some more details? Then we can find the right contentPolicyType and also securityFlags.

Comment 4

[Yury Delendik (:yury)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Created attachment 8714477 [details] [diff] [review]
> bug_1242730_asyncopen2_pdfjs.patch
> 
> Yury, you are right, there are two different use cases, so I suggest we are
> eliminating 'createNewChannel()' and inline the two newChannel functions.
> 
> 1) When loading PDF_VIEWER_WEB_PAGE, that URI can not be modified by the web
> (it's defined as const above) hence using the systemPrincipal
> (loadUsingSystemPrincipal) is the right thing to do.
> Just to make sure, we are loading a FONT here, right? In regards of using
> the correct contentPolicyType for newChannel().

Yes, we might load fonts, but we are loading that from data: schema. (I don't know if TYPE_FONT is right       contentPolicyType)

> 2) I am not sure what we are loading when using 'blobUri' - can you provide
> some more details? Then we can find the right contentPolicyType and also
> securityFlags.

We are pushing loaded binary data from PDF.js worker to the FF as downloaded file. I cannot provide more details about this part of the code, let's see if Brendan will.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Yury Delendik (:yury) from comment #4)
> > 1) When loading PDF_VIEWER_WEB_PAGE, that URI can not be modified by the web
> > (it's defined as const above) hence using the systemPrincipal
> > (loadUsingSystemPrincipal) is the right thing to do.
> > Just to make sure, we are loading a FONT here, right? In regards of using
> > the correct contentPolicyType for newChannel().
> 
> Yes, we might load fonts, but we are loading that from data: schema. (I
> don't know if TYPE_FONT is right       contentPolicyType)

We can stick to TYPE_OTHER in that case.

Comment 6

[Jonas Jenwald [:Snuffleupagus]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> [...] there are two different use cases, so I suggest we are
> eliminating 'createNewChannel()' and inline the two newChannel functions.

I'm not sure that will work, since upstream we're relying on `createNewChannel` in order for the addon to be usable with older Firefox versions, please see https://github.com/mozilla/pdf.js/blob/master/extensions/firefox/content/PdfStreamConverter.jsm#L182-L207.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Jenwald [:Snuffleupagus] from comment #6)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> > [...] there are two different use cases, so I suggest we are
> > eliminating 'createNewChannel()' and inline the two newChannel functions.
> 
> I'm not sure that will work, since upstream we're relying on
> `createNewChannel` in order for the addon to be usable with older Firefox
> versions, please see
> https://github.com/mozilla/pdf.js/blob/master/extensions/firefox/content/
> PdfStreamConverter.jsm#L182-L207.

That is potentially true. Not sure if that is really any issue though anymore, since the new API for NetUtil.newChannel() is backwards compatible. I think we only had that problem since this file was using NewChannel2, right? 

Yury, I am not the expert within pdfjs, any chance I can assign this bug to you? I can obviously provide all the guidance necessary from a security point of view.

Comment 8

[Brendan Dahl [:bdahl]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> 2) I am not sure what we are loading when using 'blobUri' - can you provide
> some more details? Then we can find the right contentPolicyType and also
> securityFlags.

What Yury said is accurate, when someone clicks the download button in pdf.js, we create a blob url that points to the pdf data which then gets sent up to the chrome code to read.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Yury, any chance I can convince you to take on the work within this bug?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Yury, any chance I can convince you to take on the work within this bug?

Re-pinging Yury. It would be awesome if you could take on the work within this bug. Can you?

Comment 11

[Yury Delendik (:yury)]

Attachment: bug1242730.patch

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8734439 [details] [diff] [review]
bug1242730.patch

Review of attachment 8734439 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Yuri for writing that patch. Looks pretty good to me!

::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm
@@ +255,5 @@
>      var docIsPrivate = this.isInPrivateBrowsing();
> +    var netChannel = NetUtil.newChannel({
> +      uri: blobUri,
> +      loadUsingSystemPrincipal: true
> +    });

I don't think this is quite what we want in this case. The blobURL comes from the web, right? In other words, it can be influenced by a webpage, hence I don't think using the systemPrincipal is appropritate. I would suggest the following instead:

var netChannel = NetUtil.newChannel({
  uri: blobUri,
  loadingNode: frontWindow.document,
  securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
  contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER
});

Are there any objections to using the frontWindow.document as the loadingNode instead of using the systemPrincipal?

@@ +282,5 @@
>            channel.contentDispositionFilename = filename;
>          }
>        } catch (e) {}
>        channel.setURI(originalUri);
> +      channel.loadInfo = netChannel.loadInfo;

This is a good catch. I have not realized that construct until now. Luckily there is only one other callsite in our tree that creates a channel using |createInstance(Ci.nsIInputStreamChannel);|. Thanks for pointing that out. Can you please add a comment on top. Something like:

Since we are creating the new channel using createInstance(), we have to manually propagate the loadInfo.

@@ +966,5 @@
>      // Create a new channel that is viewer loaded as a resource.
> +    var channel = NetUtil.newChannel({
> +      uri: PDF_VIEWER_WEB_PAGE,
> +      loadUsingSystemPrincipal: true
> +    });

Looks good, this matches old behavior.

@@ +1025,5 @@
>      var uri = NetUtil.newURI(PDF_VIEWER_WEB_PAGE, null, null);
>      var resourcePrincipal;
>      resourcePrincipal = ssm.createCodebasePrincipal(uri, {});
>      aRequest.owner = resourcePrincipal;
> +    channel.asyncOpen2(proxy);

That looks good. So instead of passing the aContext to asyncOpen() you now pass it within onStartRequest, onDataAvailable(), and onStopRequest().

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Since you are doing all the work, you should also get the credit :-) Assigning this bug to you.

Comment 14

[Yury Delendik (:yury)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> I don't think this is quite what we want in this case. The blobURL comes
> from the web, right? In other words, it can be influenced by a webpage,
> hence I don't think using the systemPrincipal is appropritate. I would
> suggest the following instead:
> 
> var netChannel = NetUtil.newChannel({
>   uri: blobUri,
>   loadingNode: frontWindow.document,
>   securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
>   contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER
> });
> 
> Are there any objections to using the frontWindow.document as the
> loadingNode instead of using the systemPrincipal?
> 

There is assert in nsBaseChannel::AsyncOpen at https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsBaseChannel.cpp#641

The URL provided by resource://pdf.js/viewer.html (in most of the cases it is blob) with intent to use it with "external-helper-app-service;1" to save it locally using doContent method. I'm trying to understand what security risk we are anticipating here.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Yury Delendik (:yury) from comment #14)
> There is assert in nsBaseChannel::AsyncOpen at
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsBaseChannel.
> cpp#641

Ah, I see that is the problem we are trying to fix within Bug 1257930.

> The URL provided by resource://pdf.js/viewer.html (in most of the cases it
> is blob) with intent to use it with "external-helper-app-service;1" to save
> it locally using doContent method. I'm trying to understand what security
> risk we are anticipating here.

The blob case seems fine to me. I was more worried about the other case where we really loading a provided URL which comes from the web. Using the systemPrincipal as the loadingPrincipal basically bypasses all security checks and might allow loading local files which is a security risk. If loading resource:// or chrome:// url can't happen, than we could use the Systemprincipal.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> (In reply to Yury Delendik (:yury) from comment #14)
> > There is assert in nsBaseChannel::AsyncOpen at
> > https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsBaseChannel.
> > cpp#641
> 
> Ah, I see that is the problem we are trying to fix within Bug 1257930.

Also if we use NetUtil.newChannel({..., loadusingSystemPrincipal: true}) that assertion wouldn't get triggered.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Yury, it seems this bug is already pretty close to being done. Any chance I could convince you to finish this one up?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Hey Yury, this bug is one of the only callsites left that still use .asyncOpen() instead of .asyncOpen2() [1]. Any updates?


[1] http://mxr.mozilla.org/mozilla-central/search?string=channel.asyncOpen%28

Comment 19

[Yury Delendik (:yury)]

No luck with recommendations in comment 15 and comment 16.

STR that toughing both those call sites (that I'm using to manually verify):

1. Open http://cdn.mozilla.net/pdfjs/tracemonkey.pdf
2. Check address bar and content/fonts of the opened document
3. Click "Download" toolbar button in the top-right
4a. Select "save as", and inspect size or content of the downloaded document
4b. If external viewer present, Select "open with" and ensure document is opened in the selected viewer

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Thanks Yury, as discussed over email, I'll have a look at what problems you run into in comment 19.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1242730_pdfjs.patch

Yury, reading through all of this bug again I think using the SystemPrincipal as the loadingPrincipal is the right thing to do in both of the cases. The uri: PDF_VIEWER_WEB_PAGE is hardcoded and can't be influenced by the webpage and also the blobURI is not influenced by the web page, hence we should just use the systemPricnipal as the loadingPrincipal.

(In reply to Yury Delendik (:yury) from comment #19)
> STR that toughing both those call sites (that I'm using to manually verify):
> 
> 1. Open http://cdn.mozilla.net/pdfjs/tracemonkey.pdf
> 2. Check address bar and content/fonts of the opened document
> 3. Click "Download" toolbar button in the top-right
> 4a. Select "save as", and inspect size or content of the downloaded document
> 4b. If external viewer present, Select "open with" and ensure document is
> opened in the selected viewer

I manually verified that both of these cases work correctly. Also all the tests within browser/extensions/pdfjs/test/ run to completion successfully.

I suppose we can land this patch - any objections?

Comment 22

[Yury Delendik (:yury)]

Comment on attachment 8757874 [details] [diff] [review]
bug_1242730_pdfjs.patch

Review of attachment 8757874 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Can you preserve createNewChannel and asyncFetchChannel? This functions are wraps pre-processor logic in the original code to provide legacy support for older FF browsers, see https://github.com/mozilla/pdf.js/blob/master/extensions/firefox/content/PdfStreamConverter.jsm#L188? If it's not hard, can you also submit the upstream patch as well?

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1242730_pdfjs.patch

Thanks Yury, preserved createNewChannel and asyncFetchChannel, carrying over r+.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6153baa6401d
Convert callsites to use channel.async0pen2() within browser/extensions/pdfjs. r=ydelendik

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6153baa6401d