Closed Bug 1490874 Opened Last year Closed Last year

"Security Error" falls under the "Logs" filter in Browser Console rather than "Errors"

Categories

(Core :: Security: CAPS, defect, P2)

63 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: from_bugzilla2, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 files, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180906162647

Steps to reproduce:

1. Find an error in the Browser Console originating from an extension
2. Click the View Source in Debugger link to trigger bug 1490856


Actual results:

The resulting Security Error appears as a log message, which will be filtered out if "Logs" has been unchecked to avoid the flood of multi-line "Download your new career!" pseudo-ads and large-fonted "Pasting anything in here could give attackers access to your account." warnings that sites like GOG.com and Discord print to the console.


Expected results:

The Security Error should be classified as an error (or at least a warning).
Component: Untriaged → Developer Tools
Product: Firefox → WebExtensions
Component: Developer Tools → DOM: Security
Product: WebExtensions → Core
Yeah, I guess we should get that fixed - thanks for filing the bug. I wonder if that ever worked correctly.
Assignee: nobody → ckerschb
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Whiteboard: [domsecurity-active]
This is more properly a "Security: CAPS" bug (it's the CheckSameOriginError string), although it's fair to say that the "DOM Security" team also owns that code and that we should just kill that BMO component (though moving the code in the repo is probably not worth the work).
(In reply to Daniel Veditz [:dveditz] from comment #2)
> This is more properly a "Security: CAPS" bug (it's the CheckSameOriginError
> string), although it's fair to say that the "DOM Security" team also owns
> that code and that we should just kill that BMO component (though moving the
> code in the repo is probably not worth the work).

Ah, yeah. I thought these were CSP errors. I think we treat CSP errors the same way and need the same sort of fix for both, though.
Component: DOM: Security → Security: CAPS
(In reply to Kris Maglione [:kmag] from comment #3)
> (In reply to Daniel Veditz [:dveditz] from comment #2)
> > This is more properly a "Security: CAPS" bug (it's the CheckSameOriginError
> > string), although it's fair to say that the "DOM Security" team also owns
> > that code and that we should just kill that BMO component (though moving the
> > code in the repo is probably not worth the work).
> 
> Ah, yeah. I thought these were CSP errors. I think we treat CSP errors the
> same way and need the same sort of fix for both, though.

CSP errors should work correctly because we use "nsIScriptError::errorFlag" which the console uses as a differentiator whether to log to "Security" or not. This SOP console log probably even predates the various panels we have to filter messages in the console.
Smaug, it seems we were simply logging to the console instead of using "nsIScriptError::errorFlag" to log to the "Security" pane within the console.

@Harald: I am passing "SOP" as the category now, can we do something similar as within Bug 1304645 or Bug 1475073 so we can link to MDNs documentation around same-origin policy?

@Smaug: Last Question, should we update caps.properties and replace
CheckSameOriginError = Security Error: Content at %S may not load data from %S.
with
CheckSameOriginError = Same-Origin Policy: Content at %S may not load data from %S.

That way it would be more clear that actually the same origin policy stopped something here. What do you think?
Flags: needinfo?(hkirschner)
Attachment #9010209 - Flags: review?(bugs)
We can't change the textual message of some existing error message, but would need a new message. Otherwise, IIRC, localization folks might not notice there is something to update.
(In reply to Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) from comment #6)
> We can't change the textual message of some existing error message, but
> would need a new message. Otherwise, IIRC, localization folks might not
> notice there is something to update.

I am fine either way. Maybe for this bug we should just leave as is. At least we would log to the Security pane. If Devtools folks think it's worth updating to reflect the 'same-origin policy' change from comment 5, then we could do in a follow up. Probably all three messages within caps.properties should be updated which in the end would require some more work anyway.
Comment on attachment 9010209 [details] [diff] [review]
bug_1490874_principal_sec_errors_correct_console.patch

>     else // Print directly to the console
>     {
>-        nsCOMPtr<nsIConsoleService> console(
>-            do_GetService("@mozilla.org/consoleservice;1"));
>-        NS_ENSURE_TRUE(console, NS_ERROR_FAILURE);
>+      nsCOMPtr<nsIConsoleService> console(do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>+      NS_ENSURE_TRUE(console, NS_ERROR_FAILURE);
>+      nsCOMPtr<nsIScriptError> error(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));
>+      NS_ENSURE_TRUE(error, NS_ERROR_FAILURE);
> 
>-        console->LogStringMessage(message.get());
>+      // using category of SOP so we can link to MDN
>+      nsCString category("SOP");
Why this category string? Why not pass just "SOP" as param.

>+
>+      nsresult rv = error->Init(message, EmptyString(), 
>+                                EmptyString(), 0, 0,
>+                                nsIScriptError::errorFlag,
>+                                category.get(), false);
Hmm, why are we passing false as the last param. That should be based on the context whether we're in private or non private browsing window.
Do we currently have https://bugzilla.mozilla.org/show_bug.cgi?id=769298 issue and do we still have that issue with the patch?
Attachment #9010209 - Flags: review?(bugs) → review-
ni? Nicolas to provide input on how to add the SOP category to the Security filter.
Flags: needinfo?(hkirschner) → needinfo?(nchevobbe)
(In reply to Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) from 
> >+      nsCString category("SOP");
> Why this category string? Why not pass just "SOP" as param.

Relict of a previous version of the patch - updated!

> >+
> >+      nsresult rv = error->Init(message, EmptyString(), 
> >+                                EmptyString(), 0, 0,
> >+                                nsIScriptError::errorFlag,
> >+                                category.get(), false);
> Hmm, why are we passing false as the last param. That should be based on the
> context whether we're in private or non private browsing window.

That is correct, but we need a cx to determine whether we are in PB mode or not. I had that in a previous patch actually, but then I realized we are in the 'else' branch here. If there is a cx, then we enter the if-clause and do |SetPendingException|.

I left a comment in the code to explain the decision to use 'false' as a fallback.
Attachment #9010209 - Attachment is obsolete: true
Attachment #9010594 - Flags: review?(bugs)
Blocks: 1492819
See Also: → 1492819
FWIW, I filed Bug 1492819 to update the naming from "Security Error:" to "Same-Origin Policy" which I think we should also fix.
Comment on attachment 9010594 [details] [diff] [review]
bug_1490874_principal_sec_errors_correct_console.patch

But can't we usually just pass principal to the method and check from there whether we're dealing with pb window? Or pass some flag telling whether we're in pb window.
Leaking the information to browser console is rather bad privacy violation from pb point of view, IMO. One couldn't use pb window in such way that some other user later couldn't see some traces of browsing in the browser console.
(This may be a generic issue with console messages, but something we should fix anyhow.)
I know, the existing code leaks the information too, but I'd rather not add yet another broken nsIScriptError usage.

ehsan may have an opinion on console messages.
Attachment #9010594 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) from comment #12)
> Leaking the information to browser console is rather bad privacy violation
> I know, the existing code leaks the information too, but I'd rather not add
> yet another broken nsIScriptError usage.
> 
> ehsan may have an opinion on console messages.

In fact, I care a lot about such privacy violations too, hence I updated all the codepaths.

Here is the thing:
* I figured the JSContext is never non null on any of the callsites, hence I removed it.
* Whenever the reportToConsole flag is true, I made sure we pass the right flag whether we are in private browsing mode or not, *but* in some cases that's not easily possible, e.g. within modules/E10SUtils.jsm. In such cases I left a comment on the callsite as well as a comment within CheckSameOriginURI() that one can not 100% rely on the isPrivateWindow flag in case reporterror is false. I guess that's a reasonable tradeoff for those few callsites.

Thanks!
Attachment #9010594 - Attachment is obsolete: true
Attachment #9010884 - Flags: review?(bugs)
Attachment #9010884 - Flags: review?(bugs) → review+
I pushed that all to TRY and it seems there is one test failing because of the change within this bug[1]. It's that test:
  browser/components/payments/test/mochitest/test_basic_card_option.htm

Doing a searchfox search doesn't return any results of the failing test as of now; probably it takes some time till searchfox can index the latest merge. In my local build it's that line:
>     this["_cc-type"].src = "chrome://formautofill/content/icon-credit-card-generic.svg";
within basic-card-options.js

Let's wait till serachfox is up to date to figure out what's going on.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=02e9588f7bf7fe207ec180522582c2e25dc55994&selectedJob=200699153
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> Doing a searchfox search doesn't return any results of the failing test as
> of now; probably it takes some time till searchfox can index the latest
> merge.

I found the problem, it's within Bug 1463545 [1]. Flagging 1463545 as blocking this bug before we can land this bug.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1463545#c25
Depends on: 1463545
This patch needs rebasing on latest central btw.
To fix the test failures please add an exception to our console listener like [1] involving a match on "Security Error: Content at http://mochi.test:8888/" and "icon-credit-card-generic.svg" or something similar.

[1] https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/browser/components/payments/test/mochitest/payments_common.js#110-113
No longer depends on: 1463545
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #17)
> To fix the test failures please add an exception to our console listener
> like [1] involving a match on "Security Error: Content at
> http://mochi.test:8888/" and "icon-credit-card-generic.svg" or something
> similar.

Got it, thanks for the speedy response.
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/510e95767aeb
Log Principal based Security Errors to the Security pane in the console. r=smaug
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac12aff1944d
Backed out changeset 510e95767aeb for security failures in browser/components/payments/test/mochitest/test_basic_card_form.html CLOSED TREE
Attached patch bug_1490874_tests_updates.patch (obsolete) — Splinter Review
Hey Matt,

ultimately there were two tests failing:
* test_basic_card_option.html
* test_basic_card_form.html

which caused me to add that 'whitelist' mechanism as you suggested. Is this what you had in mind? Finally, I wanted to make sure, is it really expected behavior in the tests that those files are not allowed to be loaded?
Flags: needinfo?(ckerschb)
Attachment #9011383 - Flags: review?(MattN+bmo)
Comment on attachment 9011383 [details] [diff] [review]
bug_1490874_tests_updates.patch

Review of attachment 9011383 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #22)
> Finally, I wanted to make sure, is it really expected
> behavior in the tests that those files are not allowed to be loaded?

Those errors aren't important to the tests and these errors only affect the tests, not shipping code, so it's fine to ignore them.

::: browser/components/payments/test/mochitest/payments_common.js
@@ +116,5 @@
> +      msg.message.includes("editAddress.css") ||
> +      msg.message.includes("editDialog.css") ||
> +      msg.message.includes("editCreditCard.css")) {
> +    // Ignoring SOP error
> +    return;

This is going to exclude all errors mentioning those files, not just SOP ones. Please use my suggestion to make this only match SOP issues related to mochi.test by adding a check for "Security Error: Content at http://mochi.test:8888/" in the message too.
Attachment #9011383 - Flags: review?(MattN+bmo) → review-
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #24)
> This is going to exclude all errors mentioning those files, not just SOP
> ones. Please use my suggestion to make this only match SOP issues related to
> mochi.test by adding a check for "Security Error: Content at
> http://mochi.test:8888/" in the message too.

Agreed, that makes sense - thanks!
Attachment #9011383 - Attachment is obsolete: true
Attachment #9011675 - Flags: review?(MattN+bmo)
Comment on attachment 9011675 [details] [diff] [review]
bug_1490874_tests_updates.patch

Review of attachment 9011675 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #9011675 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e394388fc017
Log Principal based Security Errors to the Security pane in the console. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1fc8e9cb601
Update tests to comply with new script error logging mechanism. r=MattN
https://hg.mozilla.org/mozilla-central/rev/e394388fc017
https://hg.mozilla.org/mozilla-central/rev/b1fc8e9cb601
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(In reply to :Harald Kirschner :digitarald from comment #9)
> ni? Nicolas to provide input on how to add the SOP category to the Security
> filter.

Harald, FWIW within Bug 1304645 and Bug 1475073 we used the category to link to MDN. Within this Bug we are using "SOP" as the category which should allow us to do the same for same-origin policy links, right?

[1] https://searchfox.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#1114
Flags: needinfo?(hkirschner)
Sorry for the delay, I missed this one.

> Harald, FWIW within Bug 1304645 and Bug 1475073 we used the category to link to MDN. Within this Bug we are using "SOP" as the category which should allow us to do the same for same-origin policy links, right?

yes, you can have a look at how the link is done in errordocs.js: 
https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/devtools/server/actors/errordocs.js#114-133
Flags: needinfo?(nchevobbe)
Flags: needinfo?(hkirschner)
You need to log in before you can comment on or make changes to this bug.