New Rule S6432: Counter Mode initialization vectors should not be reused

[pedro-oliveira-sonarsource]

What The rule detects "weak" initialization vectors when used with GCM and CCM block cipher mode of operation. The detection logic is similar to what is implemented for rule S3329. It supports System.Security.Cryptography.

How The rule raises on System.Security.Cryptography.AesGcm#Encrypt or System.Security.Cryptography.AesCcm#Encrypt when it matches the following criteria:

• first argument, nonce is created from a string literal and converted to a byte array through the System.Text Encoding.GetBytes function.

var key = Encoding.UTF8.GetBytes("0123456789123456");
var plaintext = Encoding.UTF8.GetBytes("plaintext");
var ciphertext = new byte[plaintext.Length];
var tag = new byte[AesGcm.TagByteSizes.MaxSize];

var nonce = Encoding.UTF8.GetBytes("7cVgr5cbdCZV"); // Secondary location: The initialization vector is a static value.

using var aes = new AesGcm(key);

aes.Encrypt(nonce, plaintext, ciphertext, tag); // Noncompliant

Code examples

https://github.com/SonarSource/security-expected-issues/pull/717

Message and highlighting

See https://github.com/SonarSource/rspec/pull/1060

[pavel-mikula-sonarsource]

MMF-2826

[andrei-epure-sonarsource]

This looks like a symbolic execution rule.

[andrei-epure-sonarsource]

Before taking a decision about Symbolic Execution vs usage of trackers, let's first look into what it would take to use trackers.

@martin-strecker-sonarsource suggestion:

• Look for invocations to Encrypt and investigate what expression was passed in as nonce argument:
  • local: look for assigments to it
  • field: look for initializer

[pavel-mikula-sonarsource]

Trackers are tracking a state of an individual object ("is this property set to 'true' or 'false'"), they don't track another value propagation through the code.

ConstantValueFinder has very limited value tracking capabilities, but it cannot answer questions we have.

This sounds like a SE rule. A simple one.

[andrei-epure-sonarsource]

Context: this is very similar to S2053 which follows the data flow for the same Encoding.GetBytes method.

S2053 has been implemented in PR#3829 and can be found in InitializationVectorShouldBeRandom.cs. The follow-up FP fixes might be relevant, too.

• a similar constraint to ByteArrayConstraint will need to be implemented for the new engine in SonarAnalyzer.Common/SymbolicExecution/Constraints.
• a similar ByteArrayCheck will need to be implemented in SymbolicExecution/Roslyn/Checks/.

I imagine this rule is even simpler than S2053 because it only cares about the ByteArray (whereas S2053 also checks the salt size).

[martin-strecker-sonarsource]

If we want to avoid SE and be more precise with respect to fields and properties as sources of the nonce we could do this instead:

In symbol start:

• Track creations of byte[] and the like (this includes e.g. utf-8 strings) that is purely made of constant values. Store any symbols this is assigned to (fields, properties, and locals)
• And also track invocations of Encrypt and any symbols passed as an argument to the nonce parameter.

In symbol end action:

• see if there is an overlap of symbols

This would cover fields/properties as stores for the nonce as well as locals. It would not cover any mutation that happens between creation and usage.

Furthermore: The requirement of the cipher value is to come from an external source:

The nonce associated with this message, which should be a unique value for every operation with the same key.

So any nonce that gets passed in as a parameter should be okay.

[andrei-epure-sonarsource]

Note: the MMF was closed by @pierre-loup-tristant-sonarsource but this rule is still relevant (see comment).