nsCSPContext::getAllowsInternal could call CSP_EnumToKeyword with CSP_HASH

RESOLVED FIXED in Firefox 48



DOM: Security
3 years ago
a year ago


(Reporter: mccr8, Assigned: freddyb)


({coverity, csectype-bounds, sec-other})

coverity, csectype-bounds, sec-other

Firefox Tracking Flags

(firefox48 fixed)


(Whiteboard: [CID 1286018], [domsecurity-active][post-critsmash-triage][adv-main48-])


(1 attachment, 1 obsolete attachment)



3 years ago
In nsCSPContext::getAllowsInternal, if aKeyword is CSP_HASH, then when we call allows() down below, for the CSPLOGUTILS, we end up calling CSP_EnumToKeyword(aKeyword) with something that isn't a keyword.

I don't know if we actually run this code in non-debug builds, but I'm filing this as a security bug just in case.


2 years ago
Group: core-security → dom-core-security
I suppose that code is not actually executed, but we should fix it anyway.
Component: Security → DOM: Security
Whiteboard: [CID 1286018] → [CID 1286018], [domsecurity-backlog]

Comment 2

2 years ago
I was trying to find out if I can take that bug (probably yes), but I found that the exact code does not exist anymore.
Matches for searching CSP_EnumToKeyword(aKeyword) only finds nsCSPUtils.cpp, not nsCSPContext.cpp.

There is one ternary expression in nsCSPBaseSrc:allows that does log templating with
> aKeyword == CSP_HASH ? "hash" : CSP_EnumToKeyword(aKeyword)
whereas most other code goes right for CSP_EnumToKeyword(aKeyword). I have yet to understand the code well enough to see in which cases the CSPKeyword can be a CSP_HASH.
I'm not entirely sure how CSP_EnumToKeyword is used over the entire codebase either. Maybe we could remove the one ternary statement (currently nsCSPUtils.cpp line 348) and have CSP_EnumToKeyword return a string (like "hash") when CSP_HASH is supplied?
Currently it's probably not the case in the codebase, but what if someone calls CSP_EnumToKeyword(CSP_HASH) [1] in a non debug build? We would index out of bounds, right? What we should do is some kind of range check before we index into the array, something like:

if (static_cast<int>(aKey) >= static_cast<int)(CSP_LAST_KEYWORD_VALUE)) {
  return "index out of bounds error";

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.h#142

Comment 4

2 years ago
Created attachment 8743742 [details] [diff] [review]
Bug 1142332 prevent calling CSP_EnumToKeyword with CSP_HASH
Attachment #8743742 - Flags: review?(ckerschb)

Comment 5

2 years ago
on try https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bbbf18eb0f6
Comment on attachment 8743742 [details] [diff] [review]
Bug 1142332 prevent calling CSP_EnumToKeyword with CSP_HASH

Review of attachment 8743742 [details] [diff] [review]:

::: dom/security/nsCSPUtils.h
@@ +147,5 @@
>                  "CSP_LAST_KEYWORD_VALUE does not match length of CSPStrKeywords");
> +
> +  if (static_cast<int>(aKey) >= static_cast<int>(CSP_LAST_KEYWORD_VALUE)) {
> +    return "index out of bounds error";
> +  }

It seems that CSP_LAST_KEYWORD_VALUE does not have an entry within the CSPStrKeywords array. So, if someone calls CSPStrKeywords(CSP_LAST_KEYWORD_VALUE) we would already index out of bounds.

You could rewrite to:
if (static_cast<uint32_t>(aKey) < static_cast<uint32_t>(CSP_LAST_KEYWORD_VALUE)) {
  return CSPStrKeywords[static_cast<uint32_t>(aKey)];
return "error: invalid keyword in CSP_EnumToKeyword";

Sorry, I know in Comment 3 I said you could use  "index out of bounds error" but know I think the new error string is more useful.
Attachment #8743742 - Flags: review?(ckerschb)

Comment 7

2 years ago
Created attachment 8743809 [details] [diff] [review]
Bug 1142332 prevent calling CSP_EnumToKeyword with CSP_HASH

I have updated patch according to your suggestions.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7ff94441097
Attachment #8743742 - Attachment is obsolete: true
Attachment #8743809 - Flags: review?(ckerschb)
Comment on attachment 8743809 [details] [diff] [review]
Bug 1142332 prevent calling CSP_EnumToKeyword with CSP_HASH

Review of attachment 8743809 [details] [diff] [review]:

thanks, r=me
Attachment #8743809 - Flags: review?(ckerschb) → review+
Assignee: nobody → fbraun
Whiteboard: [CID 1286018], [domsecurity-backlog] → [CID 1286018], [domsecurity-active]


2 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Group: dom-core-security → core-security-release
Whiteboard: [CID 1286018], [domsecurity-active] → [CID 1286018], [domsecurity-active][post-critsmash-triage]
Whiteboard: [CID 1286018], [domsecurity-active][post-critsmash-triage] → [CID 1286018], [domsecurity-active][post-critsmash-triage][adv-main48-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.