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)

enhancement

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: nobody → ckerschb
Blocks: 1492063
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Depends on: 1500083
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)
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 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-
(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)
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-
(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 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-
(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!
But do we want to keep the component alive that way? Or should there be perhaps StaticRefPtr somewherE?
Attachment #9021476 - Attachment is obsolete: true
Attachment #9021490 - Flags: review?(bugs)
(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?
I'd prefer StaticRefPtr, since I think that nsCOMPtr may create static constructor.
(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 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-
Depends on: 1503575
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)
Attachment #9021524 - Flags: review?(bugs) → review+

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)

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)
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: