mirror of
https://github.com/github/codeql.git
synced 2025-12-24 04:36:35 +01:00
Take into account remote user input in java/non-constant-time-crypto-comparison
This commit is contained in:
Artem Smotrakov
committed by
Fosstars
Fosstars
parent
8e6d227dc0
commit
a4f3a5a88e
@@ -13,12 +13,59 @@
|
||||
|
||||
import java
|
||||
import semmle.code.java.dataflow.TaintTracking
|
||||
import semmle.code.java.dataflow.TaintTracking2
|
||||
import semmle.code.java.dataflow.FlowSources
|
||||
import DataFlow::PathGraph
|
||||
|
||||
private class UserInputInCryptoOperationConfig extends TaintTracking2::Configuration {
|
||||
UserInputInCryptoOperationConfig() { this = "UserInputInCryptoOperationConfig" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) {
|
||||
exists(MethodAccess ma, Method m | m = ma.getMethod() |
|
||||
(
|
||||
(
|
||||
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "doFinal") or
|
||||
m.hasQualifiedName("java.security", "Signature", "sign")
|
||||
) and
|
||||
ma.getQualifier() = sink.asExpr()
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
override predicate isAdditionalTaintStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) {
|
||||
exists(MethodAccess ma, Method m | m = ma.getMethod() |
|
||||
(
|
||||
(
|
||||
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], ["doFinal", "update"]) or
|
||||
m.hasQualifiedName("java.security", "Signature", ["sign", "update"])
|
||||
) and
|
||||
ma.getAnArgument() = fromNode.asExpr() and
|
||||
ma.getQualifier() = toNode.asExpr()
|
||||
)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
abstract private class CryptoOperationSource extends DataFlow::Node {
|
||||
Expr cryptoOperation;
|
||||
|
||||
predicate includesUserInput() {
|
||||
exists(
|
||||
DataFlow2::PathNode source, DataFlow2::PathNode sink, UserInputInCryptoOperationConfig config
|
||||
|
|
||||
config.hasFlowPath(source, sink)
|
||||
|
|
||||
sink.getNode().asExpr() = cryptoOperation
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A source that produces a MAC.
|
||||
*/
|
||||
private class MacSource extends DataFlow::Node {
|
||||
private class MacSource extends CryptoOperationSource {
|
||||
MacSource() {
|
||||
exists(MethodAccess ma, Method m | m = ma.getMethod() |
|
||||
m.hasQualifiedName("javax.crypto", "Mac", "doFinal") and
|
||||
@@ -26,7 +73,8 @@ private class MacSource extends DataFlow::Node {
|
||||
m.getReturnType() instanceof Array and ma = this.asExpr()
|
||||
or
|
||||
m.getParameterType(0) instanceof Array and ma.getArgument(0) = this.asExpr()
|
||||
)
|
||||
) and
|
||||
cryptoOperation = ma.getQualifier()
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -34,7 +82,7 @@ private class MacSource extends DataFlow::Node {
|
||||
/**
|
||||
* A source that produces a signature.
|
||||
*/
|
||||
private class SignatureSource extends DataFlow::Node {
|
||||
private class SignatureSource extends CryptoOperationSource {
|
||||
SignatureSource() {
|
||||
exists(MethodAccess ma, Method m | m = ma.getMethod() |
|
||||
m.hasQualifiedName("java.security", "Signature", "sign") and
|
||||
@@ -42,7 +90,8 @@ private class SignatureSource extends DataFlow::Node {
|
||||
m.getReturnType() instanceof Array and ma = this.asExpr()
|
||||
or
|
||||
m.getParameterType(0) instanceof Array and ma.getArgument(0) = this.asExpr()
|
||||
)
|
||||
) and
|
||||
cryptoOperation = ma.getQualifier()
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -50,7 +99,7 @@ private class SignatureSource extends DataFlow::Node {
|
||||
/**
|
||||
* A source that produces a ciphertext.
|
||||
*/
|
||||
private class CiphertextSource extends DataFlow::Node {
|
||||
private class CiphertextSource extends CryptoOperationSource {
|
||||
CiphertextSource() {
|
||||
exists(MethodAccess ma, Method m | m = ma.getMethod() |
|
||||
m.hasQualifiedName("javax.crypto", "Cipher", "doFinal") and
|
||||
@@ -61,7 +110,28 @@ private class CiphertextSource extends DataFlow::Node {
|
||||
or
|
||||
m.getParameterType(1).(RefType).hasQualifiedName("java.nio", "ByteBuffer") and
|
||||
ma.getArgument(1) = this.asExpr()
|
||||
)
|
||||
) and
|
||||
cryptoOperation = ma.getQualifier()
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
private class UserInputComparisonConfig extends TaintTracking2::Configuration {
|
||||
UserInputComparisonConfig() { this = "UserInputComparisonConfig" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) {
|
||||
exists(MethodAccess ma, Method m | m = ma.getMethod() |
|
||||
(
|
||||
m.hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"]) or
|
||||
m.hasQualifiedName("java.nio", "ByteBuffer", ["equals", "compareTo"]) or
|
||||
m.hasQualifiedName("java.util", "Arrays", ["equals", "deepEquals"]) or
|
||||
m.hasQualifiedName("java.util", "Objects", "deepEquals") or
|
||||
m.hasQualifiedName("org.apache.commons.lang3", "StringUtils",
|
||||
["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"])
|
||||
) and
|
||||
[ma.getAnArgument(), ma.getQualifier()] = sink.asExpr()
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -70,41 +140,46 @@ private class CiphertextSource extends DataFlow::Node {
|
||||
* A sink that compares input using a non-constant time algorithm.
|
||||
*/
|
||||
private class KnownNonConstantTimeComparisonSink extends DataFlow::Node {
|
||||
Expr anotherParameter;
|
||||
|
||||
KnownNonConstantTimeComparisonSink() {
|
||||
exists(MethodAccess ma, Method m | m = ma.getMethod() |
|
||||
m.hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"]) and
|
||||
this.asExpr() = [ma.getQualifier(), ma.getAnArgument()]
|
||||
or
|
||||
m.hasQualifiedName("java.nio", "ByteBuffer", ["equals", "compareTo"]) and
|
||||
this.asExpr() = [ma.getQualifier(), ma.getAnArgument()]
|
||||
or
|
||||
m.hasQualifiedName("java.util", "Arrays", ["equals", "deepEquals"]) and
|
||||
ma.getAnArgument() = this.asExpr()
|
||||
or
|
||||
m.hasQualifiedName("java.util", "Objects", "deepEquals") and
|
||||
ma.getAnArgument() = this.asExpr()
|
||||
or
|
||||
m.hasQualifiedName("org.apache.commons.lang3", "StringUtils",
|
||||
["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"]) and
|
||||
ma.getAnArgument() = this.asExpr()
|
||||
(
|
||||
m.hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"]) or
|
||||
m.hasQualifiedName("java.nio", "ByteBuffer", ["equals", "compareTo"])
|
||||
) and
|
||||
(
|
||||
this.asExpr() = ma.getQualifier() and
|
||||
anotherParameter = ma.getAnArgument() and
|
||||
not ma.getAnArgument().isCompileTimeConstant()
|
||||
or
|
||||
this.asExpr() = ma.getAnArgument() and
|
||||
anotherParameter = ma.getQualifier() and
|
||||
not ma.getQualifier().isCompileTimeConstant()
|
||||
)
|
||||
)
|
||||
or
|
||||
exists(StaticMethodAccess ma, Method m | m = ma.getMethod() |
|
||||
(
|
||||
m.hasQualifiedName("java.util", "Arrays", ["equals", "deepEquals"]) or
|
||||
m.hasQualifiedName("java.util", "Objects", "deepEquals") or
|
||||
m.hasQualifiedName("org.apache.commons.lang3", "StringUtils",
|
||||
["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"])
|
||||
) and
|
||||
ma.getAnArgument() = this.asExpr() and
|
||||
(
|
||||
this.asExpr() = ma.getArgument(0) and anotherParameter = ma.getArgument(1)
|
||||
or
|
||||
this.asExpr() = ma.getArgument(1) and anotherParameter = ma.getArgument(0)
|
||||
)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `fromNode` to `toNode` is a dataflow step
|
||||
* that converts a `ByteBuffer` to a byte array and vice versa.
|
||||
*/
|
||||
private predicate convertByteBufferStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
|
||||
exists(MethodAccess ma, Method m | m = ma.getMethod() |
|
||||
m.hasQualifiedName("java.nio", "ByteBuffer", "array") and
|
||||
ma.getQualifier() = fromNode.asExpr() and
|
||||
ma = toNode.asExpr()
|
||||
or
|
||||
m.hasQualifiedName("java.nio", "ByteBuffer", "wrap") and
|
||||
ma.getAnArgument() = fromNode.asExpr() and
|
||||
ma = toNode.asExpr()
|
||||
)
|
||||
predicate includesUserInput() {
|
||||
exists(UserInputComparisonConfig config |
|
||||
config.hasFlowTo(DataFlow2::exprNode(anotherParameter))
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -114,24 +189,19 @@ private predicate convertByteBufferStep(DataFlow::Node fromNode, DataFlow::Node
|
||||
private class NonConstantTimeCryptoComparisonConfig extends TaintTracking::Configuration {
|
||||
NonConstantTimeCryptoComparisonConfig() { this = "NonConstantTimeCryptoComparisonConfig" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source) {
|
||||
source instanceof MacSource
|
||||
or
|
||||
source instanceof SignatureSource
|
||||
or
|
||||
source instanceof CiphertextSource
|
||||
}
|
||||
override predicate isSource(DataFlow::Node source) { source instanceof CryptoOperationSource }
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) {
|
||||
sink instanceof KnownNonConstantTimeComparisonSink
|
||||
}
|
||||
|
||||
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
|
||||
convertByteBufferStep(fromNode, toNode)
|
||||
}
|
||||
}
|
||||
|
||||
from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf
|
||||
where conf.hasFlowPath(source, sink)
|
||||
where
|
||||
conf.hasFlowPath(source, sink) and
|
||||
(
|
||||
source.getNode().(CryptoOperationSource).includesUserInput() or
|
||||
sink.getNode().(KnownNonConstantTimeComparisonSink).includesUserInput()
|
||||
)
|
||||
select sink.getNode(), source, sink,
|
||||
"Using a non-constant time algorithm for comparing results of a cryptographic operation."
|
||||
|
||||
Reference in New Issue
Block a user