1070733 - CSP: logging differences between old and new CSP backend

Status: RESOLVED INVALID

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Sid Stamm [:geekboy or :sstamm]
> > -# inline style refers to CSS code that is embedded into the HTML document.
> > -inlineStyleBlocked = An attempt to apply inline style sheets has been blocked
> > -# LOCALIZATION NOTE (scriptFromStringBlocked):
> > -# eval is a name and should not be localized.
> > -scriptFromStringBlocked = An attempt to call JavaScript from a string (by calling a function like eval) has been blocked
> 
> I think inlineScriptBlocked, inlineStyleBlocked and scriptFromStringBlocked
> are still relevant.  Problem is we never use them.
> 
> We probably should use them here-ish:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPContext.
> cpp#858

Indeed, we should make use of them.

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1070733_change_logging_message_for_inline_script_and_styles.patch

Sid, there are some cases which I think we are not even handling correctly in the old implementation, compare:
http://mxr.mozilla.org/mozilla-release/source/content/base/src/contentSecurityPolicy.js#630

Consider the following argument values:
> aObserver         : Inline Script had invalid hash
> mViolatedDirective: script-src http://mochi.test:8888
in http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPContext.cpp#788

In that case the mViolatedDirective is *not* empty in CSPReportSenderRunnable, see following stacktrace:

> Assertion failure: false, at /home/christoph/moz/mc/content/base/src/nsCSPContext.cpp:850
> CSPReportSenderRunnable (/home/christoph/moz/mc/content/base/src/nsCSPContext.cpp:850)
> nsCSPContext::AsyncReportViolation(nsISupports*, nsIURI*, nsAString_internal const&, unsigned int, > nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, unsigned int) (/home/christoph/moz/> mc/content/base/src/nsCSPContext.cpp:1019)
> nsCSPContext::LogViolationDetails(unsigned short, nsAString_internal const&, nsAString_internal const&, int, > nsAString_internal const&, nsAString_internal const&) (/home/christoph/moz/mc/content/base/src/nsCSPContext.cpp:477)
> CSPAllowsInlineScript(nsIScriptElement*, nsIDocument*) (/home/christoph/moz/mc/content/base/src/nsScriptLoader.> cpp:532)
> nsScriptLoader::ProcessScriptElement(nsIScriptElement*) (/home/christoph/moz/mc/content/base/src/nsScriptLoader.> cpp:733)
> nsScriptElement::MaybeProcessScript() (/home/christoph/moz/mc/content/base/src/nsScriptElement.cpp:140)
> nsIScriptElement::AttemptToExecute() (/home/christoph/moz/mc-obj-ff-dbg/dom/xslt/xslt/../../../dist/include/> nsIScriptElement.h:220)

but there are also cases where mViolatedDirective is empty in CSPReportSenderRunnable:
>  aObserver         : Inline Style had invalid hash
>  mViolatedDirective:

See the following stacktrace:

> [16764] ###!!! ASSERTION: violated directive empty when logging to console: '!mViolatedDirective.IsEmpty()', > file /home/christoph/moz/mc/content/base/src/nsCSPContext.cpp, line 914
> CSPReportSenderRunnable (/home/christoph/moz/mc/content/base/src/nsCSPContext.cpp:913)
> nsCSPContext::AsyncReportViolation(nsISupports*, nsIURI*, nsAString_internal const&, unsigned int, > nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, unsigned int) (/home/christoph/moz/> mc/content/base/src/nsCSPContext.cpp:1014)
> nsCSPContext::LogViolationDetails(unsigned short, nsAString_internal const&, nsAString_internal const&, int, > nsAString_internal const&, nsAString_internal const&) (/home/christoph/moz/mc/content/base/src/nsCSPContext.cpp:479)
> nsStyleUtil::CSPAllowsInlineStyle(nsIContent*, nsIPrincipal*, nsIURI*, unsigned int, nsAString_internal const&, > tag_nsresult*) (/home/christoph/moz/mc/layout/style/nsStyleUtil.cpp:742)
> nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument*, mozilla::dom::ShadowRoot*, nsICSSLoaderObserver*, bool*, > bool*, bool) (/home/christoph/moz/mc/content/base/src/nsStyleLinkElement.cpp:409)
> nsStyleLinkElement::UpdateStyleSheet(nsICSSLoaderObserver*, bool*, bool*, bool) (/home/christoph/moz/mc/content/base> /src/nsStyleLinkElement.cpp:217)
> nsHtml5DocumentBuilder::UpdateStyleSheet(nsIContent*) (/home/christoph/moz/mc/parser/html/nsHtml5DocumentBuilder.> cpp:76)


Hence I think, we should use a fallback mechanism if mViolatedDirective is empty, and also include observer-topics for nonce and hash so that mViolatedDirective never ends up being empty. What do you think?

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8493157 [details] [diff] [review]
bug_1070733_change_logging_message_for_inline_script_and_styles.patch

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

Here are some specific comments.

::: content/base/src/nsCSPContext.cpp
@@ +825,5 @@
>        }
> +
> +      if (!mViolatedDirective.IsEmpty()) {
> +        return;
> +      }

Please add a comment explaining the early return when mViolatedDirective is not empty.

@@ +834,5 @@
> +      NS_ASSERTION(stringBundleService, "String bundle service must be present!");
> +
> +      stringBundleService->CreateBundle(
> +        "chrome://global/locale/security/csp.properties",
> +        getter_AddRefs(stringBundle));

Nit: please wrap this line at second arg:
sbs->CB("string",
        foo);

@@ +837,5 @@
> +        "chrome://global/locale/security/csp.properties",
> +        getter_AddRefs(stringBundle));
> +      if (!stringBundle) {
> +        return;
> +      }

Again, please comment on why we're returning early.  Maybe add an assertion since we expect the bundle to be present.

@@ +844,5 @@
> +          aObserverSubject.EqualsASCII(SCRIPT_NONCE_VIOLATION_OBSERVER_TOPIC) ||
> +          aObserverSubject.EqualsASCII(SCRIPT_HASH_VIOLATION_OBSERVER_TOPIC)) {
> +        stringBundle->GetStringFromName(
> +          NS_ConvertUTF8toUTF16("inlineScriptBlocked").get(),
> +          getter_Copies(mViolatedDirective));

Won't this confuse report recipients if the violated-directive field in the JSON report contains something other than a directive?

Maybe in these fallback cases it makes more sense to avoid sending remote reports (or at least put the error into another field in the JSON).

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8493157 [details] [diff] [review]
> bug_1070733_change_logging_message_for_inline_script_and_styles.patch
> 
> Sid, there are some cases which I think we are not even handling correctly
> in the old implementation, compare:
> http://mxr.mozilla.org/mozilla-release/source/content/base/src/
> contentSecurityPolicy.js#630

Yep.  The old implementation was abusing the violated-directive field in the report (it's a legacy implementation, and was wrong).  We should be reporting the actual directive that was violated and if we want to include some more details, maybe those belong in the error console or another JSON field.

> but there are also cases where mViolatedDirective is empty in
> CSPReportSenderRunnable:
> >  aObserver         : Inline Style had invalid hash
> >  mViolatedDirective:
> 
> See the following stacktrace:
> 
> > [16764] ###!!! ASSERTION: violated directive empty when logging to console: '!mViolatedDirective.IsEmpty()', > file /home/christoph/moz/mc/content/base/src/nsCSPContext.cpp, line 914
> > CSPReportSenderRunnable (/home/christoph/moz/mc/content/base/src/nsCSPContext.cpp:913)
> > nsCSPContext::AsyncReportViolation(nsISupports*, nsIURI*, nsAString_internal const&, unsigned int, > nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, unsigned int) (/home/christoph/moz/> mc/content/base/src/nsCSPContext.cpp:1014)
> > nsCSPContext::LogViolationDetails(unsigned short, nsAString_internal const&, nsAString_internal const&, int, > nsAString_internal const&, nsAString_internal const&) (/home/christoph/moz/mc/content/base/src/nsCSPContext.cpp:479)

We should fix that.

> Hence I think, we should use a fallback mechanism if mViolatedDirective is
> empty, and also include observer-topics for nonce and hash so that
> mViolatedDirective never ends up being empty. What do you think?

I agree, but instead of copying the old implementation, we should actually put the violated directive into the report (if we can).

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8493157 [details] [diff] [review]
bug_1070733_change_logging_message_for_inline_script_and_styles.patch

I will incorporate changes and will flag you for review again. I guess you wanted, but haven't cleared the review flag when you did the first round of reviews here.

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

Clearing out my buglist at the moment, given that this bug hasen't been touched in over a year and also the fact that we inlined most of our CSP reporting mechanism within Bug 1026520, I think this bug became invalid.