Closed Bug 1511104 Opened 6 years ago Closed 5 years ago

Firefox download page must pass through params, but NOT as utm_

Categories

(www.mozilla.org :: Bedrock, enhancement)

Product:
www.mozilla.org
Component:
Bedrock
Version:
Production
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox66 verified)

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
firefox66 --- verified

People

(Reporter: mixedpuppy, Assigned: pmac)

References

(
URL
)

Details

Attachments

(1 file)

Link to GitHub pull-request: https://github.com/mozilla/bedrock/pull/6549
44 bytes, text/x-github-pull-request
Details | Review 
See linked doc.  On the 2nd page the problem is clearly explained.  Without this addressed, we will not be able to fully test attribution for osx downloads.
Component: General → Activity Streams: Newtab
Product: WebExtensions → Firefox
NOTE: the utm_* params must be passed through without the "utm_" part of the param name.
(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> NOTE: the utm_* params must be passed through without the "utm_" part of the
> param name.

ignore that.

The issue is that the *referrer* url must contain the utm_* codes.
Hi Eric,

Mixedpuppy is the engineer who understands the request - and wrote up explination https://docs.google.com/document/d/1w2YnGSemYViJ2Cn5_i67BHSpNoRy1TwUMOW62GS0Fik/edit.

Timing wise - we found this when we were looking at testing a feature end-to-end (that has been in progress for months).  In order to have this feature ready in Firefox 65 - which ships late Jan - we need to land code and test it out ASAP.  I hate to ask for a last minute rush - but if there is any way to look at what this would take to see how quickly it could be done, especially if it's a small change - we would really appreciate it.

I just filled in "version and target" information - because it was required to move to this area.  I have no idea what those values should be.  We are working out how to test this feature end-to-end.  So it's possible "development/ stage" is the right initial request for this change.  There is a bug 1511071 to work on how to QA something in this download area.
Component: Activity Streams: Newtab → Bedrock
Flags: needinfo?(erenaud)
Product: Firefox → www.mozilla.org
Target Milestone: --- → 1.0
Version: unspecified → Production
Assignee: nobody → pmac
Status: NEW → ASSIGNED
Flags: needinfo?(erenaud)
I know this bug explicitly states that this is just something the firefox download page needs (i.e. /firefox/new/ -> /firefox/download/thanks/), but the windows attribution is much more versitile. If a windows user comes to pretty much any page of the site with some utm_* params those are recorded, encoded, and stored in a cookie. Then when the user does go to download that data is added to the request for recording in the stub installer. Should the Mac version be as flexible, or do we only care about this working from /new/ to /thanks/, at least for now?
Well, our primary concern (and only use case right now) is installations coming from AMO product pages, which link to /new/. So I'm inclined to say it's the latter ("we only care about this working from /new/ to /thanks/, at least for now").

If someone was interested in OSX attribution in general, then we could opt for being more flexible.
(In reply to Paul [:pmac] McLanahan ⏰ET needinfo? me from comment #4)
> Then when the user does go to download that
> data is added to the request for recording in the stub installer. Should the
> Mac version be as flexible, or do we only care about this working from /new/
> to /thanks/, at least for now?

OSX doesn't have an installer, but all osx browsers add the quarantine data to downloaded binaries, which includes the download url and the referrer url.  It might be possible to pass those utm params through in a more generic fashion, such as taking the cookie data and adding params to the download url itself.  We'd have to do some adjustment to the attribution code in firefox as it only looks at the referrer url.
:mixedpuppy - Would it be acceptable to simply pass *all* query params on to /download/thanks/ at all times? Will it hurt anything to potentially have non-utm params in the referrer URL, or to have query params in the referrer for all platforms?

I'm asking because we have JS that runs on *some* /firefox/new/ variation pages that does this - appends all current page querystring values on to the download links. I'd rather avoid nitty-gritty querystring parsing unless absolutely necessary.
Flags: needinfo?(mixedpuppy)
It is fine to pass through all params.  The attribution system will parse and pull out only specific utm_* params.  The params wont have any affect on any other platform.
Flags: needinfo?(mixedpuppy)
We should not add utm parameters to a same-site link. When Google Analytics sees a utm_campaign parameter in the URL, it considers that request the start of a new session[0]. If we implement the solution described, then...

1. A user session clicking the download button on /new will cause our download count to increment.
2. That same user will arrive on /thanks with a new utm_campaign parameter, which will force a new session to begin. A new session visiting /thanks will also cause our download count to increment. Also, a (single) download will begin.
3. In GA reports, download volume among this segment will double. Each downloading user will have 2 downloads in 2 sessions, one right after the other. The number of installs and aDAU will not double.

This confounds one of our most important metrics. We should avoid it using something like the design described in comment 4. If I understand it correctly, that solution will have the added benefit of working on download links sitewide, not just the green button on /new.

[0] https://support.google.com/analytics/answer/2731565#campaign-based-expiration
Changed summary to reflect that we could pass params through in a way that still allows this to work, provided we uplift a small change to Firefox. Shane will file a bug for the other change.
Summary: Firefox download page must pass through utm params → Firefox download page must pass through params, but NOT as utm_
Flags: needinfo?(mixedpuppy)
Depends on: 1515153
Justin, I've a patch on bug 1515153 that makes the attribution code in Firefox work with the "utm_" part of the param name stripped.  So if you strip that, and add the params to the referrer for the download url, is that going to be fine on your end?
Flags: needinfo?(mixedpuppy) → needinfo?(hoosteeno)
I think this will solve the narrow case, but only if these parameters are handled in the same way by the service that validates them[0]. I can't answer that question -- do you know?

I will add one comment to bug 1515153 regarding the scope of the patch.


[0] https://github.com/mozilla-services/stubattribution/blob/master/attributioncode/validator.go
Flags: needinfo?(hoosteeno)
Quick question: Is it possible that a downloading session will have e.g. a utm_campaign parameter _and also_ a campaign parameter at the moment of download? Have we anticipated that?
(In reply to Justin Crawford [:hoosteeno] [:jcrawford] from comment #14)
> Quick question: Is it possible that a downloading session will have e.g. a
> utm_campaign parameter _and also_ a campaign parameter at the moment of
> download? Have we anticipated that?

It will only be possible if a) the download starts from AMO, b) this bug passes both utm_content and content on the referrer url for the download.  Since passing utm_ is problematic, I expect that wont happen.  I wanted to keep utm_ support in case we do want to support campaign tracking.  The attribution code will prefer the plain content over the utm_content (and other keys) so attribution data will only end up with one of them.
oh, and this is only an osx thing, it has no affect on windows.
No longer depends on: 1515172
Have we reached a decision on what exactly we want the site to do? If so can someone summarize here so that we can get it knocked out quickly?
It's basically #c1. NI mixedpuppy for sanity check.
Flags: needinfo?(mixedpuppy)
So the summary then is:

When we see any "utm_*" parameter on /firefox/new/, strip the "utm_" prefix, and add those parameters to the link to /firefox/download/thanks/.

That sound right?
Adding to Pual's comment 19 above, we should *only* add these parameters for macOS users, correct? In other words, we should explicitly *not* add these parameters for any other OS.
I don't imagine they would make any difference to any other OS, since neither campaign tracking nor attribution is looking for them. But if we want to be exact and minimize any potential future parameter bleed, then yes.
(In reply to Paul [:pmac] McLanahan ⏰ET needinfo? me from comment #19)
> So the summary then is:
> 
> When we see any "utm_*" parameter on /firefox/new/, strip the "utm_" prefix,
> and add those parameters to the link to /firefox/download/thanks/.
> 
> That sound right?

yes
Flags: needinfo?(mixedpuppy)
:mixedpuppy - A couple questions to help make this implementation as tight as possible:

1) Are there specific utm_* params you are looking for? In other words, can we filter any unnecessary utm_* params out in an effort to modify download links as little as possible?

2) If the answer to #1 is "yes", are there specific values we can allow? If not, are there specific patterns we can validate against to ensure a value is valid?

We are a little wary of taking *all* utm_* keys with *any* arbitrary value and putting that information back into the DOM/href link.
Flags: needinfo?(mixedpuppy)
(In reply to Jon Petto [:jpetto] from comment #23)
> We are a little wary of taking *all* utm_* keys with *any* arbitrary value
> and putting that information back into the DOM/href link.

I don't understand. I thought the issue was replicating utm_ params. If those prefixes are removed and the results are replicated to the download link, what is the concern?
Flags: needinfo?(jon)
The concern is if there could be some value/key+value data that could cause issues (by content and/or length) at any point along the line. We're thinking of querystring data essentially as user input that should be sanitized in some fashion.

We will make sure the keys and values are URL encoded, but beyond that don't have any checks. That being the case, the referrer URL could be 2000+ characters long with key/value pairs of (theoretically) great length. Is that acceptable on your end?
Flags: needinfo?(jon) → needinfo?(ddurst)
Ah. Well, since this is all about getting attribution working on OSX, and our previous assertion is that the querystring would be longer, I have to assume that long-but-shorter-than-previously-expected is fine.

The only parameters we should be caring about in this context (if I understand correctly) are the ones that attribution system cares about -- and that's the utm_ prefixed ones (source, medium, campaign, content) -- which, I believe, is all of them.

(One caution is that the parameters are probably already URL encoded, and we should be sure to not double up on that like happened for Windows.)

If we want to narrow the scope for safety, then we should go ahead and limit that to OSX, as you suggested in #c20.

But if there's concern about this we can wait until mixedpuppy is back next week.
Flags: needinfo?(ddurst)
> 2) If the answer to #1 is "yes", are there specific values we can allow? If
> not, are there specific patterns we can validate against to ensure a value
> is valid?

This bug seems relevant here: https://bugzilla.mozilla.org/show_bug.cgi?id=1515172

It implies a future state where we allow a couple more params into the attribution content. As you can see, it's not been heavily vetted and certainly not committed, but worth anticipating if possible.
It's probably worth making sure the utm_ params included are in the pre-defined list provided in comment 26:

utm_source
utm_medium
utm_campaign
utm_content

Without this rule, someone could manually type in utm_haxx=sqlinjectioncode and it would get passed along. On this note, imagine someone added a utm_somereservedkeyword=somenastyvalue to the URL. This would pass somereservedkeyword=somenastyvalue in the referrer URL. I don't know if/how this data is sanitized/checked at the endpoint - which is why I'm bringing it up.

This potential exploit would still exist if we limit to the keys listed above and still allow any value. There's nothing stopping a user from changing say utm_campaign=cooladdon to utm_campaign=morehaxxhere. (Again, I don't know if a URL encoded value can cause harm at the endpoint, but I think it's worth double-checking.)

FWIW, on the Firefox Accounts side, they have both a list of allowed keys and allowed values to mitigate this risk. (Not sure if that's feasible in this case, however.)

Sounds like we might want to wait on :mixedpuppy to verify we'll be safe with just the list of available keys?
You can see the codes that are consumed here:

https://searchfox.org/mozilla-central/rev/fef7f858efb695a76010b4c624da5277c16e95b3/browser/components/attribution/AttributionCode.jsm#18-22

As far as utm_random=badstuff. it wouldn't really do anything since we don't make them available.

Windows doesn't use the referrer url, so whether the params are passed for non-osx platforms or not doesn't matter. (linux doesn't support attributions at all)

Allowed values is unrealistic for utm params.
Flags: needinfo?(mixedpuppy)
Our concern is mostly due to us injecting whatever values are in these utm_ params into the DOM and the potential for an XSS attack vector on the Firefox download page. This is the most security sensitive page on the site, and this is already a very sensitive site as JS executing on the domain has privileged API access in Firefox. So we're just trying to be as overly cautious as possible with untrusted data.

To that end, is there any format that these values will take? I assume they'll contain some addon identifier. Do said identifiers conform to some pattern? If we could even limit them to a set of allowed characters and a length we'd feel better about it.

For the Windows attribution we go so far as to sanitize and include an HMAC signature of the data so that the stub attribution service can verify that the request came from www.m.o. And the attribution service in turn has a whitelist for utm_source to aid in privacy. It'd be nice if we could only accept signed params from AMO to add some trust to the system, but failing that (it'd be significantly more work for both of us) we need some way to validate the data before passing it into the DOM on the download page.
Flags: needinfo?(mixedpuppy)

I don't see anything in the attribution service that checks any signature. We wouldn't be able to get that into Firefox any time soon. I suppose it could be done on AMO, which could sign the data, and validated by w.m.o before adding the params to the referrer url for the download. I'm not the person to involve in that.

Flags: needinfo?(mixedpuppy)

I just realized that you're concerned these params are ending up in DOM. That is not the case.

source is checked that it is addons.mozilla.org. content is used as an extension id, which is used in an XHR back to AMO, which returns a json object with the extension data. That data is what is used in DOM.

Flags: needinfo?(pmac)

(In reply to Shane Caraveo (:mixedpuppy) from comment #31)

I don't see anything in the attribution service that checks any signature.

All the validation code in the service is here, including some blocks related to HMAC checking: https://github.com/mozilla-services/stubattribution/blob/master/attributioncode/validator.go#L38

(In reply to Shane Caraveo (:mixedpuppy) from comment #31)

I don't see anything in the attribution service that checks any signature. We wouldn't be able to get that into Firefox any time soon. I suppose it could be done on AMO, which could sign the data, and validated by w.m.o before adding the params to the referrer url for the download. I'm not the person to involve in that.

I was mainly referring to the communication between AMO and WMO, but I don't really think it's necessary. The sig checking in the attribution service is here:

https://github.com/mozilla-services/stubattribution/blob/45f31a191774f8af94775f195802f34ec9d56bd5/attributioncode/validator.go#L64-L69

WMO signs the data to be sent with HMAC, and the stub service checks the sig before generating a stub binary. This was done mostly to prevent someone from directly requesting thousands of worthless stub binarys being generated by just hitting the service directly. We don't have that issue here, so I think we're fine.

I just realized that you're concerned these params are ending up in DOM. That is not the case.

source is checked that it is addons.mozilla.org. content is used as an extension id, which is used in an XHR back to AMO, which returns a json object with the extension data. That data is what is used in DOM.

The DOM I'm concerned about is on our side, on the Firefox download page. We're being asked to take untrusted data from the URL on one page, insert it into the DOM on that page so that the Firefox download link is altered. It's conceivable that without some sanitization a URL could be crafted such that the new destination of the "download firefox" link could be changed to another site entirely, or that it could cause arbitrary JS to execute. It's unlikely, but this is an incredibly security sensitive page since it's the place from which people download Firefox.

So what we're asking for is some basic rules for the data; Things like, "it won't be longer than X" and "it will only contain Y characters". Then we can reject anything that doesn't look like that and avoid unnecessarily touching the download firefox link.

Once the params are on the /firefox/download/thanks/ page URL it's up to Firefox to decide whether it's safe to parse and use the data since that will still be very much untrusted and could have come from anywhere on the web, since that page too can be linked to by anyone. We're just trying to protect the /firefox/new/ page as much as possible.

Ok, my work is all on the firefox side and that's what I'm looking at. We'd need to get someone on the AMO team on this conversation instead of me, I think that would be more fruitful. I'm sure something could be done to deal with this between AMO and WMO.

ddurst, who is best for this conversation?

Flags: needinfo?(ddurst)

(In reply to Shane Caraveo (:mixedpuppy) from comment #35)

Ok, my work is all on the firefox side and that's what I'm looking at. We'd need to get someone on the AMO team on this conversation instead of me, I think that would be more fruitful. I'm sure something could be done to deal with this between AMO and WMO.

Sounds right. Thanks :mixedpuppy!

Flags: needinfo?(pmac)

There's a format specified for this over in https://github.com/mozilla/addons-frontend/issues/7240#issuecomment-447831736, so that's what would populate the utm_content (for both Windows and Mac OSX). That should be sufficient for wmo to validate? (assuming that it gets escaped for sanitization)

Flags: needinfo?(ddurst) → needinfo?(pmac)

That looks perfect! Thanks :ddurst! We'll work with that. My understanding is that:

  • utm_campaign will always be non-fx-button
  • utm_medium will always be referral
  • utm_source will always be addons.mozilla.org
  • utm_content will be rta%3A<base64_string> where <base64_string> is a collection of /[a-zA-Z0-9]+/

If those are all true then we should send the values through to the next page, otherwise we do nothing. Sound right?

Flags: needinfo?(pmac)

Yes, except noting that the fourth bullet should be:

  • utm_content will be rta%3A<base64_string> where <base64_string> is a collection of /[a-zA-Z0-9_\-]+/

(In reply to David Durst [:ddurst] from comment #39)

Yes, except noting that the fourth bullet should be:

  • utm_content will be rta%3A<base64_string> where <base64_string> is a collection of /[a-zA-Z0-9_\-]+/

Updated the PR to reflect this change.

Fixed in https://github.com/mozilla/bedrock/commit/f250e7f8c006eb96d090128f5f9c520a42b78013

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Will this fix require manual validation from the QA team or is it handled by automation/unit tests?

Flags: needinfo?(pmac)

utm_content will be rta%3A<base64_string> where <base64_string> is a collection of /[a-zA-Z0-9_-]+/

:ddurst, two questions:

  • can we length limit this to something reasonable?
  • can it be decoded and validated against actual values, rather than treating it as a blob?
Flags: needinfo?(ddurst)

(In reply to Paul [:pmac] McLanahan ⏰ET needinfo? me from comment #34)

(In reply to Shane Caraveo (:mixedpuppy) from comment #31)

I don't see anything in the attribution service that checks any signature. We wouldn't be able to get that into Firefox any time soon. I suppose it could be done on AMO, which could sign the data, and validated by w.m.o before adding the params to the referrer url for the download. I'm not the person to involve in that.

I was mainly referring to the communication between AMO and WMO, but I don't really think it's necessary. The sig checking in the attribution service is here:

https://github.com/mozilla-services/stubattribution/blob/45f31a191774f8af94775f195802f34ec9d56bd5/attributioncode/validator.go#L64-L69

WMO signs the data to be sent with HMAC, and the stub service checks the sig before generating a stub binary. This was done mostly to prevent someone from directly requesting thousands of worthless stub binarys being generated by just hitting the service directly. We don't have that issue here, so I think we're fine.

:pmac, you don't think we should hmac the value set by AMO so they can be verified by wmo before forwarding them to stubattribution?

If we don't do that, aren't we allowing anyone to create download URLs with somewhat arbitrary (but valid) data?

(In reply to Julien Vehent [:ulfr] from comment #43)

utm_content will be rta%3A<base64_string> where <base64_string> is a collection of /[a-zA-Z0-9_-]+/

:ddurst, two questions:

  • can we length limit this to something reasonable?

We can, though I'm not sure of the value there. Any GUID has a max length of 255, so the max encoded (I'm assuming this would be checked prior to decoding?) would be 340. As this should be escaped anyway, and the server rejects things that do not match or decode properly, this would/could be a check on w.m.o that wouldn't really impact anything further along the line. Meaning: if this is an abundance of caution, great; but it's not that meaningful in a mitigation sense, as far as I can tell. [Noting previous comments and email threads about the fact that an arbitrary URL used there is still not a threat.]

[Also reiterating that this is not protecting anything about attribution (since that's been around for a while) -- just the use of data stored via attribution.]

  • can it be decoded and validated against actual values, rather than treating it as a blob?

Not really. Because the GUID follows https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/browser_specific_settings#Extension_ID_format, the actual values are less useful than formats -- and those seem pretty broad. Since these are used to get a response from the API, bogus ones (ones AMO doesn't know about) will return 400 anyway.

Flags: needinfo?(ddurst)

(In reply to Vlad Jiman from comment #42)

Will this fix require manual validation from the QA team or is it handled by automation/unit tests?

As far as the websties team is concerned it's fixed. The full attribution process will probably require some QA though I'd imagine, yes.

Flags: needinfo?(pmac)

(In reply to Julien Vehent [:ulfr] from comment #44)

:pmac, you don't think we should hmac the value set by AMO so they can be verified by wmo before forwarding them to stubattribution?

If we don't do that, aren't we allowing anyone to create download URLs with somewhat arbitrary (but valid) data?

Yes, but that's already possible. Attribution on OSX is simply using the URL from which Firefox is downloaded, which is automatically stored by OSX with all downloads. So this is possible today by going to https://www.mozilla.org/en-US/firefox/download/thanks/?any-data-here=will-be-stored-by-osx. The difference is that Firefox will now be looking at that URL for specific key/value pairs and doing things with it. It does mean it'll be possible for any website to create a download link for firefox to our official page that will cause Firefox to prompt the user to install said addon after install. So the security of this thing is completely reliant on the addon vetting process on AMO to ensure we're not serving builds that will recommend a malicious or privacy compromising addon. Requiring an HMAC signature or encrypted addon ID would prevent this for this system, but would not help with links directly to /firefox/download/thanks/ since this system is about passing params from /firefox/new/ to /firefox/download/thanks/ and since OSX stores the value of the latter regardless. I think the only way to protect against this would be for AMO to sign or encrypt addon-ids and have Firefox itself verify/decrypt them, or have a list of approved addon IDs somewhere in Firefox.

(In reply to Paul [:pmac] McLanahan ⏰ET needinfo? me from comment #47)

(In reply to Julien Vehent [:ulfr] from comment #44)

:pmac, you don't think we should hmac the value set by AMO so they can be verified by wmo before forwarding them to stubattribution?

If we don't do that, aren't we allowing anyone to create download URLs with somewhat arbitrary (but valid) data?

Yes, but that's already possible. Attribution on OSX is simply using the URL from which Firefox is downloaded, which is automatically stored by OSX with all downloads. So this is possible today by going to https://www.mozilla.org/en-US/firefox/download/thanks/?any-data-here=will-be-stored-by-osx. The difference is that Firefox will now be looking at that URL for specific key/value pairs and doing things with it. It does mean it'll be possible for any website to create a download link for firefox to our official page that will cause Firefox to prompt the user to install said addon after install. So the security of this thing is completely reliant on the addon vetting process on AMO to ensure we're not serving builds that will recommend a malicious or privacy compromising addon.

I'm not sure that is a realistic control here - we are doing less review than ever, and I'm not sure that this is a risk that is considered as part of the review process.

Requiring an HMAC signature or encrypted addon ID would prevent this for this system, but would not help with links directly to /firefox/download/thanks/ since this system is about passing params from /firefox/new/ to /firefox/download/thanks/ and since OSX stores the value of the latter regardless. I think the only way to protect against this would be for AMO to sign or encrypt addon-ids and have Firefox itself verify/decrypt them, or have a list of approved addon IDs somewhere in Firefox.

Could we simply add some kind of flag to the addon data in AMO, such that when about:welcome pulls the addon data from AMO, it checks to see if this add-on is allowed for the return-to-amo flow?

In any case, if we aren't prepared to whitelist add-ons for Return-to-AMO flow somehow, then we need to accept the risk that 3rd party sites can attempt to legitimize their potentially-malicious add-ons via this flow. I'd prefer to whitelist to remove the risk entirely, but I'd concede that the risk here is limited by:

  • this only applies for users who do not already have firefox [1]
  • if the user is getting Firefox (or an add-on) from a malicious website, the website could already just point to a spurious build

Perhaps a middle ground might be just to monitor the add-ons attributions, and investigate if unexpected add-ons are installed via this flow? Would we detect that via our normal attribution data metrics?

[1] or installed but not the default browser (we will still re-install if you have it, but I dont know if we still do about:welcome if you already had firefox before re-installing)

(In reply to Paul [:pmac] McLanahan ⏰ET needinfo? me from comment #47)

the security of this thing is completely reliant on the addon vetting process on AMO to ensure we're not serving builds that will recommend a malicious or privacy compromising addon

I think we really need to discuss this further. There is no formal review process for addons. We rely on linting and best effort to refuse the worst offenders, but it would be very easy for a motivated attacker to get a fraudulent addon listed on AMO.

Just adding a note from https://bugzilla.mozilla.org/show_bug.cgi?id=1511173#c7 from Shane, that his opinion is that whitelisting addons would "inhibits the use of the feature to do this". So this is likely just a product decision. As I stated comment 48, i don't think this risk is unacceptable, but we might want to consider mitigations to detect future abuse.

(In reply to Paul Theriault [:pauljt] from comment #48)

Could we simply add some kind of flag to the addon data in AMO, such that when about:welcome pulls the addon data from AMO, it checks to see if this add-on is allowed for the return-to-amo flow?

I like this suggestion a lot. It offers protection for users and flexibility for AMO and product. It should also allow us to more easily collect statistics when and if unapproved addons show up in this way.

(In reply to Paul [:pmac] McLanahan ⏰ET needinfo? me from comment #46)

(In reply to Vlad Jiman from comment #42)

Will this fix require manual validation from the QA team or is it handled by automation/unit tests?

As far as the websties team is concerned it's fixed. The full attribution process will probably require some QA though I'd imagine, yes.

Thank you, in this case the ticket will be marked as validated only after we finish testing the attribution process.

(in reply to a few things)

I don't think adding a whitelist is useful at this time. Our fear here is that a malicious extension is alive on AMO, and then users are driven to it via an inserted value. [I don't see any risk with compromised builds, etc, since the downloads still come from where they've always come from.]

Rather than whitelisting a handful of extensions (which is overhead we don't want to deal with, at least right now), perhaps we could hash (via secret from AMO-only) some values to bundle as the ID passed to ensure that something at least came from AMO in the first place? Then the only risk is something "getting through" (though that is no more risk than having it on AMO).

(In reply to David Durst [:ddurst] from comment #53)

Rather than whitelisting a handful of extensions (which is overhead we don't want to deal with, at least right now), perhaps we could hash (via secret from AMO-only) some values to bundle as the ID passed to ensure that something at least came from AMO in the first place? Then the only risk is something "getting through" (though that is no more risk than having it on AMO).

That would work well I would think. You can generate an HMAC signature using a private key and then verify the signature when it comes back, and if it fails just log and redirect to AMO home (or whatever the team deems appropriate).

Rather than whitelisting a handful of extensions (which is overhead we don't want to deal with, at least right now), perhaps we could hash (via secret from AMO-only) some values to bundle as the ID passed to ensure that something at least came from AMO in the first place? Then the only risk is something "getting through" (though that is no more risk than having it on AMO).

That would work well I would think. You can generate an HMAC signature using a private key and then verify the signature when it comes back, and if it fails just log and redirect to AMO home (or whatever the team deems appropriate).

Actually, this won't work either, right? Because that signature is part of the querystring, all someone has to do is get something on AMO and then copy that value from the AMO link. Still, the most they can do is stuff that into attribution, and that value will subsequently be used to query the AMO API (and as such will return data that's on AMO or will return 400). So that is no more or less secure.

To ulfr's point:

links directly to /firefox/download/thanks/ since this system is about passing params from /firefox/new/ to /firefox/download/thanks/ and since OSX stores the value of the latter regardless

I'd like to clearly separate the two issues, because they seem to keep colliding:

  1. securing attribution on OSX (so that it's not just anything you stuff in a URL). I assume that vetting propagation of utm parameters (per #c38 and #c45) addresses that, though direct links are certainly an issue (and a correctly-structured-yet-fake direct link is not something attribution will really be able to differentiate from a real link);

  2. securing that the extension presented in RTAMO from attribution is "safe."

In terms of RTAMO, closing the loop from AMO visit -> Firefox install -> installing extension from AMO is the point. The problem in 1) seems addressed in this thread (?), 2) seems out of scope to solve via RTAMO.

The remaining questions then are:

a) can we tell when something bad has happened ("something bad" = a bad extension has somehow gotten through vetting and has been published on AMO and inordinately encouraged users to visit from non-Firefox and install Firefox and finish adding that extension to a new installation)?

b) can we turn it all off if something is amiss.

The answer to b) is yes, we have a server-side kill switch.

The answer to a) is arguably unknowable? Reducing this to a whitelist narrows the field, but does not solve the problem across the board.

(lastly, we could also just disable attribution for OSX, and RTAMO then would be a Windows-only feature)

(In reply to David Durst [:ddurst] from comment #55)

Actually, this won't work either, right? Because that signature is part of the querystring, all someone has to do is get something on AMO and then copy that value from the AMO link. Still, the most they can do is stuff that into attribution, and that value will subsequently be used to query the AMO API (and as such will return data that's on AMO or will return 400). So that is no more or less secure.

I was assuming that you'd only be providing signed links like that on certain manually approved addons. If these links will be signed and available for all addons then you're right, it would be pointless.

(In reply to Paul [:pmac] McLanahan ⏰ET needinfo? me from comment #57)

(In reply to David Durst [:ddurst] from comment #55)

Actually, this won't work either, right? Because that signature is part of the querystring, all someone has to do is get something on AMO and then copy that value from the AMO link. Still, the most they can do is stuff that into attribution, and that value will subsequently be used to query the AMO API (and as such will return data that's on AMO or will return 400). So that is no more or less secure.

I was assuming that you'd only be providing signed links like that on certain manually approved addons. If these links will be signed and available for all addons then you're right, it would be pointless.

Yeh ok, make sense, doesn't sound like there is much we can do here if we want to allow this flow from any add-on, rather than a whitelist.

My only other thought was for download page could check if the utm_source is addons.mozilla.org, we might check that the referer header actually was addons.mozilla.org. That would at least stop others directly linking to our download page with a copied link, but of course, then they could just link the AMO page itself to achieve the same result with one extra step.

OK, so we're going to whitelist extensions on the API side of things.

So links will remain populated with all the params (so attribution will not be disrupted, just adding the utm_content param), but the API will only respond when the request is from an extension on the whitelist.

No changes required in FF or Bedrock (this bug), change to AMO side off trains.

Great work :ddurst! Thanks!

Marking this ticket as verified based on comment #42 as the attribution process testing went smoothly and no major issues have been found on Nightly 66.0a1 - MacOS / Windows 10 x64.

Status: RESOLVED → VERIFIED
status-firefox66: --- → verified

(In reply to David Durst [:ddurst] from comment #59)

OK, so we're going to whitelist extensions on the API side of things.

So links will remain populated with all the params (so attribution will not be disrupted, just adding the utm_content param), but the API will only respond when the request is from an extension on the whitelist.

No changes required in FF or Bedrock (this bug), change to AMO side off trains.

For posterity, is it possible to link to the AMO change (is there a bug or something you can link to from here?

Flags: needinfo?(ddurst)

Yes: https://github.com/mozilla/addons-server/issues/10504

The resolution will be tracked via bug 1511173.

Flags: needinfo?(ddurst)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: