Closed
Bug 1496418
Opened 6 years ago
Closed 6 years ago
Update Content Policy check to not return early for system principal loads so we can check CSP on system privileged pages
Categories
(Core :: DOM: Security, enhancement, P2)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla69
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(2 files, 4 obsolete files)
After we have moved the CSP into the Client (see Bug 965637) we have to remove the early bail [1] for system principal loads within content policies so that loads of system privileged pages actually consult the CSP code and we can start to apply CSPs to system privileged about pages (see Bug 1492063).
[1] https://searchfox.org/mozilla-central/source/dom/base/nsContentPolicyUtils.h#169-203
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ckerschb
Blocks: 1492063
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Assignee | ||
Comment 1•6 years ago
|
||
Hey Smaug,
after we have moved the CSP from the Principal into the client we also want to enforce CSP for system privileged pages. Currently, Content Policies return early (mostly as an optimization) within CHECK_PRINCIPAL_AND_DATA.
Within this patch I am moving that fast path into each call site and return early if the loadingPrincipal is the SystemPrincipal. I am doing this within each content policy except within CSP where we actually want to evaluate the call.
Does that seem reasonable and would we accept such a patch? Alternatively we could check the type of contentpolicy and *only* consult the actual policy if the policy is of type nsiContentSecurityPolicy. I think now that legacy addons are gone we could leave it up to the individual polices and have contentPolicyUtils.h just to consult each policy but not performing additional checks.
What do you think?
Attachment #9018533 -
Flags: feedback?(bugs)
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Alternative approach:
I thought about this more and maybe we should not remove CHECK_PRINCIPAL_AND_DATA but rename it CHECK_PRINCIPAL_CSP_AND_DATA. Within that macro we could addtionally perform a CSP check and still return early in case the loadingPrincipal is the SystemPrincipal. That approach would definitely be a smaller change and would definitely not cause any unforseen problems.
Comment 4•6 years ago
|
||
Comment on attachment 9018533 [details] [diff] [review]
bug_1496418_allow_system_principal_calls_to_csp.patch
I don't like this approach, propagating system principal checks everywhere, which means it is guaranteed that we'll forget it at some
place later.
I guess the plan for the future is to allow by default for system principal, but let csp to override?
Attachment #9018533 -
Flags: feedback?(bugs) → feedback-
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Olli Pettay [:smaug] (@TPAC, slower reviews) from comment #4)
> I guess the plan for the future is to allow by default for system principal,
> but let csp to override?
Right, so I guess you prefer the approach I explained in comment 3, which makes sense to me as well. Especially now that I have looked at the new patch this approach seems way cleaner.
What do you think?
Attachment #9018533 -
Attachment is obsolete: true
Attachment #9019977 -
Flags: review?(bugs)
Assignee | ||
Comment 6•6 years ago
|
||
Comment 7•6 years ago
|
||
Comment on attachment 9019977 [details] [diff] [review]
bug_1496418_allow_system_principal_calls_to_csp.patch
>+ if (isSystem) { \
>+ /* Check CSP for System Privileged pages */ \
>+ nsCOMPtr<nsIContentPolicy> cspPolicy = \
>+ do_GetService("@mozilla.org/cspcontext;1"); \
This is the only place where @mozilla.org/cspcontext;1 is used to create a service.
Do we really want that? Using a component both as an instance and service hints usually about a bug.
And in general, I think it would be good to avoid using component service
in this tiny bit hot code. Could we cache the relevant nsIContentPolicy somewhere and access that
here?
Attachment #9019977 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Olli Pettay [:smaug] (high review load) from comment #7)
> And in general, I think it would be good to avoid using component service
> in this tiny bit hot code. Could we cache the relevant nsIContentPolicy
> somewhere and access that here?
I tried to base my code on what was already used in that macro, e.g. the dataPolicy [1]. Ultimately I agree to your suggestion though, we shouldn't create a new instance in that hot piece of code. I added a helper function to nsCSPService, which in my opinion provides the cleanest solution.
Agreed?
[1] https://searchfox.org/mozilla-central/source/dom/base/nsContentPolicyUtils.h#190-191
Attachment #9019977 -
Attachment is obsolete: true
Attachment #9021476 -
Flags: review?(bugs)
Comment 9•6 years ago
|
||
Comment on attachment 9021476 [details] [diff] [review]
bug_1496418_allow_system_principal_calls_to_csp.patch
>+#define CHECK_PRINCIPAL_CSP_AND_DATA(action) \
> nsCOMPtr<nsIURI> requestOrigin; \
> PR_BEGIN_MACRO \
> if (loadingPrincipal) { \
> /* We exempt most loads into any document with the system principal \
> * from content policy checks, mostly as an optimization. Which means \
> * that we need to apply this check to the loading principal, not the \
> * principal that triggered the load. */ \
> bool isSystem = loadingPrincipal->GetIsSystemPrincipal(); \
>+ if (isSystem) { \
>+ /* Check CSP for System Privileged pages */ \
>+ CSPService::ConsultCSP(contentLocation, loadInfo, mimeType, decision);\
This part looks good, and I like the method name even :)
>+/* static */ void
>+CSPService::ConsultCSP(nsIURI* aContentLocation,
>+ nsILoadInfo* aLoadInfo,
>+ const nsACString &aMimeType,
>+ int16_t *outDecision)
int16_t* aOutDecisision
>+{
>+ nsCOMPtr<nsIContentPolicy> cspPolicy;
>+ if (!cspPolicy) {
cspPolicy is always null here.
>+ nsresult rv;
>+ cspPolicy = do_CreateInstance("@mozilla.org/cspservice;1", &rv);
so we end up always creating a new instance, in a tad hot code path.
Could we refactor CSP checks to static methods and call such here?
>
>+ // helper function to avoid creating a new instance of the
>+ // cspservice everytime we call content policies.
yeah, it is not doing that, but creates a new instance always ;)
Attachment #9021476 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Olli Pettay [:smaug] (high review load) from comment #9)
> >+ nsCOMPtr<nsIContentPolicy> cspPolicy;
Obviously that is missing a static keyword - good that we do code reviews - thanks!
Comment 11•6 years ago
|
||
But do we want to keep the component alive that way? Or should there be perhaps StaticRefPtr somewherE?
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #9021476 -
Attachment is obsolete: true
Attachment #9021490 -
Flags: review?(bugs)
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Olli Pettay [:smaug] (high review load) from comment #11)
> But do we want to keep the component alive that way? Or should there be
> perhaps StaticRefPtr somewherE?
Oh I see what you mean. What do you prefer? StaticRefPtr?
Comment 14•6 years ago
|
||
I'd prefer StaticRefPtr, since I think that nsCOMPtr may create static constructor.
Comment 15•6 years ago
|
||
(But I still think CSPService should have static methods doing all the work. The class doesn't have any member variables storing a state [expect unused mAppStatusCache].)
Comment 16•6 years ago
|
||
Comment on attachment 9021490 [details] [diff] [review]
bug_1496418_allow_system_principal_calls_to_csp.patch
> /**
> * Returns true if this principal's CSP should override a document's CSP for
> * loads that it triggers. Currently true for system principal, for expanded
> * principals which subsume the document principal, and add-on codebase
> * principals regardless of whether they subsume the document principal.
> */
> bool OverridesCSP(nsIPrincipal* aDocumentPrincipal)
> {
>- // SystemPrincipal can override the page's CSP by definition.
>- if (mKind == eSystemPrincipal) {
>- return true;
>- }
>-
the comment for the method needs to be updated.
>+/* static */ void
>+CSPService::ConsultCSP(nsIURI* aContentLocation,
>+ nsILoadInfo* aLoadInfo,
>+ const nsACString &aMimeType,
>+ int16_t *aOutDecision)
>+{
>+ static nsCOMPtr<nsIContentPolicy> cspPolicy;
>+ if (!cspPolicy) {
>+ nsresult rv;
>+ cspPolicy = do_CreateInstance("@mozilla.org/cspservice;1", &rv);
>+ if (NS_WARN_IF(NS_FAILED(rv))) {
>+ return;
>+ }
>+ }
I'd prefer StaticRefPtr and use of ClearOnShutdown, since I assume otherwise you'll get leaks.
Attachment #9021490 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 17•6 years ago
|
||
Hey Smaug, thanks for bearing with me. FWIW, I filed Bug 1503575 to remove the unused member mAppStatusCache. Further, I created a new static function ConsultCSP so that the nsIContentPolicy implementation of shouldLoad() just forwards the call to that static method. That looks way cleaner now actually - thanks for the suggestion.
Attachment #9021490 -
Attachment is obsolete: true
Attachment #9021524 -
Flags: review?(bugs)
Updated•6 years ago
|
Attachment #9021524 -
Flags: review?(bugs) → review+
Comment 18•6 years ago
|
||
There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:ckerschb, could you have a look please?
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 19•6 years ago
|
||
Before we can apply CSP to system privileged about pages we have to fix Bug 965637, in which we move the CSP from the Principal into the Client. Please note that the Meta Bug 1492063 for applying CSP to system privileged about: pages is blocked by 965637. At the moment we are fixing the last remaining blockers and as soon as we have landed Bug 965637 I'll try to land all the dependencies of Bug 1492063 so we end up having all about: pages secured by a CSP.
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 20•6 years ago
|
||
Comment 21•6 years ago
|
||
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/62b73610a2f7
Update Content Policy checks and allow CSP checks for system principal triggered loads. r=mccr8,baku
Comment 22•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox69:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in
before you can comment on or make changes to this bug.
Description
•