hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-17 01:03:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

Go : Improvements to Timing Attacks query

Browse Source
This commit is contained in:
Porcupiney Hairs
2023-07-02 18:27:47 +05:30
parent 4656130dab
commit 74e5c15eaa
3 changed files with 101 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

67
go/ql/src/experimental/CWE-203/Timing.ql
View File

@@ -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"

24
go/ql/test/experimental/CWE-203/Timing.expected
View File

@@ -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 |

30
go/ql/test/experimental/CWE-203/timing.go
View File

@@ -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)
}