Skip to content

Commit

Permalink
Bug 1906734 - Switch WebExtension resources to use only static protoc…
Browse files Browse the repository at this point in the history
…ol flags, r=extension-reviewers,necko-reviewers,kershaw,robwu

Previously, the WebExtension protocol used dynamic protocol flags which
were based on the WebExtension policy in order to enforce things such as
availability in private browsing and the accessibility of certain
resources.

Since the shift to MV3, these checks have required more complex checks
than what was possible to specify with protocol flags, which required
the addition of WEBEXT_URI_WEB_ACCESSIBLE - a security flag which would
trigger further checks with the EPS to determine if the URI can be
loaded.

This was somewhat inefficient, as fetching the URI flags would require
looking up the policy each time dynamic flags were looked up, as well as
when policy specifics were being checked after loading flags. In
addition, it lead to a number of flags which were very specific to
extension protocols.

This patch changes extensions to no longer have dynamic flags, instead
specifying the static `URI_IS_WEBEXTENSION_RESOURCE` security flag. When
this flag is specified, security checks are made by querying the
ExtensionPolicyService to ask if the load should be permitted, combining
the specific security checks for Extension resources into a simpler
code-path, and avoids redundant checks.

Differential Revision: https://phabricator.services.mozilla.com/D216076
  • Loading branch information
mystor committed Jul 17, 2024
1 parent 26a5ddf commit 1eb3c1d
Show file tree
Hide file tree
Showing 11 changed files with 65 additions and 121 deletions.
24 changes: 12 additions & 12 deletions caps/BasePrincipal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -660,18 +660,18 @@ nsresult BasePrincipal::CheckMayLoadHelper(nsIURI* aURI,

// If the URL being loaded corresponds to a WebExtension URL, ask the policy
// if the path should be accessible.
//
// This is done directly, rather than first checking `NS_URIChainHasFlags` for
// `WEBEXT_URI_WEB_ACCESSIBLE`, as `CheckMayLoadHelper` may be called
// off-main-thread, and it is not safe to check dynamic URI flags
// off-main-thread. MV2 web-accessible resources also currently do not set
// `WEBEXT_URI_WEB_ACCESSIBLE`, and need to be allowed in this code.
extensions::URLInfo urlInfo(aURI);
if (RefPtr<extensions::WebExtensionPolicyCore> urlPolicyCore =
ExtensionPolicyService::GetCoreByURL(urlInfo)) {
extensions::URLInfo prinUrlInfo(prinURI);
if (urlPolicyCore->SourceMayAccessPath(prinUrlInfo, urlInfo.FilePath())) {
return NS_OK;
bool isWebExtensionResource;
rv = NS_URIChainHasFlags(aURI,
nsIProtocolHandler::URI_IS_WEBEXTENSION_RESOURCE,
&isWebExtensionResource);
if (NS_SUCCEEDED(rv) && isWebExtensionResource) {
extensions::URLInfo urlInfo(aURI);
if (RefPtr<extensions::WebExtensionPolicyCore> urlPolicyCore =
ExtensionPolicyService::GetCoreByURL(urlInfo)) {
extensions::URLInfo prinUrlInfo(prinURI);
if (urlPolicyCore->SourceMayAccessPath(prinUrlInfo, urlInfo.FilePath())) {
return NS_OK;
}
}
}

Expand Down
8 changes: 5 additions & 3 deletions caps/nsIAddonPolicyService.idl
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,12 @@ interface nsIAddonPolicyService : nsISupports
AString getExtensionName(in AString aAddonId);

/**
* Returns true if a given extension:// URI is web-accessible and loadable by the source.
* This should be called if the protocol flags for the extension URI has URI_WEB_ACCESSIBLE.
* Returns true if a given moz-extension:// URI is web-accessible and loadable by the source.
* This should be called if the protocol flags for the extension URI has
* URI_IS_WEBEXTENSION_RESOURCE.
*/
boolean sourceMayLoadExtensionURI(in nsIURI aSourceURI, in nsIURI aExtensionURI);
boolean sourceMayLoadExtensionURI(in nsIURI aSourceURI, in nsIURI aExtensionURI,
[optional] in boolean aFromPrivateWindow);

/**
* Maps an extension URI to the ID of the addon it belongs to.
Expand Down
36 changes: 11 additions & 25 deletions caps/nsScriptSecurityManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -971,8 +971,7 @@ nsresult nsScriptSecurityManager::CheckLoadURIFlags(
nsresult rv = aTargetBaseURI->GetScheme(targetScheme);
if (NS_FAILED(rv)) return rv;

// Check for system target URI. Regular (non web accessible) extension
// URIs will also have URI_DANGEROUS_TO_LOAD.
// Check for system target URI.
rv = DenyAccessIfURIHasFlags(aTargetURI,
nsIProtocolHandler::URI_DANGEROUS_TO_LOAD);
if (NS_FAILED(rv)) {
Expand All @@ -984,31 +983,18 @@ nsresult nsScriptSecurityManager::CheckLoadURIFlags(
return rv;
}

// Used by ExtensionProtocolHandler to prevent loading extension resources
// in private contexts if the extension does not have permission.
if (aFromPrivateWindow) {
rv = DenyAccessIfURIHasFlags(
aTargetURI, nsIProtocolHandler::URI_DISALLOW_IN_PRIVATE_CONTEXT);
if (NS_FAILED(rv)) {
if (reportErrors) {
ReportError(errorTag, aSourceURI, aTargetURI, aFromPrivateWindow,
aInnerWindowID);
}
return rv;
}
}

// If MV3 Extension uris are web accessible they have
// WEBEXT_URI_WEB_ACCESSIBLE.
bool maybeWebAccessible = false;
NS_URIChainHasFlags(aTargetURI, nsIProtocolHandler::WEBEXT_URI_WEB_ACCESSIBLE,
&maybeWebAccessible);
// WebExtension URIs are only accessible if the ExtensionPolicyService allows
// the source URI to load them.
bool targetURIIsWebExtensionResource = false;
rv = NS_URIChainHasFlags(aTargetURI,
nsIProtocolHandler::URI_IS_WEBEXTENSION_RESOURCE,
&targetURIIsWebExtensionResource);
NS_ENSURE_SUCCESS(rv, rv);
if (maybeWebAccessible) {
bool isWebAccessible = false;
if (targetURIIsWebExtensionResource) {
bool isAccessible = false;
rv = ExtensionPolicyService::GetSingleton().SourceMayLoadExtensionURI(
aSourceURI, aTargetURI, &isWebAccessible);
if (NS_SUCCEEDED(rv) && isWebAccessible) {
aSourceURI, aTargetURI, aFromPrivateWindow, &isAccessible);
if (NS_SUCCEEDED(rv) && isAccessible) {
return NS_OK;
}
if (reportErrors) {
Expand Down
2 changes: 1 addition & 1 deletion dom/security/nsContentSecurityManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1131,7 +1131,7 @@ nsresult nsContentSecurityManager::CheckChannelHasProtocolSecurityFlag(
NS_ENSURE_SUCCESS(rv, rv);

uint32_t securityFlagsSet = 0;
if (flags & nsIProtocolHandler::WEBEXT_URI_WEB_ACCESSIBLE) {
if (flags & nsIProtocolHandler::URI_IS_WEBEXTENSION_RESOURCE) {
securityFlagsSet += 1;
}
if (flags & nsIProtocolHandler::URI_LOADABLE_BY_ANYONE) {
Expand Down
39 changes: 19 additions & 20 deletions netwerk/base/nsIProtocolHandler.idl
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ interface nsIProtocolHandler : nsISupports
/**
* +-------------------------------------------------------------------+
* | |
* | ALL PROTOCOL HANDLERS MUST SET ONE OF THE FOLLOWING FIVE FLAGS. |
* | ALL PROTOCOL HANDLERS MUST SET ONE OF THE FOLLOWING SIX FLAGS. |
* | |
* +-------------------------------------------------------------------+
*
Expand All @@ -155,6 +155,7 @@ interface nsIProtocolHandler : nsISupports
* * URI_IS_UI_RESOURCE
* * URI_IS_LOCAL_FILE
* * URI_LOADABLE_BY_SUBSUMERS
* * URI_IS_WEBEXTENSION_RESOURCE
*
* These flags are used to determine who is allowed to load URIs for this
* protocol. Note that if a URI is nested, only the flags for the
Expand Down Expand Up @@ -251,59 +252,57 @@ interface nsIProtocolHandler : nsISupports
*/
const unsigned long URI_IS_POTENTIALLY_TRUSTWORTHY = (1<<17);

/**
* The URI corresponds to a WebExtension resource (i.e. moz-extension://).
* If this flag is set, the ExtensionPolicyService must be consulted to
* determine whether loading this URI is allowed.
*/
const unsigned long URI_IS_WEBEXTENSION_RESOURCE = (1<<18);

/**
* If this flag is set, then the origin for this protocol is the full URI
* spec, not just the scheme + host + port.
*
* Note: this is not supported in Firefox. It is currently only available
* in Thunderbird and SeaMonkey.
*/
const unsigned long ORIGIN_IS_FULL_SPEC = (1 << 19);
const unsigned long ORIGIN_IS_FULL_SPEC = (1<<19);

/**
* If this flag is set, the URI does not always allow content using the same
* protocol to link to it.
*/
const unsigned long URI_SCHEME_NOT_SELF_LINKABLE = (1 << 20);
const unsigned long URI_SCHEME_NOT_SELF_LINKABLE = (1<<20);

/**
* The URIs for this protocol can be loaded by extensions.
*/
const unsigned long URI_LOADABLE_BY_EXTENSIONS = (1 << 21);

/**
* The URIs for this protocol can not be loaded into private contexts.
*/
const unsigned long URI_DISALLOW_IN_PRIVATE_CONTEXT = (1 << 22);
const unsigned long URI_LOADABLE_BY_EXTENSIONS = (1<<21);

/**
* This protocol handler forbids accessing cookies e.g. for mail related
* protocols. Only used in Mailnews (comm-central).
*/
const unsigned long URI_FORBIDS_COOKIE_ACCESS = (1 << 23);

/**
* This is an extension web accessible uri that is loadable if checked
* against an allowlist using ExtensionPolicyService::SourceMayLoadExtensionURI.
*/
const unsigned long WEBEXT_URI_WEB_ACCESSIBLE = (1 << 24);
const unsigned long URI_FORBIDS_COOKIE_ACCESS = (1<<22);

/**
* This URI has a webexposed origin, meaning the URI has a non-null origin
* See https://url.spec.whatwg.org/#origin
*/
const unsigned long URI_HAS_WEB_EXPOSED_ORIGIN = (1 << 25);
const unsigned long URI_HAS_WEB_EXPOSED_ORIGIN = (1<<23);

/**
* Flags which are allowed to be different from the static flags when
* returned from `nsIProtocolHandlerWithDynamicFlags::getFlagsForURI`.
*
* All other flags must match the flags provided when the protocol handler
* was registered.
*
* The following protocols are the reasons why each flag is dynamic:
* about: URI_LOADABLE_BY_ANYONE, URI_DANGEROUS_TO_LOAD, URI_IS_POTENTIALLY_TRUSTWORTHY
* view-source: URI_LOADABLE_BY_EXTENSIONS
*/
const unsigned long DYNAMIC_URI_FLAGS =
URI_LOADABLE_BY_ANYONE | URI_DANGEROUS_TO_LOAD |
URI_IS_POTENTIALLY_TRUSTWORTHY | URI_LOADABLE_BY_EXTENSIONS |
URI_DISALLOW_IN_PRIVATE_CONTEXT | WEBEXT_URI_WEB_ACCESSIBLE |
URI_HAS_WEB_EXPOSED_ORIGIN;
URI_IS_POTENTIALLY_TRUSTWORTHY | URI_LOADABLE_BY_EXTENSIONS;
};
2 changes: 1 addition & 1 deletion netwerk/build/components.conf
Original file line number Diff line number Diff line change
Expand Up @@ -400,8 +400,8 @@ Classes = [
'URI_IS_LOCAL_RESOURCE',
'URI_IS_POTENTIALLY_TRUSTWORTHY',
'URI_HAS_WEB_EXPOSED_ORIGIN',
'URI_IS_WEBEXTENSION_RESOURCE',
],
'has_dynamic_flags': True,
},
},
{
Expand Down
46 changes: 0 additions & 46 deletions netwerk/protocol/res/ExtensionProtocolHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,6 @@ void ExtensionStreamGetter::OnFD(const FileDescriptor& aFD) {

NS_IMPL_QUERY_INTERFACE(ExtensionProtocolHandler,
nsISubstitutingProtocolHandler, nsIProtocolHandler,
nsIProtocolHandlerWithDynamicFlags,
nsISupportsWeakReference)
NS_IMPL_ADDREF_INHERITED(ExtensionProtocolHandler, SubstitutingProtocolHandler)
NS_IMPL_RELEASE_INHERITED(ExtensionProtocolHandler, SubstitutingProtocolHandler)
Expand Down Expand Up @@ -409,51 +408,6 @@ static inline ExtensionPolicyService& EPS() {
return ExtensionPolicyService::GetSingleton();
}

nsresult ExtensionProtocolHandler::GetFlagsForURI(nsIURI* aURI,
uint32_t* aFlags) {
uint32_t flags =
URI_STD | URI_IS_LOCAL_RESOURCE | URI_IS_POTENTIALLY_TRUSTWORTHY;

URLInfo url(aURI);
if (auto* policy = EPS().GetByURL(url)) {
// In general a moz-extension URI is only loadable by chrome, but an
// allowlist subset are web-accessible (and cross-origin fetchable).
// The allowlist is checked using EPS.SourceMayLoadExtensionURI in
// nsScriptSecurityManager, and WEPC.SourceMayAccessPath in BasePrincipal.
if (policy->IsWebAccessiblePath(url.FilePath())) {
if (policy->ManifestVersion() < 3) {
// We could also be `WEBEXT_URI_WEB_ACCESSIBLE` here, but this would
// just check `IsWebAccessiblePath` for Manifest V2.
// nsIPrincipal::CheckMayLoad (which is used for resources) already
// directly queries the WebExtensionPolicy, so we only need to set
// URI_LOADABLE_BY_ANYONE to allow navigation loads.
flags |= URI_LOADABLE_BY_ANYONE;
} else {
flags |= WEBEXT_URI_WEB_ACCESSIBLE;
}
} else if (policy->Type() == nsGkAtoms::theme) {
// Static themes cannot set web accessible resources, however using this
// flag here triggers SourceMayAccessPath calls necessary to allow another
// extension to access static theme resources in this extension.
flags |= WEBEXT_URI_WEB_ACCESSIBLE;
} else {
flags |= URI_DANGEROUS_TO_LOAD;
}

// Disallow in private windows if the extension does not have permission.
if (!policy->PrivateBrowsingAllowed()) {
flags |= URI_DISALLOW_IN_PRIVATE_CONTEXT;
}
} else {
// In case there is no policy, then default to treating moz-extension URIs
// as unsafe and generally only allow chrome: to load such.
flags |= URI_DANGEROUS_TO_LOAD;
}

*aFlags = flags;
return NS_OK;
}

bool ExtensionProtocolHandler::ResolveSpecialCases(const nsACString& aHost,
const nsACString& aPath,
const nsACString& aPathname,
Expand Down
9 changes: 3 additions & 6 deletions netwerk/protocol/res/ExtensionProtocolHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,11 @@ namespace net {

class ExtensionStreamGetter;

class ExtensionProtocolHandler final
: public nsISubstitutingProtocolHandler,
public nsIProtocolHandlerWithDynamicFlags,
public SubstitutingProtocolHandler,
public nsSupportsWeakReference {
class ExtensionProtocolHandler final : public nsISubstitutingProtocolHandler,
public SubstitutingProtocolHandler,
public nsSupportsWeakReference {
public:
NS_DECL_ISUPPORTS_INHERITED
NS_DECL_NSIPROTOCOLHANDLERWITHDYNAMICFLAGS
NS_FORWARD_NSIPROTOCOLHANDLER(SubstitutingProtocolHandler::)
NS_FORWARD_NSISUBSTITUTINGPROTOCOLHANDLER(SubstitutingProtocolHandler::)

Expand Down
6 changes: 4 additions & 2 deletions toolkit/components/extensions/ExtensionPolicyService.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -735,11 +735,13 @@ nsresult ExtensionPolicyService::GetExtensionName(const nsAString& aAddonId,
}

nsresult ExtensionPolicyService::SourceMayLoadExtensionURI(
nsIURI* aSourceURI, nsIURI* aExtensionURI, bool* aResult) {
nsIURI* aSourceURI, nsIURI* aExtensionURI, bool aFromPrivateWindow,
bool* aResult) {
URLInfo source(aSourceURI);
URLInfo url(aExtensionURI);
if (RefPtr<WebExtensionPolicyCore> policy = GetCoreByURL(url)) {
*aResult = policy->SourceMayAccessPath(source, url.FilePath());
*aResult = (!aFromPrivateWindow || policy->PrivateBrowsingAllowed()) &&
policy->SourceMayAccessPath(source, url.FilePath());
return NS_OK;
}
return NS_ERROR_INVALID_ARG;
Expand Down
8 changes: 4 additions & 4 deletions toolkit/components/extensions/WebExtensionPolicy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,10 @@ bool WebExtensionPolicyCore::QuarantinedFromURI(const URLInfo& aURI) const {
return !IgnoreQuarantine() && WebExtensionPolicy::IsQuarantinedURI(aURI);
}

bool WebExtensionPolicyCore::PrivateBrowsingAllowed() const {
return HasPermission(nsGkAtoms::privateBrowsingAllowedPermission);
}

/*****************************************************************************
* WebExtensionPolicy
*****************************************************************************/
Expand Down Expand Up @@ -600,10 +604,6 @@ void WebExtensionPolicy::GetContentScripts(
aScripts.AppendElements(mContentScripts);
}

bool WebExtensionPolicy::PrivateBrowsingAllowed() const {
return HasPermission(nsGkAtoms::privateBrowsingAllowedPermission);
}

bool WebExtensionPolicy::CanAccessContext(nsILoadContext* aContext) const {
MOZ_ASSERT(aContext);
return PrivateBrowsingAllowed() || !aContext->UsePrivateBrowsing();
Expand Down
6 changes: 5 additions & 1 deletion toolkit/components/extensions/WebExtensionPolicy.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ class WebExtensionPolicyCore final {
bool QuarantinedFromDoc(const DocInfo& aDoc) const;
bool QuarantinedFromURI(const URLInfo& aURI) const MOZ_EXCLUDES(mLock);

bool PrivateBrowsingAllowed() const;

// Try to get a reference to the cycle-collected main-thread-only
// WebExtensionPolicy instance.
//
Expand Down Expand Up @@ -320,7 +322,9 @@ class WebExtensionPolicy final : public nsISupports, public nsWrapperCache {
bool Active() const { return mActive; }
void SetActive(bool aActive, ErrorResult& aRv);

bool PrivateBrowsingAllowed() const;
bool PrivateBrowsingAllowed() const {
return mCore->PrivateBrowsingAllowed();
}

bool CanAccessContext(nsILoadContext* aContext) const;

Expand Down

0 comments on commit 1eb3c1d

Please sign in to comment.