Opened 5 years ago

Closed 4 years ago

#13439 closed defect (fixed)

Inspector raises the canvas prompt when hovering over images

Reported by: dcf Owned by: tbb-team
Priority: Low Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-easy, tbb-usability, TorBrowserTeam201412R
Cc: mcs, brade, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

  1. Open any old page with an image, for instance https://blog.torproject.org/
  2. Press Ctrl+Shift+I to open the Inspector.
  3. Click the "Pick an element from the page" icon, the one that looks like "Pick an element from the page" icon..
  4. Hover over an img element.

The "attempted to extract HTML5 canvas image data" prompt appears.

Child Tickets

Attachments (3)

pick.png (406 bytes) - added by dcf 5 years ago.
"Pick an element from the page" icon.
exempt_resources_from_canvas_prompt.patch (1.3 KB) - added by gacar 4 years ago.
Patch for exempting resource URLs from canvas prompt
canvas_exempt.patch (2.7 KB) - added by gacar 4 years ago.
Exempt chrome callers and PDF.js from canvas prompt

Download all attachments as: .zip

Change History (22)

Changed 5 years ago by dcf

Attachment: pick.png added

"Pick an element from the page" icon.

comment:1 Changed 5 years ago by gacar

The code that triggers the canvas prompt should be this:
http://mxr.mozilla.org/mozilla-esr31/source/toolkit/devtools/server/actors/inspector.js#2757

Apparently this is not treated as a chrome script (which are exempt from the canvas prompts.) I've no idea if this is something expected or not.

comment:2 Changed 4 years ago by CRhode

Would previewing *.pdf files in the Firefox Document Viewer raise this condition, too? That's what I'm seeing. TOR complains that my Website "attempted to extract HTML5 canvas image data."

comment:3 in reply to:  2 Changed 4 years ago by dcf

Replying to CRhode:

Would previewing *.pdf files in the Firefox Document Viewer raise this condition, too? That's what I'm seeing. TOR complains that my Website "attempted to extract HTML5 canvas image data."

I saw the warning with PDF files in 3.6 too. I think the warning on image hover is new with 4.0.

comment:4 Changed 4 years ago by gk

Cc: mcs brade added
Keywords: tbb-easy tbb-usability added; canvas tbb-4.0 removed

The problem is that we don't check whether a content or a chrome script is trying to extract data. As we are extracting the script file anyway we probably could either check for a scheme indicating a chrome script (like (resource://) or maybe there is even a better method giving back a yes/no if asked whether that is a content script or not. I have not looked closer at it yet.

The Inspector raises the issue in ESR 31 (and not earlier) as it had not been using canvas extraction in ESR 24.

Changed 4 years ago by gacar

Patch for exempting resource URLs from canvas prompt

comment:5 Changed 4 years ago by gacar

There was already a check for exempting file:// URLs from canvas prompt in `IsImageExtractionAllowed()`. The above patch adds resource:// URLs to this exemption.

The patch should prevent prompts due to the Inspector and PDF.js, but I haven't had the chance to test it.

I also agree that having this check as a separate function (e.g. IsURLExemptFromCanvas()) is a better idea.

comment:6 in reply to:  5 ; Changed 4 years ago by gk

Replying to gacar:

There was already a check for exempting file:// URLs from canvas prompt in `IsImageExtractionAllowed()`. The above patch adds resource:// URLs to this exemption.

The patch should prevent prompts due to the Inspector and PDF.js, but I haven't had the chance to test it.

gacar: No, that does not fix the bug. The problem is not the scheme of the document but rather whether the script that uses the canvas is a content or a chrome script. The patch we have assumes there are only content scripts doing that and thus just checks whether the document/website loaded is a chrome one or not. If not and if there is no permission to use the canvas yet you'll see the prompt. But apparently there are cases where we have a content document + a chrome script which should not result in the canvas warning. Thus, looking at the code you want to check for resource:// in scriptFile or maybe there is even a better method than DescribeScriptedCaller() which would just give us a "yes/no" back if asked whether we have a chrome script or a content script in the respective situation.

comment:7 in reply to:  6 Changed 4 years ago by mcs

Replying to gk:

gacar: No, that does not fix the bug. The problem is not the scheme of the document but rather whether the script that uses the canvas is a content or a chrome script. The patch we have assumes there are only content scripts doing that and thus just checks whether the document/website loaded is a chrome one or not.

I have not had a chance to debug this problem yet, but the intent of the patch was to check the security characteristics of the running script. Either the check is incorrect or PDF.js and the developer tools inspector do not run with system privileges.

comment:8 Changed 4 years ago by gacar

I debugged a bit to check the values returned by the IsSystemPrincipal for dev tools and the PDF.js.

I also checked whether nsContentUtils::IsCallerChrome could be useful here.

Method/Context PDF.js Inspector
IsSystemPrincipal False False
nsContentUtils::IsCallerChromeFalse True

So we could filter out Inspector prompts by using IsCallerChrome in addition to existing check with the IsSystemPrincipal.

Not sure what do do with the PDF.js though.

comment:9 Changed 4 years ago by gacar

According to PDF.js FAQ and this comment, most of the PDF.js code runs with content privileges.

So, adding a IsCallerChrome check would work for the Inspector, but not for the PDF.js.

Can whitelisting resource://pdf.js by scheme/URL be abused for fingerprinting? If we cannot think of a way, fixing this could help with false positives and related alert fatigue.

If you like the approach (exempt chrome callers with IsCallerChrome and whitelist PDF.js via scheme/URL whitelist) I could submit a new patch.

See, also #10570.

comment:10 in reply to:  9 ; Changed 4 years ago by mcs

Replying to gacar:

According to PDF.js FAQ and this comment, most of the PDF.js code runs with content privileges.

So, adding a IsCallerChrome check would work for the Inspector, but not for the PDF.js.

Can whitelisting resource://pdf.js by scheme/URL be abused for fingerprinting? If we cannot think of a way, fixing this could help with false positives and related alert fatigue.

If you like the approach (exempt chrome callers with IsCallerChrome and whitelist PDF.js via scheme/URL whitelist) I could submit a new patch.

Yes, please. This sounds like a good approach to me. I am not sure exactly what the pdf.js whitelisting test needs to look like; there are a bunch of files under browser/extensions/pdfjs/ so maybe we need a prefix test? Or we need to figure out which file or files access canvas.

comment:11 in reply to:  10 ; Changed 4 years ago by gacar

I just uploaded a new patch. This should allow chrome scripts and PDF.js to access canvas without the prompt.

I built and manually tested patched Tor Browser on a handful of canvas enabled sites, it worked as expected.

The only URL to be whitelisted for PDF.js turns out to be resource://pdf.js/build/pdf.js. There is a second script (pdf.worker.js), but it never shows up as a caller, probably because it's loaded by the first one.

Let me know if anything is wrong with the patch.

comment:12 Changed 4 years ago by gk

Keywords: TorBrowserTeam201412R added
Status: newneeds_review

comment:13 in reply to:  11 Changed 4 years ago by mcs

Replying to gacar:

I just uploaded a new patch. This should allow chrome scripts and PDF.js to access canvas without the prompt.

Thank you! The patch looks good.
gk -- if you agree, please land this so it is included in the next 4.5 release.

comment:14 Changed 4 years ago by mcs

Cc: gk added

comment:15 Changed 4 years ago by gk

gacar: thanks. One question: Why do you have

JS::DescribeScriptedCaller(aCx, &scriptFile, &scriptLine)

wrapped in an if-clause? Reading the code this means to me this call can fail but *not* for
third-party content as you are assuming scriptFile.get() and scriptLine are available unconditionally in this scenario. If that is correct which failing scenarios did you have in mind?

mcs: you have the merge power, too, now ;)

comment:16 in reply to:  15 ; Changed 4 years ago by gacar

Replying to gk:

gacar: thanks. One question: Why do you have

JS::DescribeScriptedCaller(aCx, &scriptFile, &scriptLine)

wrapped in an if-clause? Reading the code this means to me this call can fail but *not* for
third-party content as you are assuming scriptFile.get() and scriptLine are available unconditionally in this scenario. If that is correct which failing scenarios did you have in mind?

I didn't think about any specific scenario but just tried to be cautious.

When DescribeScriptedCaller returns false, we're sure that neither scriptFile nor scriptLine is updated with the caller script's URL and line no: http://mxr.mozilla.org/mozilla-esr31/source/js/src/jsapi.cpp#6259

So there was no reason to continue with the strcmp, when scriptFile doesn't hold the real script URL.

Does that make sense?

comment:17 in reply to:  16 ; Changed 4 years ago by gk

Replying to gacar:

Replying to gk:

gacar: thanks. One question: Why do you have

JS::DescribeScriptedCaller(aCx, &scriptFile, &scriptLine)

wrapped in an if-clause? Reading the code this means to me this call can fail but *not* for
third-party content as you are assuming scriptFile.get() and scriptLine are available unconditionally in this scenario. If that is correct which failing scenarios did you have in mind?

I didn't think about any specific scenario but just tried to be cautious.

Ok, good. :)

When DescribeScriptedCaller returns false, we're sure that neither scriptFile nor scriptLine is updated with the caller script's URL and line no: http://mxr.mozilla.org/mozilla-esr31/source/js/src/jsapi.cpp#6259

So there was no reason to continue with the strcmp, when scriptFile doesn't hold the real script URL.

Does that make sense?

Yes, but I was not talking about that part. I was wondering about:

        nsPrintfCString msg("On %s: blocked access to canvas image data"
                            " from document %s, script from %s:%u ",  // L10n
                            firstPartySpec.get(), docSpec.get(),
                            scriptFile.get(), scriptLine);

where the message is constructed with the script file and the respective line *regardless* whether DescribeScriptedCaller succeeded or not. It seems to me if you wrap the former in an if-clause to be defensive (which is good, especially as Mozilla has not our patch in its repository yet and might change behavior of code we need without us noticing it) you should make sure scriptFile is updated when constructing the message as well (if not we might just give back a generic message without it).

One additional nit: Mozilla code breaks usually at 80 chars. I think we should follow that rule. It might make it a bit easier to upstream things without spending another review cycle fixing the line-wrapping.

Now: does that make sense?

Changed 4 years ago by gacar

Attachment: canvas_exempt.patch added

Exempt chrome callers and PDF.js from canvas prompt

comment:18 in reply to:  17 Changed 4 years ago by gacar

I, too, was concerned about this part. I updated the patch, also broke down the 80+ line.

Replying to gk:

Yes, but I was not talking about that part. I was wondering about:

        nsPrintfCString msg("On %s: blocked access to canvas image data"
                            " from document %s, script from %s:%u ",  // L10n
                            firstPartySpec.get(), docSpec.get(),
                            scriptFile.get(), scriptLine);

where the message is constructed with the script file and the respective line *regardless* whether DescribeScriptedCaller succeeded or not. It seems to me if you wrap the former in an if-clause to be defensive (which is good, especially as Mozilla has not our patch in its repository yet and might change behavior of code we need without us noticing it) you should make sure scriptFile is updated when constructing the message as well (if not we might just give back a generic message without it).

comment:19 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks, gacar! This is closed by fc1c9ff7c1b2defdbc039f12214767608f46423f. I just formatted your commit message a bit and made it a bit more verbose.

Note: See TracTickets for help on using tickets.