1610844 - Include navigated page in add-on abuse report

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement, P1)

Description

[Andreas Wagner [:TheOne] [use NI]]

The add-on abuse report already contains the location of the XPI when it was installed. For webpage-based installs, that XPI origin is often very different from the page that initiated the install (= the page the user saw when installing the add-on).

In order to effectively fight abuse and analyze and block those malicious install pages, we need to add what page triggered the install to add-on abuse reports.

Comment 1

[Andreas Wagner [:TheOne] [use NI]]

Attachment: Data review request

Comment 2

[Chris H-C :chutten]

Comment on attachment 9122627 [details]
Data review request

PRELIMINARY NOTE:

I presume this additional data collection will be documented in the [Abuse Reports docs](https://addons-server.readthedocs.io/en/latest/topics/api/abuse.html). Andreas to confirm.

Also, we need an individual's name as the "responsible person" for permanent collections. "The add-ons security operations team" is too general. (will it be Stuart Colville like the other abuse report collections?)

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is documented in the [add-ons Abuse Reports docs](https://addons-server.readthedocs.io/en/latest/topics/api/abuse.html).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is user-initiated.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, the Addons Security Operations Team is responsible. (individual's name forthcoming)

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 3, Web Activity.

    Is the data collection request for default-on or default-off?

Default off.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---
Result: datareview+, pending the two clarifications mentioned above.

Comment 3

[Andreas Wagner [:TheOne] [use NI]]

(In reply to Chris H-C :chutten from comment #2)

Comment on attachment 9122627 [details]
I presume this additional data collection will be documented in the Abuse
Reports
docs.
Andreas to confirm.

Yes, we will update the documentation with the changes on the server-side that retrieve and store that info.

Also, we need an individual's name as the "responsible person" for permanent
collections. "The add-ons security operations team" is too general. (will it
be Stuart Colville like the other abuse report collections?)

I talked to Stuart and he is ok with that.

Thank you, Chris!

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1610844 - Store xpi install triggering url into the addon db when available. r?mixedpuppy!

This patch ensures that we store in the addon db the url that has triggered an xpi file to install,
when available (e.g. when the xpi file installation has been triggered from a website using the
installTrigger or mozAddonManager APIs, or when a webpage is navigated to an xpi file url).

The url collected is never sent as part of the addons telemetry events, but in a follow up patch
it is going to be included in the data submitted for an abuse report of the related addon.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1610844 - Submit the xpi install triggering url as part of submitted abuse reports. r?mixedpuppy!

This patch adds the "xpi install triggering url" to the data submitted using the Firefox integrated abuse report
(The data review related to the additional addon_install_source_url property is attached to the bugzilla issue).

Depends on D62264

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ba58bc19438
Store xpi install triggering url into the addon db when available. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/eac0c30624e1
Submit the xpi install triggering url as part of submitted abuse reports. r=mixedpuppy

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Marking this explicitly as qe-verify+

Once this change has reached a nightly build and the related change on the server side (tracked by https://github.com/mozilla/addons-server/issues/13465) has reached addons-dev (which may already be the case), we should verify that the expected addon_install_source_url is being submitted to the API endpoint.

STR to verify this change:

0. change "extensions.abuseReport.url" to ensure to submit the reports to the addons-dev server
   (also set "extensions.webapi.testing" to true if reporting or installing the extension from addons-dev)
1. install one signed extension from a addons-dev (and possibly one from a third party website, even just a website that navigates to an xpi stored elsewhere when a link is clicked)
2. submit an abuse report for the extension (from about:addons and/or from addons-dev)
3. verify that the submitted abuse report data includes the addon_install_source_url property (and that the value is the url of the webpage that originated the installation)

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3ba58bc19438
https://hg.mozilla.org/mozilla-central/rev/eac0c30624e1

Comment 9

[Victor Carciu]

Verified as fixed in 75.0a1 (2020-02-24) using Windows 10x64 and MacOS 10.15.

The submitted abuse report data includes the addon_install_source_url property and that the value is the url of the webpage that originated the installation.

Thanks.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9125521 [details]
Bug 1610844 - Store xpi install triggering url into the addon db when available. r?mixedpuppy!

Beta/Release Uplift Approval Request

• User impact if declined: Without this change, the abuse reports submitted by the users would not yet include the url of the website from where the extension installation has been triggered.

The users wouldn't notice the reduced details sent by the report panel, but the missing property would prevent the AMO admins (and addon reviewers) to investigate further the potentially malicious extension.

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Same STR provided in Comment 7
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): These two patches include:

• small changes to the AddonManager internals (in particular the internals related to installing an extension from the web), to collect the additional detail and store it in the addon db
• minimal changes to the AbuseReporter internals, to add the additional addon_install_source_url property to the details sent to the AMO API endpoint (when available)

Both the patches are also adding additional assertions to the existing tests to make sure that the feature doesn't regress (most of the changes in the two patches are actually the changes to the test cases).

• String changes made/needed:

Comment 11

[Pascal Chevrel:pascalc]

Comment on attachment 9125521 [details]
Bug 1610844 - Store xpi install triggering url into the addon db when available. r?mixedpuppy!

Low risk, has tests, verified by QA on nightly, uplift approved for 74.0b8, thanks.

Comment 12

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/7eaf5ff4a7e0
https://hg.mozilla.org/releases/mozilla-beta/rev/b3a3b51891c3

Comment 13

[Victor Carciu]

Verified as fixed in Beta using FF 74.0b9 (64-bit)

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1610844 - Store xpi install triggering url into the addon db when available. r=mixedpuppy

This patch ensures that we store in the addon db the url that has triggered an xpi file to install,
when available (e.g. when the xpi file installation has been triggered from a website using the
installTrigger or mozAddonManager APIs, or when a webpage is navigated to an xpi file url).

The url collected is never sent as part of the addons telemetry events, but in a follow up patch
it is going to be included in the data submitted for an abuse report of the related addon.

(Patch rebased and small tweaks applied to make it apply cleanly on ESR68)

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1610844 - Submit the xpi install triggering url as part of submitted abuse reports. r=mixedpuppy

This patch adds the "xpi install triggering url" to the data submitted using the Firefox integrated abuse report
(The data review related to the additional addon_install_source_url property is attached to the bugzilla issue).

(Patch rebased and small tweaks applied to make it apply cleanly on ESR68)

Depends on D64634

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

D64634 and D64635 contains the same changes we applied on Nightly (75) and Beta (74), rebased (and slightly adapted) to apply cleanly on ESR 68 (in preparation for an uplift request for ESR 68).

Both the patches are mostly unmodified, besides:

• small tweaks to adapt the actual change to the older version of those files
• some additional tweaks for the test cases:
  • in D64634 due to the tests being reformatted by prettier in the meantime
  • in D64635 due to a refactoring of the test_AbuseReporter.jsm test file that we applied in a patch that isn't on ESR 68).

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9129576 [details]
Bug 1610844 - Store xpi install triggering url into the addon db when available. r=mixedpuppy

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The new detail collected in this patches is meant to help the AMO admins to investigate issues related to extensions that a user is reporting as suspicious (in particular the additional url can help the AMO admins to get more insight about which webpage has installed the suspicious addon installated).

Given that the ESR version stays around for a longer time, the AMO admins would like to make sure that we don't miss these additional details for users that are on the ESR channel.

We recognize that this may not be critical enough to justify an uplift to ESR, but we discussed about this in our standup meeting and agreed to prepare the patches and leave to release management to decide if it would still be acceptable for an "ESR uplift" or not.

• User impact if declined: same user impact mentioned in the uplift request to beta)
• Fix Landed on Version: 75 (and uplifted to 74)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change should be low risk (for the same reasons mentioned in the uplift request to beta).

As a side note: as mentioned in comment 16, there two patches are a slightly adapted version of the two patches we landed on 75 (and then uplifted to 74).

The amount and kind of changes applied to resolve the merge conflicts do not seem to be big enough to be changing the risk level for these two patches.

• String or UUID changes made by this patch:

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9129576 [details]
Bug 1610844 - Store xpi install triggering url into the addon db when available. r=mixedpuppy

Approved for 68.6esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9129576 [details]
Bug 1610844 - Store xpi install triggering url into the addon db when available. r=mixedpuppy

This doesn't apply cleanly to ESR68. Please attach revised patches.

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

My apologies, it seems that I missed to update the esr68 branch in my mozilla-unified clone to the actual last commit before adapting those two patches

I've just updated both D64634 and D64635, they should now apply cleanly on the last commit on esr68 (the real last one this time).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9129576 [details]
Bug 1610844 - Store xpi install triggering url into the addon db when available. r=mixedpuppy

Thanks for rebasing. Re-approved for 68.6esr.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/cc1a281e9ed5
https://hg.mozilla.org/releases/mozilla-esr68/rev/02d601aa6cb8

Comment 23

[Victor Carciu]

Verified as fixed in ESR 68.6.0esr (64-bit)