Skip to content

Java: Promote insufficient key size query from experimental #10785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 59 commits into from
Nov 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
9b7df35
move files
Sep 29, 2022
3643c9e
update metadata
Sep 29, 2022
657e1e6
start refactoring query logic into lib file
Sep 29, 2022
9eb45c3
refactor tests and code, update help file
Oct 3, 2022
7d94590
add change note
Oct 3, 2022
75794ec
false negative testing - before rewrite for variable dataflow
Oct 4, 2022
7de9c05
use CompileTimeConstantExpr for FN with VarAccess, and remove KeyGene…
Oct 4, 2022
d3b1a04
handle FN case with simple VarAccess; add draft of dataflow config to…
Oct 5, 2022
8ffd252
add draft code to find algo type to replace tainttracking configs
Oct 5, 2022
ac70719
commit before adding taint flow back (since no taint flow doesn't cap…
Oct 5, 2022
5e2ef66
refactoring to use both dataflow configs; commit before deleting unus…
Oct 6, 2022
c414ee0
add ECC dataflow config; passes all test cases; still don't have algo…
Oct 6, 2022
cdac0e2
add local algo name tracking, still need to add ability to track algo…
Oct 7, 2022
b7123c1
draft of adding kpg tracking into dataflow config
Oct 8, 2022
b0af9f9
added kg taintracking config to all
Oct 10, 2022
f5a2fef
update tests for non-path version
Oct 10, 2022
3cc7f14
clean up code somewhat
Oct 10, 2022
0c2cff2
updates from discussing with Tony
Oct 10, 2022
bd76b1f
clean-up and update configurations to have specs as sink
Oct 10, 2022
b6a8c27
delete experimental files
Oct 10, 2022
e64825f
fix code-scanning bot problems
Oct 11, 2022
26f4abf
remove globalflow for key(pair)gen
Oct 11, 2022
3e8748e
add path-graph back to query alerts
Oct 11, 2022
29de0c6
make one config for asymm with flow states; seems to work...
Oct 12, 2022
01c2a8c
add symm to the single config; still seems to work
Oct 12, 2022
0fc4a33
remove commented-out code
Oct 12, 2022
37d8558
refactor code into InsufficientKeySize.qll
Oct 12, 2022
bfbb6db
clean up code
Oct 12, 2022
bcb506b
add placeholder qldocs
Oct 12, 2022
e0f0d55
condense code
Oct 13, 2022
2daa345
combine three configs into one
Oct 13, 2022
c61f23b
experiment with more code condensing
Oct 14, 2022
6eb58d8
remove dependence on typeFlag
Oct 14, 2022
47030df
remove commented-out 3 configs
Oct 14, 2022
0334470
remove commented out predicates that relied on typeFlag
Oct 14, 2022
da218fd
clean up code
Oct 14, 2022
2714c7f
update tests
Oct 14, 2022
5f39888
minor code restructure
Oct 17, 2022
383b8a8
update select statement to be closer to cpp's
Oct 19, 2022
ff557a2
add min key size predicates
Oct 19, 2022
dc8b62b
add support for AlgorithmParameterGenerator
Oct 19, 2022
4df0fbc
update tests
Oct 19, 2022
961e5c7
minor updates
Oct 19, 2022
e5982f1
minor updates
Oct 19, 2022
b7f3606
rename change note
Oct 19, 2022
345e4e0
remove unnecessary 'exists'
Oct 21, 2022
4c8e0a7
update qldoc of JavaSecurityKeyPairGenerator and JavaSecurityAlgoPara…
Oct 24, 2022
2ee23f0
update qldoc for AlgorithmParameterSpec
Oct 24, 2022
eb69b98
remove separators
Oct 24, 2022
8bc0a64
remove KeyGenInitMethodAccess class
Oct 24, 2022
09829d7
simplify instanceof usage
Oct 24, 2022
d569f93
update getAlgoSpec
Oct 24, 2022
c742a09
remove AlgoSpec class
Oct 24, 2022
1a12453
remove getNodeIntValue
Oct 24, 2022
1e80fa1
add modules
Oct 25, 2022
1bfdfc9
shorten class/predicate names
Oct 26, 2022
65f7474
simplify algorithm.matches
Oct 27, 2022
f40eefc
use CompileTimeConstantExpr instead of StringLiteral
Oct 27, 2022
910eebc
update change note
Nov 3, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 68 additions & 2 deletions java/ql/lib/semmle/code/java/security/Encryption.qll
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,22 @@ class KeyPairGenerator extends RefType {
KeyPairGenerator() { this.hasQualifiedName("java.security", "KeyPairGenerator") }
}

/** The `init` method declared in `javax.crypto.KeyGenerator`. */
class KeyGeneratorInitMethod extends Method {
KeyGeneratorInitMethod() {
this.getDeclaringType() instanceof KeyGenerator and
this.hasName("init")
}
}

/** The `initialize` method declared in `java.security.KeyPairGenerator`. */
class KeyPairGeneratorInitMethod extends Method {
KeyPairGeneratorInitMethod() {
this.getDeclaringType() instanceof KeyPairGenerator and
this.hasName("initialize")
}
}

/** The `verify` method of the class `javax.net.ssl.HostnameVerifier`. */
class HostnameVerifierVerify extends Method {
HostnameVerifierVerify() {
Expand Down Expand Up @@ -367,8 +383,8 @@ class JavaSecuritySignature extends JavaSecurityAlgoSpec {
override Expr getAlgoSpec() { result = this.(ConstructorCall).getArgument(0) }
}

/** A method call to the Java class `java.security.KeyPairGenerator`. */
class JavaSecurityKeyPairGenerator extends JavaxCryptoAlgoSpec {
/** A call to the `getInstance` method declared in `java.security.KeyPairGenerator`. */
class JavaSecurityKeyPairGenerator extends JavaSecurityAlgoSpec {
JavaSecurityKeyPairGenerator() {
exists(Method m | m.getAReference() = this |
m.getDeclaringType() instanceof KeyPairGenerator and
Expand All @@ -378,3 +394,53 @@ class JavaSecurityKeyPairGenerator extends JavaxCryptoAlgoSpec {

override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) }
}

/** The Java class `java.security.AlgorithmParameterGenerator`. */
class AlgorithmParameterGenerator extends RefType {
AlgorithmParameterGenerator() {
this.hasQualifiedName("java.security", "AlgorithmParameterGenerator")
}
}

/** The `init` method declared in `java.security.AlgorithmParameterGenerator`. */
class AlgoParamGeneratorInitMethod extends Method {
AlgoParamGeneratorInitMethod() {
this.getDeclaringType() instanceof AlgorithmParameterGenerator and
this.hasName("init")
}
}

/** A call to the `getInstance` method declared in `java.security.AlgorithmParameterGenerator`. */
class JavaSecurityAlgoParamGenerator extends JavaSecurityAlgoSpec {
JavaSecurityAlgoParamGenerator() {
exists(Method m | m.getAReference() = this |
m.getDeclaringType() instanceof AlgorithmParameterGenerator and
m.getName() = "getInstance"
)
}

override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) }
}

/** An implementation of the `java.security.spec.AlgorithmParameterSpec` interface. */
abstract class AlgorithmParameterSpec extends RefType { }

/** The Java class `java.security.spec.ECGenParameterSpec`. */
class EcGenParameterSpec extends AlgorithmParameterSpec {
EcGenParameterSpec() { this.hasQualifiedName("java.security.spec", "ECGenParameterSpec") }
}

/** The Java class `java.security.spec.RSAKeyGenParameterSpec`. */
class RsaKeyGenParameterSpec extends AlgorithmParameterSpec {
RsaKeyGenParameterSpec() { this.hasQualifiedName("java.security.spec", "RSAKeyGenParameterSpec") }
}

/** The Java class `java.security.spec.DSAGenParameterSpec`. */
class DsaGenParameterSpec extends AlgorithmParameterSpec {
DsaGenParameterSpec() { this.hasQualifiedName("java.security.spec", "DSAGenParameterSpec") }
}

/** The Java class `javax.crypto.spec.DHGenParameterSpec`. */
class DhGenParameterSpec extends AlgorithmParameterSpec {
DhGenParameterSpec() { this.hasQualifiedName("javax.crypto.spec", "DHGenParameterSpec") }
}
193 changes: 193 additions & 0 deletions java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
/** Provides classes and predicates related to insufficient key sizes in Java. */

private import semmle.code.java.security.Encryption
private import semmle.code.java.dataflow.DataFlow

/** A source for an insufficient key size. */
abstract class InsufficientKeySizeSource extends DataFlow::Node {
/** Holds if this source has the specified `state`. */
predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty }
}

/** A sink for an insufficient key size. */
abstract class InsufficientKeySizeSink extends DataFlow::Node {
/** Holds if this sink has the specified `state`. */
predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty }
}

/** Provides models for asymmetric cryptography. */
private module Asymmetric {
/** Provides models for non-elliptic-curve asymmetric cryptography. */
private module NonEllipticCurve {
/** A source for an insufficient key size used in RSA, DSA, and DH algorithms. */
private class Source extends InsufficientKeySizeSource {
Source() { this.asExpr().(IntegerLiteral).getIntValue() < getMinKeySize() }

override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
}

/** A sink for an insufficient key size used in RSA, DSA, and DH algorithms. */
private class Sink extends InsufficientKeySizeSink {
Sink() {
exists(KeyPairGenInit kpgInit, KeyPairGen kpg |
kpg.getAlgoName().matches(["RSA", "DSA", "DH"]) and
DataFlow::localExprFlow(kpg, kpgInit.getQualifier()) and
this.asExpr() = kpgInit.getKeySizeArg()
)
or
exists(Spec spec | this.asExpr() = spec.getKeySizeArg())
}

override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
}

/** Returns the minimum recommended key size for RSA, DSA, and DH algorithms. */
private int getMinKeySize() { result = 2048 }

/** An instance of an RSA, DSA, or DH algorithm specification. */
private class Spec extends ClassInstanceExpr {
Spec() {
this.getConstructedType() instanceof RsaKeyGenParameterSpec or
this.getConstructedType() instanceof DsaGenParameterSpec or
this.getConstructedType() instanceof DhGenParameterSpec
}

/** Gets the `keysize` argument of this instance. */
Argument getKeySizeArg() { result = this.getArgument(0) }
}
}

/** Provides models for elliptic-curve asymmetric cryptography. */
private module EllipticCurve {
/** A source for an insufficient key size used in elliptic curve (EC) algorithms. */
private class Source extends InsufficientKeySizeSource {
Source() {
this.asExpr().(IntegerLiteral).getIntValue() < getMinKeySize()
or
// the below is needed for cases when the key size is embedded in the curve name
getKeySize(this.asExpr().(StringLiteral).getValue()) < getMinKeySize()
}

override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
}

/** A sink for an insufficient key size used in elliptic curve (EC) algorithms. */
private class Sink extends InsufficientKeySizeSink {
Sink() {
exists(KeyPairGenInit kpgInit, KeyPairGen kpg |
kpg.getAlgoName().matches("EC%") and
DataFlow::localExprFlow(kpg, kpgInit.getQualifier()) and
this.asExpr() = kpgInit.getKeySizeArg()
)
or
exists(Spec s | this.asExpr() = s.getKeySizeArg())
}

override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
}

/** Returns the minimum recommended key size for elliptic curve (EC) algorithms. */
private int getMinKeySize() { result = 256 }

/** Returns the key size from an EC algorithm's curve name string */
bindingset[algorithm]
private int getKeySize(string algorithm) {
algorithm.matches("sec%") and // specification such as "secp256r1"
result = algorithm.regexpCapture("sec[p|t](\\d+)[a-zA-Z].*", 1).toInt()
or
algorithm.matches("X9.62%") and // specification such as "X9.62 prime192v2"
result = algorithm.regexpCapture("X9\\.62 .*[a-zA-Z](\\d+)[a-zA-Z].*", 1).toInt()
or
algorithm.matches(["prime%", "c2tnb%"]) and // specification such as "prime192v2"
result = algorithm.regexpCapture(".*[a-zA-Z](\\d+)[a-zA-Z].*", 1).toInt()
}

/** An instance of an elliptic curve (EC) algorithm specification. */
private class Spec extends ClassInstanceExpr {
Spec() { this.getConstructedType() instanceof EcGenParameterSpec }

/** Gets the `keysize` argument of this instance. */
Argument getKeySizeArg() { result = this.getArgument(0) }
}
}

/**
* A call to the `initialize` method declared in `java.security.KeyPairGenerator`
* or to the `init` method declared in `java.security.AlgorithmParameterGenerator`.
*/
private class KeyPairGenInit extends MethodAccess {
KeyPairGenInit() {
this.getMethod() instanceof KeyPairGeneratorInitMethod or
this.getMethod() instanceof AlgoParamGeneratorInitMethod
}

/** Gets the `keysize` argument of this call. */
Argument getKeySizeArg() { result = this.getArgument(0) }
}

/**
* An instance of a `java.security.KeyPairGenerator`
* or of a `java.security.AlgorithmParameterGenerator`.
*/
private class KeyPairGen extends GeneratorAlgoSpec {
KeyPairGen() {
this instanceof JavaSecurityKeyPairGenerator or
this instanceof JavaSecurityAlgoParamGenerator
}

override Expr getAlgoSpec() {
result =
[
this.(JavaSecurityKeyPairGenerator).getAlgoSpec(),
this.(JavaSecurityAlgoParamGenerator).getAlgoSpec()
]
}
}
}

/** Provides models for symmetric cryptography. */
private module Symmetric {
/** A source for an insufficient key size used in AES algorithms. */
private class Source extends InsufficientKeySizeSource {
Source() { this.asExpr().(IntegerLiteral).getIntValue() < getMinKeySize() }

override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
}

/** A sink for an insufficient key size used in AES algorithms. */
private class Sink extends InsufficientKeySizeSink {
Sink() {
exists(KeyGenInit kgInit, KeyGen kg |
kg.getAlgoName() = "AES" and
DataFlow::localExprFlow(kg, kgInit.getQualifier()) and
this.asExpr() = kgInit.getKeySizeArg()
)
}

override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
}

/** Returns the minimum recommended key size for AES algorithms. */
private int getMinKeySize() { result = 128 }

/** A call to the `init` method declared in `javax.crypto.KeyGenerator`. */
private class KeyGenInit extends MethodAccess {
KeyGenInit() { this.getMethod() instanceof KeyGeneratorInitMethod }

/** Gets the `keysize` argument of this call. */
Argument getKeySizeArg() { result = this.getArgument(0) }
}

/** An instance of a `javax.crypto.KeyGenerator`. */
private class KeyGen extends GeneratorAlgoSpec instanceof JavaxCryptoKeyGenerator {
override Expr getAlgoSpec() { result = JavaxCryptoKeyGenerator.super.getAlgoSpec() }
}
}

/** An instance of a generator that specifies an encryption algorithm. */
abstract private class GeneratorAlgoSpec extends CryptoAlgoSpec {
/** Returns an uppercase string representing the algorithm name specified by this generator object. */
string getAlgoName() {
result = this.getAlgoSpec().(CompileTimeConstantExpr).getStringValue().toUpperCase()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/** Provides data flow configurations to be used in queries related to insufficient key sizes. */

import semmle.code.java.dataflow.DataFlow
import semmle.code.java.security.InsufficientKeySize

/** A data flow configuration for tracking key sizes used in cryptographic algorithms. */
class KeySizeConfiguration extends DataFlow::Configuration {
KeySizeConfiguration() { this = "KeySizeConfiguration" }

override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
source.(InsufficientKeySizeSource).hasState(state)
}

override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
sink.(InsufficientKeySizeSink).hasState(state)
}
}
55 changes: 55 additions & 0 deletions java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.qhelp
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>

<overview>
<p>Modern encryption relies on the computational infeasibility of breaking a cipher and decoding its
message without the key. As computational power increases, the ability to break ciphers grows, and key
sizes need to become larger as a result. Cryptographic algorithms that use too small of a key size are
vulnerable to brute force attacks, which can reveal sensitive data.</p>
</overview>

<recommendation>
<p>Use a key of the recommended size or larger. The key size should be at least 128 bits for AES encryption,
256 bits for elliptic-curve cryptography (ECC), and 2048 bits for RSA, DSA, or DH encryption.</p>
</recommendation>

<example>

<p>
The following code uses cryptographic algorithms with insufficient key sizes.
</p>

<sample src="InsufficientKeySizeBad.java" />

<p>
To fix the code, change the key sizes to be the recommended size or
larger for each algorithm.
</p>

</example>

<references>
<li>
Wikipedia:
<a href="http://en.wikipedia.org/wiki/Key_size">Key size</a>.
</li>
<li>
Wikipedia: <a href="https://en.wikipedia.org/wiki/Strong_cryptography">Strong cryptography</a>.
</li>
<li>
OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Cryptographic_Storage_Cheat_Sheet.html#algorithms">
Cryptographic Storage Cheat Sheet</a>.
</li>
<li>
OWASP: <a href="https://owasp.org/www-project-web-security-testing-guide/stable/4-Web_Application_Security_Testing/09-Testing_for_Weak_Cryptography/04-Testing_for_Weak_Encryption">
Testing for Weak Encryption</a>.
</li>
<li>
NIST:
<a href="https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar2.pdf">
Transitioning the Use of Cryptographic Algorithms and Key Lengths</a>.
</li>
</references>
</qhelp>
22 changes: 22 additions & 0 deletions java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* @name Use of a cryptographic algorithm with insufficient key size
* @description Using cryptographic algorithms with too small a key size can
* allow an attacker to compromise security.
* @kind path-problem
* @problem.severity warning
* @security-severity 7.5
* @precision high
* @id java/insufficient-key-size
* @tags security
* external/cwe/cwe-326
*/

import java
import semmle.code.java.security.InsufficientKeySizeQuery
import DataFlow::PathGraph

from DataFlow::PathNode source, DataFlow::PathNode sink, KeySizeConfiguration cfg
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink,
"This $@ is less than the recommended key size of " + source.getState() + " bits.",
source.getNode(), "key size"
15 changes: 15 additions & 0 deletions java/ql/src/Security/CWE/CWE-326/InsufficientKeySizeBad.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
KeyPairGenerator keyPairGen1 = KeyPairGenerator.getInstance("RSA");
keyPairGen1.initialize(1024); // BAD: Key size is less than 2048

KeyPairGenerator keyPairGen2 = KeyPairGenerator.getInstance("DSA");
keyPairGen2.initialize(1024); // BAD: Key size is less than 2048

KeyPairGenerator keyPairGen3 = KeyPairGenerator.getInstance("DH");
keyPairGen3.initialize(1024); // BAD: Key size is less than 2048

KeyPairGenerator keyPairGen4 = KeyPairGenerator.getInstance("EC");
ECGenParameterSpec ecSpec = new ECGenParameterSpec("secp112r1"); // BAD: Key size is less than 256
keyPairGen4.initialize(ecSpec);

KeyGenerator keyGen = KeyGenerator.getInstance("AES");
keyGen.init(64); // BAD: Key size is less than 128
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: newQuery
---
* The query `java/insufficient-key-size` has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/4926).
Loading