Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pref breakage: security.csp.experimentalEnabled #223

Closed
crssi opened this issue Aug 28, 2017 · 14 comments
Closed

pref breakage: security.csp.experimentalEnabled #223

crssi opened this issue Aug 28, 2017 · 14 comments

Comments

@crssi
Copy link

crssi commented Aug 28, 2017

/* 2674  */ user_pref("security.csp.experimentalEnabled", false); // true breaks CSS at https://securityheaders.io/

Just FYI.
What implications has setting this pref to false instead to true?
Cheers

@Theemim
Copy link

Theemim commented Aug 28, 2017

I think that pref enables processing of the require-sri-for directive.

https://dxr.mozilla.org/mozilla-release/source/dom/security/nsCSPParser.cpp#1058

 // Check if it is a valid directive
  if (!CSP_IsValidDirective(mCurToken) ||
       (!sCSPExperimentalEnabled &&
         CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::REQUIRE_SRI_FOR))) {
    const char16_t* params[] = { mCurToken.get() };
    logWarningErrorToConsole(nsIScriptError::warningFlag, "couldNotProcessUnknownDirective",
                             params, ArrayLength(params));
    return nullptr;
  }

At securityheaders.io, I see require-sri-for in the content-security-policy-report-only header but not the Content-Security-Policy header.

@crssi
Copy link
Author

crssi commented Aug 28, 2017

Thank you @Theemim
So, this is BUG at https://securityheaders.io/ page, since headers are not complete?
General question, not connected with this specific page, is it safe to reset this pref to default (false)?

@earthlng
Copy link
Contributor

earthlng commented Aug 28, 2017

Nice find @crssi !

There seem to be 2 problems here, 1st what seems to be a bug in FF because the require-sri-for directive in the report-only CSP should not block the CSS IMO, and the 2nd problem is with the site itself in that it provides incorrect hashes. From the console output:

None of the “sha512” hashes in the integrity attribute match the content of the subresource.

Someone should test this with the latest nightly and if the bug is not already fixed there, open a new bug @ bugzilla. Any volunteers? xD

As to whether "is it safe to reset this pref to default (false)?", I personally never set it to true in the 1st place because I assumed that it's probably not ready given that it still defaults to false.
edit: ... and because @fmarier said:

it's a new feature which hasn't been standardized yet

@ScottHelme can you check whether the hashes are indeed wrong, please?

The only problem is we need a new test-site for the bug-report if the hashes are wrong and Scott fixes it :(

@Atavic
Copy link

Atavic commented Aug 28, 2017

Securityheaders page source, line 29, is the same found by quering here for
https://cdnjs.cloudflare.com/ajax/libs/skel/2.2.1/skel.min.js

@Atavic
Copy link

Atavic commented Aug 28, 2017

@crssi It lacks Content-Security-Policy headers.

@crssi
Copy link
Author

crssi commented Aug 28, 2017

Thank you all. :)

@earthlng should this be disabled or forced to false/default in ghacksuser.js?

@earthlng
Copy link
Contributor

earthlng commented Aug 28, 2017

@earthlng should this be disabled or forced to false/default in ghacksuser.js?

#53

date: 13-March-2017

  • other new additions since ghacks-user.js v51
    user_pref("security.csp.experimentalEnabled", true); // experimental - yes we're adventurous - use on your own risk !!

shortly thereafter, still mid March 2017:

#10 (comment)
#10 (comment)

#10 (comment) ...

fmarier: For that same reason, there's probably not much point in enabling it either.

earthlng: Pants, I'd say we disable it again and wait until mozilla enables it by default, if ever.

@crssi
Copy link
Author

crssi commented Aug 28, 2017

Go it... lets see then what @Thorin-Oakenpants will say, ;)
And at least in the latest user.js no experimental is mentioned in the comments, but it is in the name of the pref. :)
Thanks

@earthlng
Copy link
Contributor

earthlng commented Aug 28, 2017

Securityheaders page source, line 29, is the same found by quering here

@Atavic thanks for that link. So if the hashes are correct then it seems that FF expects hashes for all non-inline scripts and styles. Regardless of that, the directive in a CSP report-only header should not deny loading those files.

edit: as François said:

it's a new feature which hasn't been standardized yet

@earthlng
Copy link
Contributor

earthlng commented Aug 28, 2017

So if the hashes are correct then it seems that FF expects hashes for all non-inline scripts and styles.

Although, reading it again, the error message clearly says "... the “sha512” hashes in the integrity attribute ..." and none of the 1st-party styles + scripts loading tags on Scott's site have an integrity attribute.
Well, IDK WTH is happening - will just keep waiting for volunteers to inform mozilla about this xD

@fmarier
Copy link

fmarier commented Aug 28, 2017

That feature isn't finished yet, which is why it's not enabled by default.

@earthlng
Copy link
Contributor

earthlng commented Aug 28, 2017

https://w3c.github.io/webappsec-subresource-integrity/

Editors:
François Marier (Mozilla) [email protected]

I guess we can consider mozilla informed about this issue now :)

@earthlng
Copy link
Contributor

earthlng commented Sep 2, 2017

@earthlng if you want to flip the pref to inactive, do a direct commit.

No it's fine. When it works it's a nice security feature and when it doesn't it shows in the console. And since probably not many sites use it anyway it shouldn't cause a lot of breakage. And it has 'experimental' right in the name so people should expect some kind of breakage.

@earthlng earthlng closed this as completed Sep 2, 2017
@Atavic
Copy link

Atavic commented May 3, 2018

It seems definitely dead to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants