From 74e5c15eaa0392005be301f3bdcf2e1af9ffb9d2 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Sun, 2 Jul 2023 18:27:47 +0530 Subject: [PATCH] Go : Improvements to Timing Attacks query --- go/ql/src/experimental/CWE-203/Timing.ql | 67 +++++++++++++++---- .../test/experimental/CWE-203/Timing.expected | 24 +++++-- go/ql/test/experimental/CWE-203/timing.go | 30 ++++++++- 3 files changed, 101 insertions(+), 20 deletions(-) diff --git a/go/ql/src/experimental/CWE-203/Timing.ql b/go/ql/src/experimental/CWE-203/Timing.ql index a22fd8727cd..b9cfa0f1a6d 100644 --- a/go/ql/src/experimental/CWE-203/Timing.ql +++ b/go/ql/src/experimental/CWE-203/Timing.ql @@ -24,16 +24,38 @@ private predicate isBadResult(DataFlow::Node e) { */ abstract class Sink extends DataFlow::Node { } -/** A taint-tracking sink which models comparisons of sensitive variables. */ -private class SensitiveCompareSink extends Sink { - ComparisonExpr c; +/** A taint-tracking sink which models comparisons of sensitive expressions using `strings.Compare` function. */ +private class SensitiveStringCompareSink extends Sink { + SensitiveStringCompareSink() { + // We select a comparison where a secret or password is tested. + exists(DataFlow::CallNode c, Expr op1, Expr nonSensitiveOperand | + c.getTarget().hasQualifiedName("strings", "Compare") and + c.getArgument(_).asExpr() = op1 and + op1.(SensitiveVariableAccess).getClassification() = + [SensitiveExpr::secret(), SensitiveExpr::password()] and + c.getArgument(_).asExpr() = nonSensitiveOperand and + not op1 = nonSensitiveOperand and + not ( + // Comparisons with `nil` should be excluded. + nonSensitiveOperand = Builtin::nil().getAReference() + or + // Comparisons with empty string should also be excluded. + nonSensitiveOperand.getStringValue().length() = 0 + ) + | + // It is important to note that the name of both the operands need not be + // `sensitive`. Even if one of the operands appears to be sensitive, we consider it a potential sink. + nonSensitiveOperand = this.asExpr() + ) + } +} +/** A taint-tracking sink which models comparisons of sensitive expressions. */ +private class SensitiveCompareSink extends Sink { SensitiveCompareSink() { // We select a comparison where a secret or password is tested. - exists(SensitiveVariableAccess op1, Expr op2 | + exists(SensitiveExpr op1, Expr op2, EqualityTestExpr c | op1.getClassification() = [SensitiveExpr::secret(), SensitiveExpr::password()] and - // exclude grant to avoid FP from OAuth - not op1.getClassification().matches("%grant%") and op1 = c.getAnOperand() and op2 = c.getAnOperand() and not op1 = op2 and @@ -45,13 +67,34 @@ private class SensitiveCompareSink extends Sink { op2.getStringValue().length() = 0 ) | - // It is important to note that the name of both the operands need not be - // `sensitive`. Even if one of the operands appears to be sensitive, we consider it a potential sink. - c.getAnOperand() = this.asExpr() + op2 = this.asExpr() ) } +} - DataFlow::Node getOtherOperand() { result.asExpr() = c.getAnOperand() and not result = this } +/** A taint-tracking sink which models comparisons of sensitive strings. */ +private class SensitiveStringSink extends Sink { + SensitiveStringSink() { + // We select a comparison where a secret or password is tested. + exists(StringLit op1, Expr op2, EqualityTestExpr c | + op1.getStringValue() + .regexpMatch(HeuristicNames::maybeSensitive([ + SensitiveExpr::secret(), SensitiveExpr::password() + ])) and + op1 = c.getAnOperand() and + op2 = c.getAnOperand() and + not op1 = op2 and + not ( + // Comparisons with `nil` should be excluded. + op2 = Builtin::nil().getAReference() + or + // Comparisons with empty string should also be excluded. + op2.getStringValue().length() = 0 + ) + | + op2 = this.asExpr() + ) + } } class SecretTracking extends TaintTracking::Configuration { @@ -65,8 +108,6 @@ class SecretTracking extends TaintTracking::Configuration { } from SecretTracking cfg, DataFlow::PathNode source, DataFlow::PathNode sink -where - cfg.hasFlowPath(source, sink) and - not cfg.hasFlowTo(sink.getNode().(SensitiveCompareSink).getOtherOperand()) +where cfg.hasFlowPath(source, sink) select sink.getNode(), source, sink, "$@ may be vulnerable to timing attacks.", source.getNode(), "Hardcoded String" diff --git a/go/ql/test/experimental/CWE-203/Timing.expected b/go/ql/test/experimental/CWE-203/Timing.expected index a94866cda5a..05e19774dbc 100644 --- a/go/ql/test/experimental/CWE-203/Timing.expected +++ b/go/ql/test/experimental/CWE-203/Timing.expected @@ -1,10 +1,22 @@ edges -| timing.go:14:18:14:27 | selection of Header | timing.go:14:18:14:45 | call to Get | -| timing.go:14:18:14:45 | call to Get | timing.go:16:25:16:36 | headerSecret | +| timing.go:15:18:15:27 | selection of Header | timing.go:15:18:15:45 | call to Get | +| timing.go:15:18:15:45 | call to Get | timing.go:17:31:17:42 | headerSecret | +| timing.go:28:18:28:27 | selection of Header | timing.go:28:18:28:45 | call to Get | +| timing.go:28:18:28:45 | call to Get | timing.go:30:47:30:58 | headerSecret | +| timing.go:41:18:41:27 | selection of Header | timing.go:41:18:41:45 | call to Get | +| timing.go:41:18:41:45 | call to Get | timing.go:42:25:42:36 | headerSecret | nodes -| timing.go:14:18:14:27 | selection of Header | semmle.label | selection of Header | -| timing.go:14:18:14:45 | call to Get | semmle.label | call to Get | -| timing.go:16:25:16:36 | headerSecret | semmle.label | headerSecret | +| timing.go:15:18:15:27 | selection of Header | semmle.label | selection of Header | +| timing.go:15:18:15:45 | call to Get | semmle.label | call to Get | +| timing.go:17:31:17:42 | headerSecret | semmle.label | headerSecret | +| timing.go:28:18:28:27 | selection of Header | semmle.label | selection of Header | +| timing.go:28:18:28:45 | call to Get | semmle.label | call to Get | +| timing.go:30:47:30:58 | headerSecret | semmle.label | headerSecret | +| timing.go:41:18:41:27 | selection of Header | semmle.label | selection of Header | +| timing.go:41:18:41:45 | call to Get | semmle.label | call to Get | +| timing.go:42:25:42:36 | headerSecret | semmle.label | headerSecret | subpaths #select -| timing.go:16:25:16:36 | headerSecret | timing.go:14:18:14:27 | selection of Header | timing.go:16:25:16:36 | headerSecret | $@ may be vulnerable to timing attacks. | timing.go:14:18:14:27 | selection of Header | Hardcoded String | +| timing.go:17:31:17:42 | headerSecret | timing.go:15:18:15:27 | selection of Header | timing.go:17:31:17:42 | headerSecret | $@ may be vulnerable to timing attacks. | timing.go:15:18:15:27 | selection of Header | Hardcoded String | +| timing.go:30:47:30:58 | headerSecret | timing.go:28:18:28:27 | selection of Header | timing.go:30:47:30:58 | headerSecret | $@ may be vulnerable to timing attacks. | timing.go:28:18:28:27 | selection of Header | Hardcoded String | +| timing.go:42:25:42:36 | headerSecret | timing.go:41:18:41:27 | selection of Header | timing.go:42:25:42:36 | headerSecret | $@ may be vulnerable to timing attacks. | timing.go:41:18:41:27 | selection of Header | Hardcoded String | diff --git a/go/ql/test/experimental/CWE-203/timing.go b/go/ql/test/experimental/CWE-203/timing.go index 627d1a59a36..43401bd4111 100644 --- a/go/ql/test/experimental/CWE-203/timing.go +++ b/go/ql/test/experimental/CWE-203/timing.go @@ -4,6 +4,7 @@ import ( "crypto/subtle" "fmt" "net/http" + "strings" ) func bad(w http.ResponseWriter, req *http.Request) (interface{}, error) { @@ -13,7 +14,32 @@ func bad(w http.ResponseWriter, req *http.Request) (interface{}, error) { headerSecret := req.Header.Get(secretHeader) secretStr := string(secret) - if len(secret) != 0 && headerSecret != secretStr { + if len(headerSecret) != 0 && headerSecret != secretStr { + return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret) + } + return nil, nil +} + +func bad2(w http.ResponseWriter, req *http.Request) (interface{}, error) { + + secret := "MySuperSecretPasscode" + secretHeader := "X-Secret" + + headerSecret := req.Header.Get(secretHeader) + secretStr := string(secret) + if len(headerSecret) != 0 && strings.Compare(headerSecret, secretStr) != 0 { + return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret) + } + return nil, nil +} + +func bad4(w http.ResponseWriter, req *http.Request) (interface{}, error) { + + secret := "MySuperSecretPasscode" + secretHeader := "X-Secret" + + headerSecret := req.Header.Get(secretHeader) + if len(secret) != 0 && headerSecret != "SecretStringLiteral" { return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret) } return nil, nil @@ -34,4 +60,6 @@ func good(w http.ResponseWriter, req *http.Request) (interface{}, error) { func main() { bad(nil, nil) good(nil, nil) + bad2(nil, nil) + bad4(nil, nil) }