From a7e549e771b7ac8bf73a5915c17bf374ca449503 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 14 Jul 2020 14:30:53 +0100 Subject: [PATCH] Exclude TLS version sources accompanied by a non-nil error It is common to return 0 has a dummy value with an error; these are very likely not going to be used as a real TLS version. --- ql/src/experimental/CWE-327/InsecureTLS.ql | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/ql/src/experimental/CWE-327/InsecureTLS.ql b/ql/src/experimental/CWE-327/InsecureTLS.ql index 023ca4669a7..a355932a6bf 100644 --- a/ql/src/experimental/CWE-327/InsecureTLS.ql +++ b/ql/src/experimental/CWE-327/InsecureTLS.ql @@ -83,13 +83,30 @@ predicate nodeSuggestsOldVersion(AstNode node) { exprSuggestsOldVersion(node.(CaseClause).getAnExpr()) } +/** + * Holds if `node` refers to a value returned alongside a non-nil error value. + * + * For example, `0` in `func tryGetInt() (int, error) { return 0, errors.New("no good") }` + */ +predicate isReturnedWithError(DataFlow::Node node) { + exists(ReturnStmt ret | + ret.getExpr(0) = node.asExpr() and + ret.getNumExpr() = 2 and + ret.getExpr(1).getType().implements(Builtin::error().getType().getUnderlyingType()) and + ret.getExpr(1) != Builtin::nil().getAReference() + ) +} + /** * Flow of TLS versions into a `tls.Config` struct, to the `MinVersion` and `MaxVersion` fields. */ class TlsVersionFlowConfig extends TaintTracking::Configuration { TlsVersionFlowConfig() { this = "TlsVersionFlowConfig" } - predicate isSource(DataFlow::Node source, int val) { val = source.getIntValue() } + predicate isSource(DataFlow::Node source, int val) { + val = source.getIntValue() and + not isReturnedWithError(source) + } predicate isSink(DataFlow::Node sink, Field fld, DataFlow::Node base, Write fieldWrite) { fld.hasQualifiedName("crypto/tls", "Config", ["MinVersion", "MaxVersion"]) and