Closed Bug 1142332 Opened 5 years ago Closed 4 years ago

nsCSPContext::getAllowsInternal could call CSP_EnumToKeyword with CSP_HASH

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mccr8, Assigned: freddyb)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, csectype-bounds, sec-other, Whiteboard: [CID 1286018], [domsecurity-active][post-critsmash-triage][adv-main48-])

Attachments

(1 file, 1 obsolete file)

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.
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]
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 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)
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
Status: NEW → ASSIGNED
Whiteboard: [CID 1286018], [domsecurity-backlog] → [CID 1286018], [domsecurity-active]
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a4845199b1bb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
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.