From 497bfeee83f5b21acf055fcb0216f05833a1fa5e Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Mon, 27 Jan 2020 11:49:28 -0800 Subject: [PATCH] BadRedirectSanitizer: Use SsaWithFields instead of ValueEntity --- ql/src/Security/CWE-601/BadRedirectCheck.ql | 24 +++++++++---------- .../BadRedirectCheck.expected | 10 ++++---- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.ql b/ql/src/Security/CWE-601/BadRedirectCheck.ql index 5c254924f35..51006b02da5 100644 --- a/ql/src/Security/CWE-601/BadRedirectCheck.ql +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -13,9 +13,9 @@ import go -DataFlow::Node checkForLeadingSlash(ValueEntity v) { +DataFlow::Node checkForLeadingSlash(SsaWithFields v) { exists(StringOps::HasPrefix hp, DataFlow::Node substr | - result = hp and hp.getBaseString() = v.getARead() and hp.getSubstring() = substr + result = hp and hp.getBaseString() = v.getAUse() and hp.getSubstring() = substr | substr.getStringValue() = "/" or @@ -23,8 +23,8 @@ DataFlow::Node checkForLeadingSlash(ValueEntity v) { ) } -DataFlow::Node checkForSecondSlash(ValueEntity v) { - exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getARead() | +DataFlow::Node checkForSecondSlash(SsaWithFields v) { + exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getAUse() | hp.getSubstring().getStringValue() = "//" ) or @@ -32,14 +32,14 @@ DataFlow::Node checkForSecondSlash(ValueEntity v) { result = eq | slash.getIntValue() = 47 and // ASCII value for '/' - er.getBase() = v.getARead() and + er.getBase() = v.getAUse() and er.getIndex().getIntValue() = 1 and eq.eq(_, er, slash) ) } -DataFlow::Node checkForSecondBackslash(ValueEntity v) { - exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getARead() | +DataFlow::Node checkForSecondBackslash(SsaWithFields v) { + exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getAUse() | hp.getSubstring().getStringValue() = "/\\" ) or @@ -47,21 +47,21 @@ DataFlow::Node checkForSecondBackslash(ValueEntity v) { result = eq | slash.getIntValue() = 92 and // ASCII value for '\' - er.getBase() = v.getARead() and + er.getBase() = v.getAUse() and er.getIndex().getIntValue() = 1 and eq.eq(_, er, slash) ) } -predicate isBadRedirectCheck(DataFlow::Node node, ValueEntity v) { +predicate isBadRedirectCheck(DataFlow::Node node, SsaWithFields v) { node = checkForLeadingSlash(v) and not (exists(checkForSecondSlash(v)) and exists(checkForSecondBackslash(v))) } -from DataFlow::Node node, ValueEntity v +from DataFlow::Node node, SsaWithFields v where isBadRedirectCheck(node, v) and - v.getName().regexpMatch("(?i).*url.*|.*redir.*|.*target.*") + v.getQualifiedName().regexpMatch("(?i).*url.*|.*redir.*|.*target.*") select node, "This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position.", - v, v.getName() + v, v.getQualifiedName() diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected index 4735977860c..e2830337608 100644 --- a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected @@ -1,5 +1,5 @@ -| BadRedirectCheck.go:4:23:4:37 | ...==... | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | redir | redir | -| cves.go:11:26:11:38 | ...==... | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | url | url | -| cves.go:22:6:22:37 | call to HasPrefix | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | redirect | redirect | -| cves.go:29:6:29:37 | call to HasPrefix | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | redirect | redirect | -| main.go:8:7:8:38 | call to HasPrefix | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | redirect | redirect | +| BadRedirectCheck.go:4:23:4:37 | ...==... | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | BadRedirectCheck.go:3:18:3:22 | (def@3:18) | redir | +| cves.go:11:26:11:38 | ...==... | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | cves.go:10:22:10:24 | (def@10:22) | url | +| cves.go:22:6:22:37 | call to HasPrefix | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | cves.go:21:2:21:9 | (def@21:2) | redirect | +| cves.go:29:6:29:37 | call to HasPrefix | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | cves.go:28:2:28:9 | (def@28:2) | redirect | +| main.go:8:7:8:38 | call to HasPrefix | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | main.go:5:19:5:26 | (def@5:19) | redirect |