diff --git a/javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql b/javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql index 4c8b420583a..8394bbf1ee9 100644 --- a/javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql +++ b/javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql @@ -13,17 +13,27 @@ import javascript private import semmle.javascript.dataflow.InferredTypes -from DataFlow::MethodCallNode call, string name, DataFlow::Node substring, string target +/** + * A check on a string for whether it contains a given substring, possibly with restrictions on the location of the substring. + */ +class SomeSubstringCheck extends DataFlow::Node { + DataFlow::Node substring; + + SomeSubstringCheck() { + this.(StringOps::StartsWith).getSubstring() = substring or + this.(StringOps::Includes).getSubstring() = substring or + this.(StringOps::EndsWith).getSubstring() = substring + } + + /** + * Gets the substring. + */ + DataFlow::Node getSubstring() { result = substring } +} + +from SomeSubstringCheck check, DataFlow::Node substring, string target, string msg where - ( - name = "indexOf" or - name = "lastIndexOf" or - name = "includes" or - name = "startsWith" or - name = "endsWith" - ) and - call.getMethodName() = name and - substring = call.getArgument(0) and + substring = check.getSubstring() and substring.mayHaveStringValue(target) and ( // target contains a domain on a common TLD, and perhaps some other URL components @@ -34,33 +44,22 @@ where // target is a HTTP URL to a domain on any TLD target.regexpMatch("(?i)https?://([a-z0-9-]+\\.)+([a-z]+)(:[0-9]+)?/?") ) and + ( + if check instanceof StringOps::StartsWith + then msg = "may be followed by an arbitrary host name" + else + if check instanceof StringOps::EndsWith + then msg = "may be preceded by an arbitrary host name" + else msg = "can be anywhere in the URL, and arbitrary hosts may come before or after it" + ) and // whitelist not ( - (name = "indexOf" or name = "lastIndexOf") and - ( - // arithmetic on the indexOf-result - any(ArithmeticExpr e).getAnOperand().getUnderlyingValue() = call.asExpr() - or - // non-trivial position check on the indexOf-result - exists(EqualityTest test, Expr n | test.hasOperands(call.asExpr(), n) | - not n.getIntValue() = [-1 .. 0] - ) - ) - or // the leading dot in a subdomain sequence makes the suffix-check safe (if it is performed on the host of the url) - name = "endsWith" and + check instanceof StringOps::EndsWith and target.regexpMatch("(?i)\\.([a-z0-9-]+)(\\.[a-z0-9-]+)+") or // the trailing port or slash makes the prefix-check safe - ( - name = "startsWith" - or - name = "indexOf" and - exists(EqualityTest test, Expr n | - test.hasOperands(call.asExpr(), n) and - n.getIntValue() = 0 - ) - ) and + check instanceof StringOps::StartsWith and target.regexpMatch(".*(:[0-9]+|/)") ) -select call, "'$@' may be at an arbitrary position in the sanitized URL.", substring, target +select check, "'$@' " + msg + ".", substring, target diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSubstringSanitization.expected b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSubstringSanitization.expected index 96e67f929c9..efbaad5a672 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSubstringSanitization.expected +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSubstringSanitization.expected @@ -1,18 +1,22 @@ -| tst-IncompleteUrlSubstringSanitization.js:4:5:4:27 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:4:15:4:26 | "secure.com" | secure.com | -| tst-IncompleteUrlSubstringSanitization.js:5:5:5:27 | x.index ... e.net") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:5:15:5:26 | "secure.net" | secure.net | -| tst-IncompleteUrlSubstringSanitization.js:6:5:6:28 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:6:15:6:27 | ".secure.com" | .secure.com | -| tst-IncompleteUrlSubstringSanitization.js:10:5:10:27 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:10:15:10:26 | "secure.com" | secure.com | -| tst-IncompleteUrlSubstringSanitization.js:11:5:11:27 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:11:15:11:26 | "secure.com" | secure.com | -| tst-IncompleteUrlSubstringSanitization.js:12:5:12:27 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:12:15:12:26 | "secure.com" | secure.com | -| tst-IncompleteUrlSubstringSanitization.js:14:5:14:38 | x.start ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:14:18:14:37 | "https://secure.com" | https://secure.com | -| tst-IncompleteUrlSubstringSanitization.js:15:5:15:28 | x.endsW ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:15:16:15:27 | "secure.com" | secure.com | -| tst-IncompleteUrlSubstringSanitization.js:20:5:20:28 | x.inclu ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:20:16:20:27 | "secure.com" | secure.com | -| tst-IncompleteUrlSubstringSanitization.js:32:5:32:35 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:32:15:32:34 | "https://secure.com" | https://secure.com | -| tst-IncompleteUrlSubstringSanitization.js:33:5:33:39 | x.index ... m:443") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:33:15:33:38 | "https: ... om:443" | https://secure.com:443 | -| tst-IncompleteUrlSubstringSanitization.js:34:5:34:36 | x.index ... .com/") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:34:15:34:35 | "https: ... e.com/" | https://secure.com/ | -| tst-IncompleteUrlSubstringSanitization.js:52:5:52:41 | x.index ... ernal") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:52:15:52:40 | "https: ... ternal" | https://example.internal | -| tst-IncompleteUrlSubstringSanitization.js:55:5:55:44 | x.start ... ernal") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:55:18:55:43 | "https: ... ternal" | https://example.internal | -| tst-IncompleteUrlSubstringSanitization.js:56:5:56:45 | x.index ... l.org') | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:56:15:56:44 | 'https: ... al.org' | https://example.internal.org | -| tst-IncompleteUrlSubstringSanitization.js:57:5:57:45 | x.index ... l.org') | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:57:15:57:44 | 'https: ... al.org' | https://example.internal.org | -| tst-IncompleteUrlSubstringSanitization.js:58:5:58:30 | x.endsW ... l.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:58:16:58:29 | "internal.com" | internal.com | -| tst-IncompleteUrlSubstringSanitization.js:61:2:61:24 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:61:12:61:23 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:4:5:4:34 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:4:15:4:26 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:5:5:5:34 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:5:15:5:26 | "secure.net" | secure.net | +| tst-IncompleteUrlSubstringSanitization.js:6:5:6:35 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:6:15:6:27 | ".secure.com" | .secure.com | +| tst-IncompleteUrlSubstringSanitization.js:10:5:10:34 | x.index ... === -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:10:15:10:26 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:11:5:11:33 | x.index ... ) === 0 | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:11:15:11:26 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:12:5:12:32 | x.index ... ") >= 0 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:12:15:12:26 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:14:5:14:38 | x.start ... e.com") | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:14:18:14:37 | "https://secure.com" | https://secure.com | +| tst-IncompleteUrlSubstringSanitization.js:15:5:15:28 | x.endsW ... e.com") | '$@' may be preceded by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:15:16:15:27 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:20:5:20:28 | x.inclu ... e.com") | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:20:16:20:27 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:32:5:32:42 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:32:15:32:34 | "https://secure.com" | https://secure.com | +| tst-IncompleteUrlSubstringSanitization.js:33:5:33:46 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:33:15:33:38 | "https: ... om:443" | https://secure.com:443 | +| tst-IncompleteUrlSubstringSanitization.js:34:5:34:43 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:34:15:34:35 | "https: ... e.com/" | https://secure.com/ | +| tst-IncompleteUrlSubstringSanitization.js:52:5:52:48 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:52:15:52:40 | "https: ... ternal" | https://example.internal | +| tst-IncompleteUrlSubstringSanitization.js:55:5:55:44 | x.start ... ernal") | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:55:18:55:43 | "https: ... ternal" | https://example.internal | +| tst-IncompleteUrlSubstringSanitization.js:56:5:56:51 | x.index ... ) !== 0 | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:56:15:56:44 | 'https: ... al.org' | https://example.internal.org | +| tst-IncompleteUrlSubstringSanitization.js:57:5:57:51 | x.index ... ) === 0 | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:57:15:57:44 | 'https: ... al.org' | https://example.internal.org | +| tst-IncompleteUrlSubstringSanitization.js:58:5:58:30 | x.endsW ... l.com") | '$@' may be preceded by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:58:16:58:29 | "internal.com" | internal.com | +| tst-IncompleteUrlSubstringSanitization.js:61:2:61:31 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:61:12:61:23 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:62:2:62:31 | x.index ... === -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:62:12:62:23 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:63:4:63:33 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:63:14:63:25 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:64:3:64:26 | x.inclu ... e.com") | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:64:14:64:25 | "secure.com" | secure.com | +| tst-IncompleteUrlSubstringSanitization.js:66:6:66:29 | x.inclu ... e.com") | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:66:17:66:28 | "secure.com" | secure.com | diff --git a/javascript/ql/test/query-tests/Security/CWE-020/tst-IncompleteUrlSubstringSanitization.js b/javascript/ql/test/query-tests/Security/CWE-020/tst-IncompleteUrlSubstringSanitization.js index de52d7c0e46..f7246c2a401 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/tst-IncompleteUrlSubstringSanitization.js +++ b/javascript/ql/test/query-tests/Security/CWE-020/tst-IncompleteUrlSubstringSanitization.js @@ -1,11 +1,11 @@ (function(x){ - x.indexOf("internal"); // NOT OK, but not flagged - x.indexOf("localhost"); // NOT OK, but not flagged - x.indexOf("secure.com"); // NOT OK - x.indexOf("secure.net"); // NOT OK - x.indexOf(".secure.com"); // NOT OK - x.indexOf("sub.secure."); // NOT OK, but not flagged - x.indexOf(".sub.secure."); // NOT OK, but not flagged + x.indexOf("internal") !== -1; // NOT OK, but not flagged + x.indexOf("localhost") !== -1; // NOT OK, but not flagged + x.indexOf("secure.com") !== -1; // NOT OK + x.indexOf("secure.net") !== -1; // NOT OK + x.indexOf(".secure.com") !== -1; // NOT OK + x.indexOf("sub.secure.") !== -1; // NOT OK, but not flagged + x.indexOf(".sub.secure.") !== -1; // NOT OK, but not flagged x.indexOf("secure.com") === -1; // NOT OK x.indexOf("secure.com") === 0; // NOT OK @@ -19,38 +19,38 @@ x.includes("secure.com"); // NOT OK - x.indexOf("#"); // OK - x.indexOf(":"); // OK - x.indexOf(":/"); // OK - x.indexOf("://"); // OK - x.indexOf("//"); // OK - x.indexOf(":443"); // OK - x.indexOf("/some/path/"); // OK - x.indexOf("some/path"); // OK - x.indexOf("/index.html"); // OK - x.indexOf(":template:"); // OK - x.indexOf("https://secure.com"); // NOT OK - x.indexOf("https://secure.com:443"); // NOT OK - x.indexOf("https://secure.com/"); // NOT OK + x.indexOf("#") !== -1; // OK + x.indexOf(":") !== -1; // OK + x.indexOf(":/") !== -1; // OK + x.indexOf("://") !== -1; // OK + x.indexOf("//") !== -1; // OK + x.indexOf(":443") !== -1; // OK + x.indexOf("/some/path/") !== -1; // OK + x.indexOf("some/path") !== -1; // OK + x.indexOf("/index.html") !== -1; // OK + x.indexOf(":template:") !== -1; // OK + x.indexOf("https://secure.com") !== -1; // NOT OK + x.indexOf("https://secure.com:443") !== -1; // NOT OK + x.indexOf("https://secure.com/") !== -1; // NOT OK - x.indexOf(".cn"); // NOT OK, but not flagged - x.indexOf(".jpg"); // OK - x.indexOf("index.html"); // OK - x.indexOf("index.js"); // OK - x.indexOf("index.php"); // OK - x.indexOf("index.css"); // OK + x.indexOf(".cn") !== -1; // NOT OK, but not flagged + x.indexOf(".jpg") !== -1; // OK + x.indexOf("index.html") !== -1; // OK + x.indexOf("index.js") !== -1; // OK + x.indexOf("index.php") !== -1; // OK + x.indexOf("index.css") !== -1; // OK - x.indexOf("secure=true"); // OK (query param) - x.indexOf("&auth="); // OK (query param) + x.indexOf("secure=true") !== -1; // OK (query param) + x.indexOf("&auth=") !== -1; // OK (query param) - x.indexOf(getCurrentDomain()); // NOT OK, but not flagged - x.indexOf(location.origin); // NOT OK, but not flagged + x.indexOf(getCurrentDomain()) !== -1; // NOT OK, but not flagged + x.indexOf(location.origin) !== -1; // NOT OK, but not flagged - x.indexOf("tar.gz") + offset // OK - x.indexOf("tar.gz") - offset // OK + x.indexOf("tar.gz") + offset; // OK + x.indexOf("tar.gz") - offset; // OK - x.indexOf("https://example.internal"); // NOT OK - x.indexOf("https://"); // OK + x.indexOf("https://example.internal") !== -1; // NOT OK + x.indexOf("https://") !== -1; // OK x.startsWith("https://example.internal"); // NOT OK x.indexOf('https://example.internal.org') !== 0; // NOT OK @@ -59,4 +59,13 @@ x.startsWith("https://example.internal:80"); // OK x.indexOf("secure.com") !== -1; // NOT OK + x.indexOf("secure.com") === -1; // OK + !(x.indexOf("secure.com") !== -1); // OK + !x.includes("secure.com"); // OK + + if(!x.includes("secure.com")) { // NOT OK + + } else { + doSomeThingWithTrustedURL(x); + } });