Closed Bug 1826651 Opened 1 year ago Closed 1 year ago

[DNR] Explicitly ignore subresources from documents that are not accessible to extensions

Categories

(WebExtensions :: Request Handling, enhancement)

enhancement

Tracking

(firefox113 fixed, firefox114 fixed)

RESOLVED FIXED
114 Branch
Tracking Status
firefox113 --- fixed
firefox114 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [addons-jira])

Attachments

(1 file)

In webRequest, extensions may only modify requests when they have the host permissions to access to the request URL and to access to the document URL, if any. This is an allowlist-based approach.

In DNR, the declarativeNetRequest permission is meant to offer request blocking functionality without host permission checks. Anything more than just blocking (redirect / modifyHeaders) requires host permissions. DNR currently uses a deny-based approach: Consistent with webRequest, DNR already ignores system requests and requests to restricted domains. And with bug 1810753, requests from other extensions are blocked too. But e.g. about:-URLs are not explicitly rejected.

As a result, if I open a new tab (about:newtab) after installing a DNR extension with rules, an attempt to evaluate the DNR rules occurs. Because about:newtab is unexpected, an error is thrown when the unexpected URL is encountered in #domainFromURI for the initiatorURI (triggeringPrincipal):

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://gre/modules/ExtensionDNR.sys.mjs :: #domainFromURI :: line 1160" data: no]

Instead of this accidental implementation detail, we should just filter out URIs whose schemes are certainly not known to be available to extensions. Note: opaque initiators are out of scope here (handled in bug 1798225).

  • Let DNR ignore not-explicitly-allowed schemes (e.g. about:) without
    logging an error in the console.
  • Improve testMatchOutcome to explicitly ignore unsupported schemes,
    to be consistent with what we do for evaluations from the network.

FYI: Chrome's declarativeNetRequest.testMatchOutcome method does not provide any access checks at all. I'd argue that to be a mistake, since the testMatchOutcome method is supposed to show whether a hypothetical request would match any rule.

This can be verified (e.g. in Chrome 111) by running the following from an unpacked extension that has the declarativeNetRequest permission. (to test in Firefox for comparison: also add the "declarativeNetRequestFeedback" permission + set the extensions.dnr.feedback pref to true).

await chrome.declarativeNetRequest.updateSessionRules({
  addRules: [{ id: 1, condition: {}, action: { type: "block" } }],
});
console.log(await chrome.declarativeNetRequest.testMatchOutcome({ type: "other", url: "chrome://extensions" });
console.log(await chrome.declarativeNetRequest.testMatchOutcome({ type: "other", url: "http://a", initiator: "chrome://extensions" });
console.log(await chrome.declarativeNetRequest.testMatchOutcome({ type: "other", url: "this-is:completely/bogus" });
Blocks: 1827422
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/f177c2c74131
Explicitly check other schemes in DNR r=rpl

Backed out for causing DNR related failures.

Flags: needinfo?(rob)

The new xpcshell test failed on specific test configurations because it loads about:logo, but the xpcshell test utilities unconditionally load a data:text/javascript,// frame script, which triggers a crash in nsContentSecurityUtils::ValidateScriptFilename,

  • Hit MOZ_CRASH(Blocking a script load data:text/javascript,// from file (None))

... or otherwise at least an error message in MaybeValidateFilename:

  • "InternalError: unsafe filename: data:text/javascript,//"

While this observation is understandable given the circumstances, it is not helpful in the test. Therefore I am going to reland the test with security.allow_parent_unrestricted_js_loads=true for the specific test task.

Flags: needinfo?(rob)
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/739bfe1bfc6b
Explicitly check other schemes in DNR r=rpl
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 114 Branch

Comment on attachment 9327256 [details]
Bug 1826651 - Explicitly check other schemes in DNR

Beta/Release Uplift Approval Request

  • User impact if declined: DNR evaluation engine tries to match more requests than intended; If the user installs an extension using DNR and opens a new tab (about:newtab), the console contains some logspam.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Change to specific part of extension API, fully covered by unit tests. The API was enabled in Firefox 113 (currently Beta).
  • String changes made/needed: None
  • Is Android affected?: Yes
Attachment #9327256 - Flags: approval-mozilla-beta?

Documentation covered by https://github.com/mdn/content/pull/26189, specifically the part

Some requests are restricted and cannot be matched by extensions. That includes privileged browser requests, requests to or from [restricted domains](/en-US/docs/Mozilla/Add-ons/WebExtensions/Content_scripts#restricted_domains), and requests from other extensions.

Comment on attachment 9327256 [details]
Bug 1826651 - Explicitly check other schemes in DNR

Approved for 113 beta 4, thanks.

Attachment #9327256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: