Closed Bug 1277524 Opened 3 years ago Closed 3 years ago

Password security error is raised if a moz-extension:// page contains a password field

Categories

(WebExtensions :: Untriaged, defect, P5)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: rpl, Assigned: johannh)

Details

(Whiteboard: triaged)

Attachments

(2 files)

Currently the following security error is logged if a WebExtension page contains a password field:

> Password fields present on an insecure (http://) page. 
> This is a security risk that allows user login credentials to be stolen.

input tags with type password are probably common in a WebExtension options_ui page (e.g. Mailvelope Chrome-version of the addon uses a password field in its options page), and the above security error is going to be logged every time the options page is opened (but besides the logged security error, the password field should work as expected).
It seems reasonable that moz-extension: could be potentially trustworthy origin, see https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy.

I'll look into this, maybe we should get more opinions from security folks.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
So after talking with some people in the office I think I'll go ahead with a patch here. Tanvi, can you take a look at it?
https://reviewboard.mozilla.org/r/57224/#review54098

::: dom/security/nsContentSecurityManager.cpp:627
(Diff revision 1)
>    if (scheme.EqualsLiteral("https") ||
>        scheme.EqualsLiteral("file") ||
>        scheme.EqualsLiteral("resource") ||
>        scheme.EqualsLiteral("app") ||
> +      scheme.EqualsLiteral("moz-extension") ||
>        scheme.EqualsLiteral("wss")) {

Is there a particular reason we can't check for the `URI_IS_LOCAL_RESOURCE` or `URI_IS_LOCAL_FILE` flag rather than listing all of these explicitly?

Either way, we should add a WebExtension test to check that `isOriginPotentiallyTrustworthy` returns true for URLs returned by `runtime.getURL`.
https://reviewboard.mozilla.org/r/57224/#review54098

> Is there a particular reason we can't check for the `URI_IS_LOCAL_RESOURCE` or `URI_IS_LOCAL_FILE` flag rather than listing all of these explicitly?
> 
> Either way, we should add a WebExtension test to check that `isOriginPotentiallyTrustworthy` returns true for URLs returned by `runtime.getURL`.

Seems to me listing them explicitly is the proper way of doing it, I suppose because this is an implementation of the W3C spec. Tanvi can probably tell us more.
Priority: -- → P5
Whiteboard: triaged
Comment on attachment 8759589 [details]
Bug 1277524 - Add a WebExtension test that moz-extension is considered a trustworthy origin.

https://reviewboard.mozilla.org/r/57544/#review54404

Looks good. Thanks!

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_trustworthy_origin.html:17
(Diff revision 1)
> +
> +<script type="text/javascript">
> +"use strict";
> +const {
> +  utils: Cu,
> +} = Components;

Nit: We're starting to have a lot of chrome mochitests. Can you add a head_chrome.js script that defines Cc/Ci/Cu/Cr, and defines lazy module getters for Services and NetUtil?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_trustworthy_origin.html:28
(Diff revision 1)
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "gScriptSecurityManager",
> +                                   "@mozilla.org/scriptsecuritymanager;1",
> +                                   "nsIScriptSecurityManager");

This is available as `Services.scriptSecurityManager`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_trustworthy_origin.html:44
(Diff revision 1)
> +  background: "(" + backgroundScript.toString() + ")()",
> +  manifest: {},
> +  files: {
> +    "test.html": `<html><head></head><body></body></html>`,
> +  },
> +};

Please move `extensionData` and `backgroundScript` into the task function.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_trustworthy_origin.html:49
(Diff revision 1)
> +};
> +
> +add_task(function* () {
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +  yield extension.startup();
> +  info("extension loaded");

Nit: We've moved away from adding extension loaded/extension unloaded info messages to newer tests.
Attachment #8759589 - Flags: review?(kmaglione+bmo) → review+
https://reviewboard.mozilla.org/r/57224/#review54098

> Seems to me listing them explicitly is the proper way of doing it, I suppose because this is an implementation of the W3C spec. Tanvi can probably tell us more.

The spec leaves us a lot of leeway here: ‘If origin's scheme component is one which the user agent considers to be authenticated, return "Potentially Trustworthy"’. The protocol flags already exist to give us this kind of information, so I don't see any benefit to having to repeat it in multiple places.
I won't be reviewing this anytime soon.  If it becomes a priority, please let me know so that I can review sooner than later.  Thanks!
Comment on attachment 8759589 [details]
Bug 1277524 - Add a WebExtension test that moz-extension is considered a trustworthy origin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57544/diff/1-2/
Comment on attachment 8759589 [details]
Bug 1277524 - Add a WebExtension test that moz-extension is considered a trustworthy origin.

Thanks for the r+, but can you give it another look to make sure I didn't mess up with the chrome_head.js stuff? :)
Attachment #8759589 - Flags: review+ → review?(kmaglione+bmo)
Comment on attachment 8759589 [details]
Bug 1277524 - Add a WebExtension test that moz-extension is considered a trustworthy origin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57544/diff/2-3/
Comment on attachment 8759589 [details]
Bug 1277524 - Add a WebExtension test that moz-extension is considered a trustworthy origin.

https://reviewboard.mozilla.org/r/57544/#review55576

::: toolkit/components/extensions/test/mochitest/chrome_head.js:6
(Diff revision 3)
> +"use strict";
> +
> +const {
> +  classes: Cc,
> +  interfaces: Ci,
> +  utils: Cu,

We should also add `results: Cr,` since people will assume it's defined.
Attachment #8759589 - Flags: review?(kmaglione+bmo) → review+
(In reply to Johann Hofmann [:johannh] from comment #5)
> https://reviewboard.mozilla.org/r/57224/#review54098
> 
> > Is there a particular reason we can't check for the `URI_IS_LOCAL_RESOURCE` or `URI_IS_LOCAL_FILE` flag rather than listing all of these explicitly?
> > 
> > Either way, we should add a WebExtension test to check that `isOriginPotentiallyTrustworthy` returns true for URLs returned by `runtime.getURL`.
> 
> Seems to me listing them explicitly is the proper way of doing it, I suppose
> because this is an implementation of the W3C spec. Tanvi can probably tell
> us more.

What does it mean for a resource to have a moz-extension:// uri?  Does it mean that the resource is stored locally in the extension code?  Do moz-extension:// uris result in a network request?  If not and if it truly is local, then URI_IS_LOCAL_RESOURCE would be fine - http://searchfox.org/mozilla-central/source/netwerk/base/nsIProtocolHandler.idl#249.  We could add that flag to moz-extension to fix this bug.  If moz-extension:// uris can be used as subresources, there could also be potential mixed content blocker issues with moz-extension without this flag - http://searchfox.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#514

As far isOriginPotentiallyTrustworthy though, that doesn't appear to use URI flags.  Matt or Jonathan Watt may know why.

http://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#600
Comment on attachment 8759194 [details]
Bug 1277524 - Add moz-extension to the list of potentially trustworthy origins.

https://reviewboard.mozilla.org/r/57224/#review60166

This looks fine as is, but depending on the answers to comment 14, we may also need a protocol flag on moz-extension://
Attachment #8759194 - Flags: review?(tanvi) → review+
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #14)
> (In reply to Johann Hofmann [:johannh] from comment #5)
> > https://reviewboard.mozilla.org/r/57224/#review54098
> > 
> > > Is there a particular reason we can't check for the `URI_IS_LOCAL_RESOURCE` or `URI_IS_LOCAL_FILE` flag rather than listing all of these explicitly?
> > > 
> > > Either way, we should add a WebExtension test to check that `isOriginPotentiallyTrustworthy` returns true for URLs returned by `runtime.getURL`.
> > 
> > Seems to me listing them explicitly is the proper way of doing it, I suppose
> > because this is an implementation of the W3C spec. Tanvi can probably tell
> > us more.
> 
> What does it mean for a resource to have a moz-extension:// uri?  Does it
> mean that the resource is stored locally in the extension code?  Do
> moz-extension:// uris result in a network request?  If not and if it truly
> is local, then URI_IS_LOCAL_RESOURCE would be fine -
> http://searchfox.org/mozilla-central/source/netwerk/base/nsIProtocolHandler.
> idl#249.  We could add that flag to moz-extension to fix this bug.  If
> moz-extension:// uris can be used as subresources, there could also be
> potential mixed content blocker issues with moz-extension without this flag
> -
> http://searchfox.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#514

We do set the URI_IS_LOCAL_RESOURCE flag, but set flags dynamically with
getFlagsForURI, rather than for the entire scheme. It looks like the mixed
content blocker should support this.

> As far isOriginPotentiallyTrustworthy though, that doesn't appear to use URI
> flags.  Matt or Jonathan Watt may know why.
> 
> http://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#600

Can you needinfo the appropriate person? Using protocol flags for this would
be my preference, but I'm fine just adding this to the hard-coded list if
there's a reason for it.
Flags: needinfo?(tanvi)
Matt, can you help resolve the question above? Would be great if we could get this landed eventually. Thanks!
Flags: needinfo?(MattN+bmo)
(In reply to Kris Maglione [:kmag] from comment #16)
> (In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #14)
> > (In reply to Johann Hofmann [:johannh] from comment #5)
> > > https://reviewboard.mozilla.org/r/57224/#review54098
> > > 
> > > > Is there a particular reason we can't check for the `URI_IS_LOCAL_RESOURCE` or `URI_IS_LOCAL_FILE` flag rather than listing all of these explicitly?
> > > > 
> > > > Either way, we should add a WebExtension test to check that `isOriginPotentiallyTrustworthy` returns true for URLs returned by `runtime.getURL`.
> > > 
> > > Seems to me listing them explicitly is the proper way of doing it, I suppose
> > > because this is an implementation of the W3C spec. Tanvi can probably tell
> > > us more.
> > 
> > What does it mean for a resource to have a moz-extension:// uri?  Does it
> > mean that the resource is stored locally in the extension code?  Do
> > moz-extension:// uris result in a network request?  If not and if it truly
> > is local, then URI_IS_LOCAL_RESOURCE would be fine -
> > http://searchfox.org/mozilla-central/source/netwerk/base/nsIProtocolHandler.
> > idl#249.  We could add that flag to moz-extension to fix this bug.  If
> > moz-extension:// uris can be used as subresources, there could also be
> > potential mixed content blocker issues with moz-extension without this flag
> > -
> > http://searchfox.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#514
> 
> We do set the URI_IS_LOCAL_RESOURCE flag, but set flags dynamically with
> getFlagsForURI, rather than for the entire scheme. It looks like the mixed
> content blocker should support this.
> 
> > As far isOriginPotentiallyTrustworthy though, that doesn't appear to use URI
> > flags.  Matt or Jonathan Watt may know why.
> > 
> > http://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#600
> 
> Can you needinfo the appropriate person? Using protocol flags for this would
> be my preference, but I'm fine just adding this to the hard-coded list if
> there's a reason for it.


For this bug, I would say to add URI_IS_LOCAL_RESOURCE to moz-extension if the resource is always stored locally and if there are never any network requests associated with it.  (If someone could elaborate on the use of the protocol, that would be great.  Perhaps it is for content stored in addons.)  And then hardcode moz-extension in isOriginPotentiallyTrustworthy.

Refactoring MixedContentBlocker to use isOriginPotentiallyTrustworthy and potentially refactoring isOriginPotentiallyTrustworthy to use protocol flags is a larger effort that should go into a different bug.
Flags: needinfo?(tanvi)
> 
> For this bug, I would say to add URI_IS_LOCAL_RESOURCE to moz-extension if
> the resource is always stored locally and if there are never any network
> requests associated with it.  (If someone could elaborate on the use of the
> protocol, that would be great.  Perhaps it is for content stored in addons.)
> And then hardcode moz-extension in isOriginPotentiallyTrustworthy.
> 

Great! Since moz-extension has URI_IS_LOCAL_RESOURCE already (http://searchfox.org/mozilla-central/source/netwerk/protocol/res/ExtensionProtocolHandler.cpp#33) I can probably go ahead and land this patch.
Comment on attachment 8759194 [details]
Bug 1277524 - Add moz-extension to the list of potentially trustworthy origins.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57224/diff/1-2/
Comment on attachment 8759589 [details]
Bug 1277524 - Add a WebExtension test that moz-extension is considered a trustworthy origin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57544/diff/3-4/
https://hg.mozilla.org/integration/fx-team/rev/82e3f2a1196a227ce83ae8e0d53d5c2b273205a5
Bug 1277524 - Add moz-extension to the list of potentially trustworthy origins. r=tanvi

https://hg.mozilla.org/integration/fx-team/rev/e1ac106612412ecf23e8cc0faff7519b03728c37
Bug 1277524 - Add a WebExtension test that moz-extension is considered a trustworthy origin. r=kmag
https://hg.mozilla.org/mozilla-central/rev/82e3f2a1196a
https://hg.mozilla.org/mozilla-central/rev/e1ac10661241
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: needinfo?(MattN+bmo)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.