Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#17446 closed defect (fixed)

Canvas image extraction prompt logic

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-fingerprinting, PearlCrescent201511R, TorBrowserTeam201512R
Cc: brade, mcs, mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

For our canvas image extraction patch (#6253), I think the following describes the logic in
bool IsImageExtractionAllowed(nsIDocument *aDocument, JSContext *aCx)
in CanvasUtils.cpp:

  1. Get the first party URI of the aDocument
  2. Check Permission Manager to see if that first party URI has permission to access canvas data; return "true" if so.
  3. Otherwise, check if aDocument is "third party" (meaning, presumably, an iframe or similar)
  4. If aDocument is not "third party", then show a prompt allowing user to give permission (to the first party) to access canvas data.
  5. If the user gives permission ("always"), then add the first-party URI to the Permissions database

Is there a reason we are preventing third parties from requesting permission on behalf of the first party?

My feeling is we should either (a) allow third parties to request permission to extract canvas data, but assign that permission to the first party, or (b) prevent third parties from extracting canvas data at all. I might be confused about this, though.

Child Tickets

Change History (19)

comment:1 in reply to:  description ; Changed 4 years ago by mcs

Replying to arthuredelstein:

Is there a reason we are preventing third parties from requesting permission on behalf of the first party?

When the canvas prompt was first added, there were too many prompts. See #7265.

My feeling is we should either (a) allow third parties to request permission to extract canvas data, but assign that permission to the first party, or (b) prevent third parties from extracting canvas data at all.

I think (a) results in too many prompts. I think you have found a problem though: because the first party permission check is done first, if permission is ever granted for a page then all third party access is allowed. I am in favor of (b) but I do not know if that would break any sites that use canvas in a legitimate, non-fingerprinting way.

comment:2 in reply to:  1 ; Changed 4 years ago by arthuredelstein

Replying to mcs:

Replying to arthuredelstein:

Is there a reason we are preventing third parties from requesting permission on behalf of the first party?

When the canvas prompt was first added, there were too many prompts. See #7265.

My feeling is we should either (a) allow third parties to request permission to extract canvas data, but assign that permission to the first party, or (b) prevent third parties from extracting canvas data at all.

I think (a) results in too many prompts. I think you have found a problem though: because the first party permission check is done first, if permission is ever granted for a page then all third party access is allowed. I am in favor of (b) but I do not know if that would break any sites that use canvas in a legitimate, non-fingerprinting way.

I guess any third-party canvas extractions broken under (b) are also already broken by the current patch, unless that domain has previously been given permission as a first-party domain. This seems like an undesirable bit of linkability anyhow.

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

Replying to arthuredelstein:

I guess any third-party canvas extractions broken under (b) are also already broken by the current patch, unless that domain has previously been given permission as a first-party domain.

Right, and that scenario seems unlikely.

This seems like an undesirable bit of linkability anyhow.

Do you want to create a patch or should Kathy and I?

comment:4 in reply to:  3 Changed 4 years ago by arthuredelstein

Replying to mcs:

Replying to arthuredelstein:

I guess any third-party canvas extractions broken under (b) are also already broken by the current patch, unless that domain has previously been given permission as a first-party domain.

Right, and that scenario seems unlikely.

This seems like an undesirable bit of linkability anyhow.

Do you want to create a patch or should Kathy and I?

I'm happy to give it a try, if you don't mind reviewing it. :) I'm going to attempt to rebase the canvas patch to mozilla-central but I'll wait until this fixup is settled.

comment:5 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201511R added
Status: newneeds_review

Here's a patch for review. I did some reformatting of the IsImageExtractionAllowed function to try to clarify each of the steps.

https://github.com/arthuredelstein/tor-browser/commit/17446

comment:6 Changed 4 years ago by mcs

Keywords: PearlCrescent201511R added

comment:7 Changed 4 years ago by mcs

Status: needs_reviewneeds_revision

Kathy and I reviewed this and the logic seems correct. A couple of small things:
a) Please replace "Blocked third party %s from extract canvas data." with "Blocked third party %s from extracting canvas data." (s/extract/extracting)
b) It would be better to keep the console messages as one message instead of two (better for filtering the console output).
c) We should continue to log both the first party URI and the doc URI when canvas access is blocked. Otherwise it is difficult for end-users to figure out what was blocked on what page.

comment:8 Changed 4 years ago by arthuredelstein

Status: needs_revisionneeds_review

Thanks for the review. Here's a new version with fixes made as requested:

https://github.com/arthuredelstein/tor-browser/commit/17446+1

comment:9 Changed 4 years ago by mcs

Status: needs_reviewneeds_revision

This mostly looks good, but I think Kathy and I missed a problem during our previous review. In the following code block, the test should be permission != nsIPermissionManager::DENY_ACTION. We want to log when the user has not yet made a decision (i.e., they have not responded to the prompt). After they choose "Never for this site" (aka DENY_ACTION) there is no need to log.

    } else if (permission == nsIPermissionManager::DENY_ACTION) {
      nsAutoCString message;
      message.AppendPrintf("Blocked page %s from extracting canvas data.",
                           firstPartySpec.get());
      if (isScriptKnown) {
        message.AppendPrintf(" %s:%u.",
                             scriptFile.get(), scriptLine);
      }
      nsContentUtils::LogMessageToConsole(message.get());
      return false;
    }

And shouldn't we log the docURI in this case also?

It would be good if your commit message briefly explained the purpose of this fixup.

comment:10 Changed 4 years ago by gk

While we are at it (I have not checked the whole patch yet):

1) Your new patch has added a newline right at the beginning of the file.

2) We have

  if (!aDocument || !aCx)
    return false;

  nsPIDOMWindow *win = aDocument->GetWindow();

which basically assumes we'll always have a document as otherwise aDocument->GetWindow() would blow up. Why do we have the !aCx check then at all?

Last edited 4 years ago by gk (previous) (diff)

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

Replying to gk:

2) We have

  if (!aDocument || !aCx)
    return false;

  nsPIDOMWindow *win = aDocument->GetWindow();

which basically assumes we'll always have a document as otherwise aDocument->GetWindow() would blow up. Why do we have the !aCx check then at all?

That's actually bullshit, sorry for the noise. Some kind of Friday-evening code-blindness or something. :(

Last edited 4 years ago by gk (previous) (diff)

comment:12 in reply to:  9 ; Changed 4 years ago by arthuredelstein

Replying to mcs:

This mostly looks good, but I think Kathy and I missed a problem during our previous review. In the following code block, the test should be permission != nsIPermissionManager::DENY_ACTION. We want to log when the user has not yet made a decision (i.e., they have not responded to the prompt). After they choose "Never for this site" (aka DENY_ACTION) there is no need to log.

    } else if (permission == nsIPermissionManager::DENY_ACTION) {
      nsAutoCString message;
      message.AppendPrintf("Blocked page %s from extracting canvas data.",
                           firstPartySpec.get());
      if (isScriptKnown) {
        message.AppendPrintf(" %s:%u.",
                             scriptFile.get(), scriptLine);
      }
      nsContentUtils::LogMessageToConsole(message.get());
      return false;
    }

Argh, sorry for screwing that up. I have fixed it now.

And shouldn't we log the docURI in this case also?

Yes, I added it back in.

It would be good if your commit message briefly explained the purpose of this fixup.

Good idea. Here's the new patch:

https://github.com/arthuredelstein/tor-browser/commit/17446+2

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

Replying to arthuredelstein:

... Here's the new patch:

https://github.com/arthuredelstein/tor-browser/commit/17446+2

Looks good.
r=mcs, r=brade

comment:14 Changed 4 years ago by gk

Status: needs_revisionneeds_review

So, the code looks good for me wrt what it is intended to do. But I am not sure whether we really want it to behave that way: the spec is supporting the old behavior (i.e. this seems to be a feature):

If the user hasn't previously allowed the site in the URL bar to access Canvas image data, pure white image data is returned to the Javascript APIs.

It has some appeal to think about this being like a cookie or DOM storage token and thus treat it the same way (bind it to the state of the domain in the URL bar). But this model might be flawed in this case as a) it is possible to get rid of the client side tokens/have some control about the collection which is hard in the case of the fingerprint created by the canvas extraction and b) there is some additional information leaking as the canvas extraction is only allowed after the user gave consent. And then, c), there seems to be the use-case missing for third-party canvas extraction (but maybe I am wrong here).

I guess we can test it in the alpha a bit, though. commit b501eedc0b4b8018f930fdaf3fc5d0116fab0b14 merged the patch.

comment:15 Changed 4 years ago by gk

Cc: mikeperry added
Status: needs_reviewneeds_information

Mike, any thoughts about my reasoning in comment:14?

comment:16 Changed 4 years ago by gk

It seems my comment:14 has been a bit dense. So here is what I thought the former behavior is/was (which seems to me some kind of binding the canvas extraction to the URL bar domain): Given three distinct domains A, B and C.

Scenario 1: vanilla Tor Browser, first-party domain A with no script trying to extract image data, third-party domain B and the script on B wants to extract image data from the canvas. Result: that is not going to happen and there will be no prompt, only some logs about this in the console.

Scenario 2: like scenario 1 but now A got granted the permission to extract the data previously. Result: the script on B is allowed too, nothing is logged to the console.

Scenario 3: scenario 2 happened a while ago. Now, the user is visiting C which includes B as third-party domain. Result: the same like in scenario 1

Scenario 4: like scenario 3 but now C got granted the permission to extract the data previously.
Result: the script on B is allowed too, nothing is logged to the console.

Now, this looks like the permission and hence the canvas extraction is bound to the URL bar domain (and whether that extracts canvas data, too, and is allowed to) which seems, at first glance, to fit neatly to our efforts to thwart cross-domain linking. But then there is mainly a) in comment:14 speaking against that impression. And b) and c) cast even more doubt whether we should stick to the old patch being specified in our design document.

(I hope my thoughts are clearer now. :) )

comment:17 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201512R added; TorBrowserTeam201511R removed

comment:18 Changed 4 years ago by gk

Resolution: fixed
Status: needs_informationclosed

Okay, I've been thinking about that more and I came to the conclusion that this is strictly better for stable users as well compared to the current behavior: third party access would be mostly blocked anyway and seemingly randomly (form a user perspective) allowing access to the canvas data feels only weird (let alone the additional fingerprinting risk). So, I applied the patch to the stable, too: commit 9a717351831505dff9b48b579353c043d8531fd6 has it.

comment:19 Changed 4 years ago by mcs

See #17931 for a crash introduced by this fix.

Note: See TracTickets for help on using tickets.