Closed Bug 1318565 Opened 3 years ago Closed 3 years ago

WebExtensions cannot save tainted <canvas>

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox55 fixed)

VERIFIED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: masayuki, Assigned: Tomislav)

References

Details

Attachments

(2 files)

I'm trying to create an add-on which save images without context menu and naming the file names automatically. However, I realized that there are no way to access tainted <canvas>.

Looks like that there is no way to <canvas> in content from background script. Additionally, content script runs without chrome privilege, therefore, if <canvas> is tainted, add-on cannot save image of it with .toDataURL().
https://dxr.mozilla.org/mozilla-central/rev/05e5b12f41df270b31955ff7e6d09245c1f83a7a/dom/html/HTMLCanvasElement.cpp#629,636-638

I guess, content script should have another permission to access tainted <canvas> or something special API.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1310318
Looks like that bug 1310318 doesn't allow to access neither |.toDataURL()| nor |.toBlob()|.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Depends on: 1310318
do you have any insight on this based on draw window?
Flags: needinfo?(tomica)
Not really, but I can investigate..
Assignee: nobody → tomica
Flags: needinfo?(tomica)
Attachment #8853586 - Flags: review?(bzbarsky)
Attachment #8853587 - Flags: review?(kmaglione+bmo)
I missed the getImageData().
Hey Boris, this is using the same permission as drawWindow() from bug 1310318, since an extension can effectively already do this -- add an image to a blank document, draw that window.


Relatedly, this also raises the question if drawWindow() should actually mark the canvas as tainted?  Currently, the only two allowed callers are chrome code and an extension with a permission (which ignore the tainted flag anyway), but both of those might (inadvertently) pass that canvas back to some content js, which could be problematic.  Worth a bug?
Comment on attachment 8853586 [details]
Bug 1318565 - Allow extensions with permission to read from tainted Canvas

https://reviewboard.mozilla.org/r/125656/#review129796

::: dom/canvas/CanvasRenderingContext2D.cpp:5401
(Diff revision 2)
>    if (mCanvasElement && mCanvasElement->IsWriteOnly() &&
>        // We could ask bindings for the caller type, but they already hand us a
>        // JSContext, and we're at least _somewhat_ perf-sensitive (so may not
>        // want to compute the caller type in the common non-write-only case), so
>        // let's just use what we have.
> -      !nsContentUtils::IsSystemCaller(aCx))
> +      !nsContentUtils::CallerHasPermission(aCx, NS_LITERAL_STRING("<all_urls>")))

Would it make sense to also allow reading tainted canvas with a slightly finer-grained permission?  Or would that be too misleading/confusing?

::: dom/html/HTMLCanvasElement.cpp:651
(Diff revision 2)
>                               nsAString& aDataURL,
>                               CallerType aCallerType,
>                               ErrorResult& aRv)
>  {
>    // do a trust check if this is a write-only canvas
> -  if (mWriteOnly && aCallerType != CallerType::System) {
> +  if (mWriteOnly &&

Is the aCallerType unused, then?  If so, we should probably remove that arg and the corresponding annotation in the webidl...

::: dom/html/HTMLCanvasElement.cpp:835
(Diff revision 2)
>                            JS::Handle<JS::Value> aParams,
>                            CallerType aCallerType,
>                            ErrorResult& aRv)
>  {
>    // do a trust check if this is a write-only canvas
> -  if (mWriteOnly && aCallerType != CallerType::System) {
> +  if (mWriteOnly &&

And here.
Attachment #8853586 - Flags: review?(bzbarsky) → review+
Having drawWindow mark the canvas as tainted seems like a good idea, btw.
Comment on attachment 8853587 [details]
Bug 1318565 - Test extension permission to read from a tainted canvas

https://reviewboard.mozilla.org/r/125658/#review130012
Attachment #8853587 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8853586 [details]
Bug 1318565 - Allow extensions with permission to read from tainted Canvas

https://reviewboard.mozilla.org/r/125656/#review129796

> Would it make sense to also allow reading tainted canvas with a slightly finer-grained permission?  Or would that be too misleading/confusing?

It would be nice if we had a finer-grained notion of tainting, and we could check for specific host permissions instead of <all_urls>. But I think that would be more work than it's worth, and we'd still have to fall back to <all_urls> after drawWindow, anyway.
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e5e9ecdfd8a
Allow extensions with permission to read from tainted Canvas r=bz
https://hg.mozilla.org/integration/autoland/rev/d23faba1c134
Test extension permission to read from a tainted canvas r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e5e9ecdfd8a
https://hg.mozilla.org/mozilla-central/rev/d23faba1c134
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thank you, I confirmed with today's Nightly.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.