From cdee44bbd11c718d76f3cd078877235cca6f964c Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 27 Oct 2021 10:31:03 +0100 Subject: [PATCH 1/4] Add barrier guard for comparison --- .../Security/CWE-326/InsufficientKeySize.ql | 12 ++++++++ .../CWE-326/InsufficientKeySize.expected | 8 +++++ .../Security/CWE-326/InsufficientKeySize.go | 30 +++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/ql/src/Security/CWE-326/InsufficientKeySize.ql b/ql/src/Security/CWE-326/InsufficientKeySize.ql index 6e1096f7708..6166d62c6a1 100644 --- a/ql/src/Security/CWE-326/InsufficientKeySize.ql +++ b/ql/src/Security/CWE-326/InsufficientKeySize.ql @@ -27,6 +27,18 @@ class RsaKeyTrackingConfiguration extends DataFlow::Configuration { c.getTarget().hasQualifiedName("crypto/rsa", "GenerateKey") ) } + + override predicate isBarrierGuard(DataFlow::BarrierGuard guard) { + guard instanceof ComparisonBarrierGuard + } +} + +class ComparisonBarrierGuard extends DataFlow::BarrierGuard, DataFlow::RelationalComparisonNode { + override predicate checks(Expr e, boolean branch) { + exists(DataFlow::Node lesser , DataFlow::Node greater, int bias | this.leq(branch, lesser, greater, bias) | + globalValueNumber(DataFlow::exprNode(e)) = globalValueNumber(greater) and lesser.getIntValue() - bias >= 2048 + ) + } } from RsaKeyTrackingConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink diff --git a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.expected b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.expected index c7840f6adb5..f8c4e0998c8 100644 --- a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.expected +++ b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.expected @@ -2,6 +2,8 @@ edges | InsufficientKeySize.go:13:10:13:13 | 1024 : int | InsufficientKeySize.go:14:31:14:34 | size | | InsufficientKeySize.go:18:7:18:10 | 1024 : int | InsufficientKeySize.go:25:11:25:14 | definition of size : int | | InsufficientKeySize.go:25:11:25:14 | definition of size : int | InsufficientKeySize.go:26:31:26:34 | size | +| InsufficientKeySize.go:30:13:30:16 | 1024 : int | InsufficientKeySize.go:32:32:32:38 | keyBits | +| InsufficientKeySize.go:44:13:44:16 | 1024 : int | InsufficientKeySize.go:47:32:47:38 | keyBits | nodes | InsufficientKeySize.go:9:31:9:34 | 1024 | semmle.label | 1024 | | InsufficientKeySize.go:13:10:13:13 | 1024 : int | semmle.label | 1024 : int | @@ -9,7 +11,13 @@ nodes | InsufficientKeySize.go:18:7:18:10 | 1024 : int | semmle.label | 1024 : int | | InsufficientKeySize.go:25:11:25:14 | definition of size : int | semmle.label | definition of size : int | | InsufficientKeySize.go:26:31:26:34 | size | semmle.label | size | +| InsufficientKeySize.go:30:13:30:16 | 1024 : int | semmle.label | 1024 : int | +| InsufficientKeySize.go:32:32:32:38 | keyBits | semmle.label | keyBits | +| InsufficientKeySize.go:44:13:44:16 | 1024 : int | semmle.label | 1024 : int | +| InsufficientKeySize.go:47:32:47:38 | keyBits | semmle.label | keyBits | #select | InsufficientKeySize.go:9:31:9:34 | 1024 | InsufficientKeySize.go:9:31:9:34 | 1024 | InsufficientKeySize.go:9:31:9:34 | 1024 | The size of this RSA key should be at least 2048 bits. | | InsufficientKeySize.go:14:31:14:34 | size | InsufficientKeySize.go:13:10:13:13 | 1024 : int | InsufficientKeySize.go:14:31:14:34 | size | The size of this RSA key should be at least 2048 bits. | | InsufficientKeySize.go:26:31:26:34 | size | InsufficientKeySize.go:18:7:18:10 | 1024 : int | InsufficientKeySize.go:26:31:26:34 | size | The size of this RSA key should be at least 2048 bits. | +| InsufficientKeySize.go:32:32:32:38 | keyBits | InsufficientKeySize.go:30:13:30:16 | 1024 : int | InsufficientKeySize.go:32:32:32:38 | keyBits | The size of this RSA key should be at least 2048 bits. | +| InsufficientKeySize.go:47:32:47:38 | keyBits | InsufficientKeySize.go:44:13:44:16 | 1024 : int | InsufficientKeySize.go:47:32:47:38 | keyBits | The size of this RSA key should be at least 2048 bits. | diff --git a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go index ad1f1c8de75..d91d2cfa7cd 100644 --- a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go +++ b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go @@ -25,3 +25,33 @@ func foo4() { func foo5(size int) { rsa.GenerateKey(rand.Reader, size) } + +func foo6() { + keyBits := 1024 + if keyBits >= 2047 { + rsa.GenerateKey(rand.Reader, keyBits) // BAD + } +} + +func foo7() { + keyBits := 1024 + if keyBits >= 2048 { + rsa.GenerateKey(rand.Reader, keyBits) // GOOD + } +} + +func foo8() { + keyBits := 1024 + switch { + case keyBits >= 2047: + rsa.GenerateKey(rand.Reader, keyBits) // BAD + } +} + +func foo9() { + keyBits := 1024 + switch { + case keyBits >= 2048: + rsa.GenerateKey(rand.Reader, keyBits) // GOOD + } +} From e0e1a4671a687f493f3b9a609fc3d47de5e5df2a Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 27 Oct 2021 15:02:29 +0100 Subject: [PATCH 2/4] Address review comments --- ql/src/Security/CWE-326/InsufficientKeySize.ql | 13 ++++++++++--- .../Security/CWE-326/InsufficientKeySize.expected | 4 ++++ .../Security/CWE-326/InsufficientKeySize.go | 11 +++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/ql/src/Security/CWE-326/InsufficientKeySize.ql b/ql/src/Security/CWE-326/InsufficientKeySize.ql index 6166d62c6a1..ab5a40bf621 100644 --- a/ql/src/Security/CWE-326/InsufficientKeySize.ql +++ b/ql/src/Security/CWE-326/InsufficientKeySize.ql @@ -33,10 +33,17 @@ class RsaKeyTrackingConfiguration extends DataFlow::Configuration { } } -class ComparisonBarrierGuard extends DataFlow::BarrierGuard, DataFlow::RelationalComparisonNode { +/** + * A comparison which guarantees that an expression is at least 2048, + * considered as a barrier guard for key sizes. + */ +class ComparisonBarrierGuard extends DataFlow::BarrierGuard instanceof DataFlow::RelationalComparisonNode { override predicate checks(Expr e, boolean branch) { - exists(DataFlow::Node lesser , DataFlow::Node greater, int bias | this.leq(branch, lesser, greater, bias) | - globalValueNumber(DataFlow::exprNode(e)) = globalValueNumber(greater) and lesser.getIntValue() - bias >= 2048 + exists(DataFlow::Node lesser, DataFlow::Node greater, int bias | + super.leq(branch, lesser, greater, bias) + | + globalValueNumber(DataFlow::exprNode(e)) = globalValueNumber(greater) and + lesser.getIntValue() - bias >= 2048 ) } } diff --git a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.expected b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.expected index f8c4e0998c8..fc7765fa889 100644 --- a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.expected +++ b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.expected @@ -4,6 +4,7 @@ edges | InsufficientKeySize.go:25:11:25:14 | definition of size : int | InsufficientKeySize.go:26:31:26:34 | size | | InsufficientKeySize.go:30:13:30:16 | 1024 : int | InsufficientKeySize.go:32:32:32:38 | keyBits | | InsufficientKeySize.go:44:13:44:16 | 1024 : int | InsufficientKeySize.go:47:32:47:38 | keyBits | +| InsufficientKeySize.go:61:21:61:24 | 1024 : int | InsufficientKeySize.go:67:31:67:37 | keyBits | nodes | InsufficientKeySize.go:9:31:9:34 | 1024 | semmle.label | 1024 | | InsufficientKeySize.go:13:10:13:13 | 1024 : int | semmle.label | 1024 : int | @@ -15,9 +16,12 @@ nodes | InsufficientKeySize.go:32:32:32:38 | keyBits | semmle.label | keyBits | | InsufficientKeySize.go:44:13:44:16 | 1024 : int | semmle.label | 1024 : int | | InsufficientKeySize.go:47:32:47:38 | keyBits | semmle.label | keyBits | +| InsufficientKeySize.go:61:21:61:24 | 1024 : int | semmle.label | 1024 : int | +| InsufficientKeySize.go:67:31:67:37 | keyBits | semmle.label | keyBits | #select | InsufficientKeySize.go:9:31:9:34 | 1024 | InsufficientKeySize.go:9:31:9:34 | 1024 | InsufficientKeySize.go:9:31:9:34 | 1024 | The size of this RSA key should be at least 2048 bits. | | InsufficientKeySize.go:14:31:14:34 | size | InsufficientKeySize.go:13:10:13:13 | 1024 : int | InsufficientKeySize.go:14:31:14:34 | size | The size of this RSA key should be at least 2048 bits. | | InsufficientKeySize.go:26:31:26:34 | size | InsufficientKeySize.go:18:7:18:10 | 1024 : int | InsufficientKeySize.go:26:31:26:34 | size | The size of this RSA key should be at least 2048 bits. | | InsufficientKeySize.go:32:32:32:38 | keyBits | InsufficientKeySize.go:30:13:30:16 | 1024 : int | InsufficientKeySize.go:32:32:32:38 | keyBits | The size of this RSA key should be at least 2048 bits. | | InsufficientKeySize.go:47:32:47:38 | keyBits | InsufficientKeySize.go:44:13:44:16 | 1024 : int | InsufficientKeySize.go:47:32:47:38 | keyBits | The size of this RSA key should be at least 2048 bits. | +| InsufficientKeySize.go:67:31:67:37 | keyBits | InsufficientKeySize.go:61:21:61:24 | 1024 : int | InsufficientKeySize.go:67:31:67:37 | keyBits | The size of this RSA key should be at least 2048 bits. | diff --git a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go index d91d2cfa7cd..bcfac4b4937 100644 --- a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go +++ b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go @@ -55,3 +55,14 @@ func foo9() { rsa.GenerateKey(rand.Reader, keyBits) // GOOD } } + +func foo10(customOptionSupplied bool, nonConstantKeyBits int) { + keyBits := 0 + constantKeyBits := 1024 + if customOptionSupplied { + keyBits = constantKeyBits + } else { + keyBits = nonConstantKeyBits + } + rsa.GenerateKey(rand.Reader, keyBits) // BAD +} From 599c276fd8b098acfc6401be1c28d5ecce8b24fc Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 27 Oct 2021 15:05:54 +0100 Subject: [PATCH 3/4] Add change note --- change-notes/2021-10-27-insufficient-key-size-sanitizer.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 change-notes/2021-10-27-insufficient-key-size-sanitizer.md diff --git a/change-notes/2021-10-27-insufficient-key-size-sanitizer.md b/change-notes/2021-10-27-insufficient-key-size-sanitizer.md new file mode 100644 index 00000000000..5cd7a616e08 --- /dev/null +++ b/change-notes/2021-10-27-insufficient-key-size-sanitizer.md @@ -0,0 +1,3 @@ +lgtm,codescanning +* The query "Use of a weak cryptographic key" has been improved to recognize more cases where the + key size should be considered to be safe, which should lead to fewer false positive results. From 004beab750a1c46500d38b981eccf2339b361fe5 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 29 Oct 2021 11:00:42 +0100 Subject: [PATCH 4/4] Add a good variant of test case foo10 --- .../Security/CWE-326/InsufficientKeySize.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go index bcfac4b4937..9d5ce2ac424 100644 --- a/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go +++ b/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.go @@ -66,3 +66,16 @@ func foo10(customOptionSupplied bool, nonConstantKeyBits int) { } rsa.GenerateKey(rand.Reader, keyBits) // BAD } + +func foo11(customOptionSupplied bool, nonConstantKeyBits int) { + keyBits := 0 + constantKeyBits := 1024 + if customOptionSupplied { + keyBits = constantKeyBits + } else { + keyBits = nonConstantKeyBits + } + if keyBits >= 2048 { + rsa.GenerateKey(rand.Reader, keyBits) // GOOD + } +}