Closed
Bug 1142332
Opened 9 years ago
Closed 8 years ago
nsCSPContext::getAllowsInternal could call CSP_EnumToKeyword with CSP_HASH
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mccr8, Assigned: freddy)
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)
1.14 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Group: core-security → dom-core-security
Comment 1•8 years ago
|
||
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]
Assignee | ||
Comment 2•8 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?
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8743742 -
Flags: review?(ckerschb)
Assignee | ||
Comment 5•8 years ago
|
||
on try https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bbbf18eb0f6
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Updated•8 years ago
|
Assignee: nobody → fbraun
Status: NEW → ASSIGNED
Whiteboard: [CID 1286018], [domsecurity-backlog] → [CID 1286018], [domsecurity-active]
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 9•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4845199b1bb
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a4845199b1bb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Group: dom-core-security → core-security-release
Updated•8 years ago
|
Whiteboard: [CID 1286018], [domsecurity-active] → [CID 1286018], [domsecurity-active][post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [CID 1286018], [domsecurity-active][post-critsmash-triage] → [CID 1286018], [domsecurity-active][post-critsmash-triage][adv-main48-]
Updated•7 years ago
|
Group: core-security-release
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•