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

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P5)

Description

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

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

Comment 1

[Johann Hofmann [:johannh]]

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.

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1277524 - Add moz-extension to the list of potentially trustworthy origins.

Review commit: https://reviewboard.mozilla.org/r/57224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57224/

Comment 3

[Johann Hofmann [:johannh]]

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?

Comment 4

[Kris Maglione [:kmag]]

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

Comment 5

[Johann Hofmann [:johannh]]

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.

Comment 6

[Johann Hofmann [:johannh]]

Attachment: Bug 1277524 - Add a WebExtension test that moz-extension is considered a trustworthy origin.

Review commit: https://reviewboard.mozilla.org/r/57544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57544/

Comment 7

[Kris Maglione [:kmag]]

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.

Comment 8

[Kris Maglione [:kmag]]

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.

Comment 9

[Tanvi Vyas[:tanvi]]

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 10

[Johann Hofmann [:johannh]]

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 11

[Johann Hofmann [:johannh]]

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? :)

Comment 12

[Johann Hofmann [:johannh]]

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 13

[Kris Maglione [:kmag]]

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.

Comment 14

[Tanvi Vyas[:tanvi]]

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

[Tanvi Vyas[:tanvi]]

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://

Comment 16

[Kris Maglione [:kmag]]

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

Comment 17

[Johann Hofmann [:johannh]]

Matt, can you help resolve the question above? Would be great if we could get this landed eventually. Thanks!

Comment 18

[Tanvi Vyas[:tanvi]]

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

Comment 19

[Johann Hofmann [:johannh]]

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

[Johann Hofmann [:johannh]]

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 21

[Johann Hofmann [:johannh]]

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/

Comment 22

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=759718dea882

Comment 23

[Johann Hofmann [:johannh]]

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

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82e3f2a1196a
https://hg.mozilla.org/mozilla-central/rev/e1ac10661241