hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-23 15:55:18 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix FP by requiring that the regular expression mention on of the chars important in the prefix

Browse Source
This commit is contained in:
erik-krogh
2023-07-01 20:30:09 +02:00
parent bd400be6ec
commit f9eee906cf
3 changed files with 14 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
javascript/ql/lib/semmle/javascript/security/IncompleteMultiCharacterSanitizationQuery.qll
View File

@@ -15,6 +15,14 @@ private class DangerousPrefix extends string {
this = "<!--" or
this = "<" + ["iframe", "script", "cript", "scrip", "style"]
}
/**
* Gets a character that is important to the dangerous prefix.
* That is, a char that should be mentioned in a regular expression that explicitly sanitizes the dangerous prefix.
*/
string getAnImportantChar() {
if this = ["/..", "../"] then result = ["/", "."] else result = "<"
}
}
/**
@@ -62,7 +70,11 @@ private DangerousPrefixSubstring getADangerousMatchedChar(EmptyReplaceRegExpTerm
*/
private DangerousPrefix getADangerousMatchedPrefix(EmptyReplaceRegExpTerm t) {
result = getADangerousMatchedPrefixSubstring(t) and
not exists(EmptyReplaceRegExpTerm pred | pred = t.getPredecessor+() and not pred.isNullable())
not exists(EmptyReplaceRegExpTerm pred | pred = t.getPredecessor+() and not pred.isNullable()) and
// the regex must explicitly mention a char important to the prefix.
forex(string char | char = result.getAnImportantChar() |
t.getRootTerm().getAChild*().(RegExpConstant).getValue().matches("%" + char + "%")
)
}
/**

1
javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteMultiCharacterSanitization.expected
View File

@@ -39,4 +39,3 @@
| tst-multi-character-sanitization.js:145:13:145:90 | content ... /g, '') | This string may still contain $@, which may cause an HTML element injection vulnerability. | tst-multi-character-sanitization.js:145:30:145:30 | < | <script |
| tst-multi-character-sanitization.js:148:3:148:99 | n.clone ... gi, '') | This string may still contain $@, which may cause an HTML element injection vulnerability. | tst-multi-character-sanitization.js:148:41:148:41 | < | <script |
| tst-multi-character-sanitization.js:152:3:152:99 | n.clone ... gi, '') | This string may still contain $@, which may cause an HTML element injection vulnerability. | tst-multi-character-sanitization.js:152:41:152:41 | < | <script |
| tst-multi-character-sanitization.js:156:13:156:44 | content ... )/, '') | This string may still contain $@, which may cause an HTML element injection vulnerability. | tst-multi-character-sanitization.js:156:30:156:32 | .+? | <script |

2
javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst-multi-character-sanitization.js
View File

@@ -153,5 +153,5 @@
o.push({specified : 1, nodeName : a});
});
content = content.replace(/.+?(?=\s)/, ''); // OK - but flagged as not sanitizing <script> tags [INCONSISTENCY]
content = content.replace(/.+?(?=\s)/, ''); // OK
});