-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Java: Static initialization vector #6357
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
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
218731c
Added a query for static initialization vectors in encryption
artem-smotrakov cfe74b5
Use inline-expectation tests for StaticInitializationVector.ql
artem-smotrakov df0f9ee
Fixed a few typos
artem-smotrakov 4e69081
Support multi-dimensional arrays
artem-smotrakov 1199240
Be precise when checking for Cipher.ENCRYPT_MODE
artem-smotrakov d218813
Updated qldoc for ArrayUpdate
artem-smotrakov e2dc975
Covered copyOfRange() and clone() in ArrayUpdate
artem-smotrakov fbac589
Fixed a typo in qldoc
artem-smotrakov c80a1da
Don't consider copyOf() and clone() in ArrayUpdate
artem-smotrakov 86b7b2b
Updated qldoc for ArrayUpdate
artem-smotrakov f97c8bb
Removed sanitizer in StaticInitializationVectorConfig
artem-smotrakov f41828e
Better qldoc in StaticInitializationVectorQuery.qll
artem-smotrakov 23e2322
Simplify ArrayUpdate
artem-smotrakov 1dd4bf0
Simplify StaticInitializationVectorSource
artem-smotrakov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
4 changes: 4 additions & 0 deletions
4
java/ql/src/experimental/Security/CWE/CWE-1204/BadStaticInitializationVector.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
byte[] iv = new byte[16]; // all zeroes | ||
GCMParameterSpec params = new GCMParameterSpec(128, iv); | ||
Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); | ||
cipher.init(Cipher.ENCRYPT_MODE, key, params); |
6 changes: 6 additions & 0 deletions
6
java/ql/src/experimental/Security/CWE/CWE-1204/GoodRandomInitializationVector.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
byte[] iv = new byte[16]; | ||
SecureRandom random = SecureRandom.getInstanceStrong(); | ||
random.nextBytes(iv); | ||
GCMParameterSpec params = new GCMParameterSpec(128, iv); | ||
Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); | ||
cipher.init(Cipher.ENCRYPT_MODE, key, params); |
46 changes: 46 additions & 0 deletions
46
java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd"> | ||
<qhelp> | ||
|
||
<overview> | ||
<p> | ||
A cipher needs an initialization vector (IV) when it is used in certain modes | ||
such as CBC or GCM. Under the same secret key, IVs should be unique and ideally unpredictable. | ||
Given a secret key, if the same IV is used for encryption, the same plaintexts result in the same ciphertexts. | ||
This lets an attacker learn if the same data pieces are transferred or stored, | ||
or this can help the attacker run a dictionary attack. | ||
</p> | ||
</overview> | ||
|
||
<recommendation> | ||
<p> | ||
Use a random IV generated by <code>SecureRandom</code>. | ||
</p> | ||
</recommendation> | ||
|
||
<example> | ||
<p> | ||
The following example initializes a cipher with a static IV which is unsafe: | ||
</p> | ||
<sample src="BadStaticInitializationVector.java" /> | ||
|
||
<p> | ||
The next example initializes a cipher with a random IV: | ||
</p> | ||
<sample src="GoodRandomInitializationVector.java" /> | ||
</example> | ||
|
||
<references> | ||
<li> | ||
Wikipedia: | ||
<a href="https://en.wikipedia.org/wiki/Initialization_vector">Initialization vector</a>. | ||
</li> | ||
<li> | ||
National Institute of Standards and Technology: | ||
<a href="https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38a.pdf">Recommendation for Block Cipher Modes of Operation</a>. | ||
</li> | ||
<li> | ||
National Institute of Standards and Technology: | ||
<a href="https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.140-2.pdf">FIPS 140-2: Security Requirements for Cryptographic Modules</a>. | ||
</li> | ||
</references> | ||
</qhelp> |
26 changes: 26 additions & 0 deletions
26
java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/** | ||
* @name Using a static initialization vector for encryption | ||
* @description A cipher needs an initialization vector (IV) in some cases, | ||
* for example, when CBC or GCM modes are used. IVs are used to randomize the encryption, | ||
* therefore they should be unique and ideally unpredictable. | ||
* Otherwise, the same plaintexts result in same ciphertexts under a given secret key. | ||
* If a static IV is used for encryption, this lets an attacker learn | ||
* if the same data pieces are transferred or stored, | ||
* or this can help the attacker run a dictionary attack. | ||
* @kind path-problem | ||
* @problem.severity warning | ||
* @precision high | ||
* @id java/static-initialization-vector | ||
* @tags security | ||
* external/cwe/cwe-329 | ||
* external/cwe/cwe-1204 | ||
*/ | ||
|
||
import java | ||
import experimental.semmle.code.java.security.StaticInitializationVectorQuery | ||
import DataFlow::PathGraph | ||
|
||
from DataFlow::PathNode source, DataFlow::PathNode sink, StaticInitializationVectorConfig conf | ||
where conf.hasFlowPath(source, sink) | ||
select sink.getNode(), source, sink, "A $@ should not be used for encryption.", source.getNode(), | ||
"static initialization vector" |
165 changes: 165 additions & 0 deletions
165
java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,165 @@ | ||
import java | ||
import semmle.code.java.dataflow.TaintTracking | ||
import semmle.code.java.dataflow.TaintTracking2 | ||
|
||
/** | ||
* Holds if `array` is initialized only with constants. | ||
*/ | ||
private predicate initializedWithConstants(ArrayCreationExpr array) { | ||
// creating an array without an initializer, for example `new byte[8]` | ||
not exists(array.getInit()) | ||
or | ||
// creating a multidimensional array with an initializer like `{ new byte[8], new byte[16] }` | ||
// This works around https://github.com/github/codeql/issues/6552 -- change me once there is | ||
// a better way to distinguish nested initializers that create zero-filled arrays | ||
// (e.g. `new byte[1]`) from those with an initializer list (`new byte[] { 1 }` or just `{ 1 }`) | ||
array.getInit().getAnInit().getAChildExpr() instanceof IntegerLiteral | ||
or | ||
// creating an array wit an initializer like `new byte[] { 1, 2 }` | ||
forex(Expr element | element = array.getInit().getAnInit() | | ||
element instanceof CompileTimeConstantExpr | ||
) | ||
} | ||
|
||
/** | ||
* An expression that creates a byte array that is initialized with constants. | ||
*/ | ||
private class StaticByteArrayCreation extends ArrayCreationExpr { | ||
StaticByteArrayCreation() { | ||
this.getType().(Array).getElementType().(PrimitiveType).getName() = "byte" and | ||
initializedWithConstants(this) | ||
} | ||
} | ||
|
||
/** An expression that updates `array`. */ | ||
private class ArrayUpdate extends Expr { | ||
artem-smotrakov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Expr array; | ||
|
||
ArrayUpdate() { | ||
exists(Assignment assign | | ||
assign = this and | ||
assign.getDest().(ArrayAccess).getArray() = array and | ||
not assign.getSource() instanceof CompileTimeConstantExpr | ||
) | ||
or | ||
exists(StaticMethodAccess ma | | ||
ma.getMethod().hasQualifiedName("java.lang", "System", "arraycopy") and | ||
ma = this and | ||
ma.getArgument(2) = array | ||
) | ||
or | ||
exists(MethodAccess ma, Method m | | ||
m = ma.getMethod() and | ||
ma = this and | ||
ma.getArgument(0) = array | ||
| | ||
m.hasQualifiedName("java.io", "InputStream", "read") or | ||
m.hasQualifiedName("java.nio", "ByteBuffer", "get") or | ||
m.hasQualifiedName("java.security", "SecureRandom", "nextBytes") or | ||
m.hasQualifiedName("java.util", "Random", "nextBytes") | ||
) | ||
} | ||
|
||
/** Returns the updated array. */ | ||
Expr getArray() { result = array } | ||
} | ||
|
||
/** | ||
* A config that tracks dataflow from creating an array to an operation that updates it. | ||
*/ | ||
private class ArrayUpdateConfig extends TaintTracking2::Configuration { | ||
ArrayUpdateConfig() { this = "ArrayUpdateConfig" } | ||
|
||
override predicate isSource(DataFlow::Node source) { | ||
source.asExpr() instanceof StaticByteArrayCreation | ||
} | ||
|
||
override predicate isSink(DataFlow::Node sink) { | ||
exists(ArrayUpdate update | update.getArray() = sink.asExpr()) | ||
} | ||
} | ||
|
||
/** | ||
* A source that defines an array that doesn't get updated. | ||
*/ | ||
private class StaticInitializationVectorSource extends DataFlow::Node { | ||
StaticInitializationVectorSource() { | ||
exists(StaticByteArrayCreation array | array = this.asExpr() | | ||
not exists(ArrayUpdateConfig config | config.hasFlow(DataFlow2::exprNode(array), _)) | ||
) | ||
} | ||
} | ||
|
||
/** | ||
* A config that tracks initialization of a cipher for encryption. | ||
*/ | ||
private class EncryptionModeConfig extends TaintTracking2::Configuration { | ||
EncryptionModeConfig() { this = "EncryptionModeConfig" } | ||
|
||
override predicate isSource(DataFlow::Node source) { | ||
source | ||
.asExpr() | ||
.(FieldRead) | ||
.getField() | ||
.hasQualifiedName("javax.crypto", "Cipher", "ENCRYPT_MODE") | ||
} | ||
|
||
override predicate isSink(DataFlow::Node sink) { | ||
exists(MethodAccess ma, Method m | m = ma.getMethod() | | ||
m.hasQualifiedName("javax.crypto", "Cipher", "init") and | ||
ma.getArgument(0) = sink.asExpr() | ||
) | ||
} | ||
} | ||
|
||
/** | ||
* A sink that initializes a cipher for encryption with unsafe parameters. | ||
*/ | ||
private class EncryptionInitializationSink extends DataFlow::Node { | ||
EncryptionInitializationSink() { | ||
exists(MethodAccess ma, Method m, EncryptionModeConfig config | m = ma.getMethod() | | ||
m.hasQualifiedName("javax.crypto", "Cipher", "init") and | ||
m.getParameterType(2) | ||
.(RefType) | ||
.hasQualifiedName("java.security.spec", "AlgorithmParameterSpec") and | ||
ma.getArgument(2) = this.asExpr() and | ||
config.hasFlowToExpr(ma.getArgument(0)) | ||
) | ||
} | ||
} | ||
|
||
/** | ||
* Holds if `fromNode` to `toNode` is a dataflow step | ||
* that creates cipher's parameters with initialization vector. | ||
*/ | ||
private predicate createInitializationVectorSpecStep(DataFlow::Node fromNode, DataFlow::Node toNode) { | ||
exists(ConstructorCall cc, RefType type | | ||
cc = toNode.asExpr() and type = cc.getConstructedType() | ||
| | ||
type.hasQualifiedName("javax.crypto.spec", "IvParameterSpec") and | ||
cc.getArgument(0) = fromNode.asExpr() | ||
or | ||
type.hasQualifiedName("javax.crypto.spec", ["GCMParameterSpec", "RC2ParameterSpec"]) and | ||
cc.getArgument(1) = fromNode.asExpr() | ||
or | ||
type.hasQualifiedName("javax.crypto.spec", "RC5ParameterSpec") and | ||
cc.getArgument(3) = fromNode.asExpr() | ||
) | ||
} | ||
|
||
/** | ||
* A config that tracks dataflow to initializing a cipher with a static initialization vector. | ||
*/ | ||
class StaticInitializationVectorConfig extends TaintTracking::Configuration { | ||
StaticInitializationVectorConfig() { this = "StaticInitializationVectorConfig" } | ||
|
||
override predicate isSource(DataFlow::Node source) { | ||
source instanceof StaticInitializationVectorSource | ||
} | ||
|
||
override predicate isSink(DataFlow::Node sink) { sink instanceof EncryptionInitializationSink } | ||
|
||
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) { | ||
createInitializationVectorSpecStep(fromNode, toNode) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to use
getComponentType()
instead ofgetElementType()
, because the above would also hold forbyte[][][]
while withgetComponentType()
it would only hold forbyte[]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This expression doesn't check the dimension. Both methods work fine here, I don't really see why
getComponentType()
might be better here. The docs forgetElementType()
says:That's exactly what is necessary here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But will a multi-dimensional
byte
array (e.g.byte[][][]
), or one of its sub arrays, ever be used as initialization vector (and would the other parts of this query even handle that correctly)?Because
StaticByteArrayCreation
is currently matching that, e.g.new byte[5][]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Marcono is right, you really do only want
byte[]
hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't consider multi-dimensional arrays. I think it may make sense to support them. For example, an application can use a set of static IVs like this:
I added test cases
encryptWithOneOfStaticIvs01
,encryptWithOneOfStaticIvs02
andencryptWithOneOfStaticZeroIvs
that use multi-dimensional arrays, and updated theinitializedWithConstants()
predicate.array.getInit().getAnInit().getAChildExpr() instanceof IntegerLiteral
covers the following case:This expression looks weird to me, but I could not find a better way to cover the case above. While debugging this case, I saw the child elements of the
ArrayInit
s are instances ofIntegerLiteral
andTypeAccess
. Please let me know if there is a better way.StaticByteArrayCreation
still usesgetElementType()
because it wants to check only the type but not dimensions. If we decide to cover only one-dimension arrays, then we can usegetElementType()
. I would prefer to support multi-dimensional arrays though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The VSCode extension AST viewer shows that for me as well, and manually quering
ArrayCreationExpr
andArrayInit
seems to confirm that.To me that looks like a bug in the extractor, that
ArrayCreationExpr
inside anArrayInit
is modelled as (malformed)ArrayInit
. Might be worth creating an issue for this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also expected to see
ArrayCreationExpr
inside. @smowton Does it look like a bug? Or, are we missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #6552 (comment)