Inspector raises the canvas prompt when hovering over images
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 
.
4. Hover over an img element.
The "attempted to extract HTML5 canvas image data" prompt appears.
Activity
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.
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.
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 (re/)) 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.
Trac:
Keywords: canvas tbb-4.0 deleted, tbb-usability, tbb-easy added
Cc: N/A to mcs, bradeThere was already a check for exempting
file://URLs from canvas prompt inIsImageExtractionAllowed(). The above patch addsre[/](/`) 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.-
Replying to gacar:
There was already a check for exempting
file://URLs from canvas prompt inIsImageExtractionAllowed(). The above patch addsre[/](/`) 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
re[/](/) inscriptFileor maybe there is even a better method thanDescribeScriptedCaller()` 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. 
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.
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 IsSystemPrincipalFalse 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.
According to PDF.js FAQ and this comment, most of the PDF.js code runs with content privileges.
So, adding a
IsCallerChromecheck would work for the Inspector, but not for the PDF.js.Can whitelisting
re[/pdf.js](/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
IsCallerChromeand whitelist PDF.js via scheme/URL whitelist) I could submit a new patch.See, also #10570.
-
Replying to gacar:
According to PDF.js FAQ and this comment, most of the PDF.js code runs with content privileges.
So, adding a
IsCallerChromecheck would work for the Inspector, but not for the PDF.js.Can whitelisting
re[/pdf.js](/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
IsCallerChromeand 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.
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
re[/pdf.js/build/pdf.js.](/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.
-
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.
-
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()andscriptLineare 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 ;)
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()andscriptLineare 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
DescribeScriptedCallerreturns false, we're sure that neitherscriptFilenorscriptLineis updated with the caller script's URL and line no: http://mxr.mozilla.org/mozilla-esr31/source/js/src/jsapi.cpp#6259So there was no reason to continue with the
strcmp, whenscriptFiledoesn't hold the real script URL.Does that make sense?
-
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
DescribeScriptedCallerreturns false, we're sure that neitherscriptFilenorscriptLineis updated with the caller script's URL and line no: http://mxr.mozilla.org/mozilla-esr31/source/js/src/jsapi.cpp#6259So there was no reason to continue with the
strcmp, whenscriptFiledoesn'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? Trac:
canvas_exempt.patchI, 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
DescribeScriptedCallersucceeded 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 surescriptFileis updated when constructing the message as well (if not we might just give back a generic message without it).mentioned in issue #14205 (moved)
mentioned in issue #14985 (moved)
