Closed Bug 1610844 Opened 2 months ago Closed 2 months ago

Include navigated page in add-on abuse report

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
mozilla75
Tracking Status
firefox-esr68 --- verified
firefox74 --- verified
firefox75 --- verified

People

(Reporter: TheOne, Assigned: rpl)

References

Details

Attachments

(5 files)

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.

Attached file Data review request
Attachment #9122627 - Flags: data-review?(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.
Flags: needinfo?(awagner)
Attachment #9122627 - Flags: data-review?(chutten) → data-review+

(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!

Flags: needinfo?(awagner)
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Priority: -- → P1

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.

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

Attachment #9125522 - Attachment description: Bug 1610844 - Submit the xpi install triggering url as part of submitted abuse reports. r?mixedpuppy!,janerik! → Bug 1610844 - Submit the xpi install triggering url as part of submitted abuse reports. r?mixedpuppy!
Pushed by luca.greco@alcacoop.it:
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

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:

  1. 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)
  2. 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)
  3. submit an abuse report for the extension (from about:addons and/or from addons-dev)
  4. 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)
Flags: qe-verify+
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

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:
Attachment #9125521 - Flags: approval-mozilla-beta?
Attachment #9125522 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Attachment #9125521 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9125522 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: qe-verify+

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)

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

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 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.

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:
Attachment #9129576 - Flags: approval-mozilla-esr68?
Attachment #9129577 - Flags: approval-mozilla-esr68?

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

Approved for 68.6esr.

Attachment #9129576 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9129577 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

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.

Flags: needinfo?(lgreco)
Attachment #9129576 - Flags: approval-mozilla-esr68+
Attachment #9129577 - Flags: approval-mozilla-esr68+

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).

Flags: needinfo?(lgreco)

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.

Attachment #9129576 - Flags: approval-mozilla-esr68+
Attachment #9129577 - Flags: approval-mozilla-esr68+

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

You need to log in before you can comment on or make changes to this bug.