This rule is flagging correct and necessary usage of a non-random IV. This is an example made from the one in the MSFT docs.
using System;
using System.IO;
using System.Security.Cryptography;
namespace RijndaelManaged_Example
{
class RijndaelExample
{
public static void Main()
{
try
{
string original = "Here is some data to encrypt!";
byte[] encrypted;
byte[] key;
byte[] iv;
// Create a new instance of the Rijndael
// class. This generates a new key and initialization
// vector (IV).
using (Rijndael encryptingRijndael = Rijndael.Create())
{
key = encryptingRijndael.Key;
iv = encryptingRijndael.IV;
encrypted = EncryptStringToBytes(original, key, iv);
}
using (Rijndael decryptingRijndael = Rijndael.Create())
{
string sharedKeyNotIv = DecryptStringFromBytes(encrypted, key, decryptingRijndael.IV);
string sharedKeyAndIv = DecryptStringFromBytes(encrypted, key, iv);
Console.WriteLine("Original: \t\t\t\t\t{0}", original);
Console.WriteLine("Round Trip (sharedKeyNotIv):\t{0}", sharedKeyNotIv);
Console.WriteLine("Round Trip (sharedKeyAndIv):\t{0}", sharedKeyAndIv);
}
}
catch (Exception e)
{
Console.WriteLine("Error: {0}", e.Message);
}
}
static byte[] EncryptStringToBytes(string plainText, byte[] Key, byte[] IV)
{
// Check arguments.
if (plainText == null || plainText.Length <= 0)
throw new ArgumentNullException("plainText");
if (Key == null || Key.Length <= 0)
throw new ArgumentNullException("Key");
if (IV == null || IV.Length <= 0)
throw new ArgumentNullException("IV");
byte[] encrypted;
// Create an Rijndael object
// with the specified key and IV.
using (Rijndael rijAlg = Rijndael.Create())
{
rijAlg.Key = Key;
rijAlg.IV = IV;
// Create an encryptor to perform the stream transform.
ICryptoTransform encryptor = rijAlg.CreateEncryptor(rijAlg.Key, rijAlg.IV);
// Create the streams used for encryption.
using (MemoryStream msEncrypt = new MemoryStream())
{
using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
{
using (StreamWriter swEncrypt = new StreamWriter(csEncrypt))
{
//Write all data to the stream.
swEncrypt.Write(plainText);
}
encrypted = msEncrypt.ToArray();
}
}
}
return encrypted;
}
static string DecryptStringFromBytes(byte[] cipherText, byte[] Key, byte[] IV)
{
// Check arguments.
if (cipherText == null || cipherText.Length <= 0)
throw new ArgumentNullException("cipherText");
if (Key == null || Key.Length <= 0)
throw new ArgumentNullException("Key");
if (IV == null || IV.Length <= 0)
throw new ArgumentNullException("IV");
// Declare the string used to hold
// the decrypted text.
string plaintext = null;
// Create an Rijndael object
// with the specified key and IV.
using (Rijndael rijAlg = Rijndael.Create())
{
rijAlg.Key = Key;
rijAlg.IV = IV;
ICryptoTransform decryptor = rijAlg.CreateDecryptor(rijAlg.Key, rijAlg.IV);
// Create the streams used for decryption.
using (MemoryStream msDecrypt = new MemoryStream(cipherText))
{
using (CryptoStream csDecrypt = new CryptoStream(msDecrypt, decryptor, CryptoStreamMode.Read))
{
using (StreamReader srDecrypt = new StreamReader(csDecrypt))
{
// Read the decrypted bytes from the decrypting stream
// and place them in a string.
plaintext = srDecrypt.ReadToEnd();
}
}
}
}
return plaintext;
}
}
}
output
Original: Here is some data to encrypt!
Round Trip (sharedKeyNotIv): ļæ½ļæ½ļæ½_S|ļæ½
[ļæ½bnļæ½ļæ½a to encrypt!
Round Trip (sharedKeyAndIv): Here is some data to encrypt!
The IV needs to be reused in order to decrypt the whole value correctly.
Thanks, that helps indeed. SonarCloud automatically uses the latest version (in comparison to SonarQube that could be any version). Unfortunately no IV issue is raised for me if I scan your example code. Is your project public by any chance, so that I could have a look there if something is different?
The example was to point out that the IV cannot be dynamic and different each time. Its not the code thats getting flagged. That code getting flagged looks like thisā¦
byte[] keyBytes = new byte[16];
byte[] ivBytes = new byte[16];
//assign the bytes
rijndaelCipher.Key = keyBytes;
rijndaelCipher.IV = ivBytes;
ICryptoTransform transform = rijndaelCipher.CreateEncryptor();
//or it can be one of the other overloads
byte[] keyBytes = new byte[16];
byte[] ivBytes = new byte[16];
//assign the bytes
ICryptoTransform transform = rijndaelCipher.CreateEncryptor(keyBytes, ivBytes);
Of course if you want to decrypt something that is using CBC you will have to use the same IV as for the encryption. The rule does not forbid that. As stated in its description, the rule is intended for the encryption process:
In encryption, when Cipher Block Chaining (CBC) is used, the Initialization Vector (IV) must be random and unpredictable. Otherwise, the encrypted value is vulnerable to crypto-analysis attacks such as the āChosen-Plaintext Attackā.
An IV value should be associated to one, and only one encryption cycle, because the IVās purpose is to ensure that the same plaintext encrypted twice will yield two different ciphertexts.
It is possible that it raises a false-positive for you but if I am not able to reproduce it there is nothing I can do about it. In the code you posted I do not see where a value is assigned to ivBytes and this is the relevant part.
It does sound like a true-positive then. But it should not happen that you have to review it again after every commit. I will try to find someone that can take over from here since this is out of my expertise.
It does sound like a true-positive then. But it should not happen that you have to review it again after every commit. I will try to find someone that can take over from here since this is out of my expertise.
Its not critical or blocking anything yet so thats fine.
Iād still contend that its a false positive as there is a pair of Encrypt and Decrypt (with matching overloads) methods in the class depicted and both do the same with the Key and IV.
Its at least worthy of a better description and example with a note about one-way vs two-way encryption.
Sorry for the delay, I spent some more time on this rule now to have a look how it is implemented.
If both the encryption and decryption use the non-dynamically generated IV the rule raises as intended (though, only for the encryption part). If the encryption uses a dynamically generated IV and only the decryption uses the non-dynamically generated IV then it is indeed a false-positive. I am happy to investigate it in more detail but as I wrote before - the relevant part for that in your code sample is missing. So I can only say: yes, it is possible that the rule produces a false-positive in general, i.e. when the non-dynamically generated IV is used for decryption only.
The description is very explicit about the fact that it is about the encryption only though. Could you please elaborate what part you think could be improved? I also do not understand the comment about one-way vs two-way encryption.
We had to drop and recreate the Sonar project to resolve the āi had to enter the issue resolution multiple timesā.
Using a different IV every time to encrypt means we would have to store the IVās used by every encryption request somewhere so it could be supplied again for the decryption. I get the reason for doing all of that, but Iāve not actually seen anyone ever put up a database of encryption requests and related IVās, and Ive worked in some heavily regulated industries (HR/Payroll/Pharma Manuf/etc), The more common approach is to have the IV passed in as a parameter to the encryption function, so one can be supplied and controlled by the scope of the calling application.
In fact this is what is happening in the function being flagged. The IV is being determined from a function argument. I would like to share the entire function code here but there is no way to mark it so that only you or I can see it, but its fairly similar to the MSFT example where the plaintext, key, and IV are parameters to the function that does the encryption.
1 way vs. 2 way was me missing that the reported issue was on the Encrypt and not the Decrypt.
Storing the IV in a database is not a good approach, I completely agree. Usually the IV is just combined with the cipher text, e.g.
{
"encrypted_data": "...",
"iv": "..."
}
Having the IV passed in as a parameter to the encryption function is completely fine, it depends where the IV comes from. The rule should only raise if it comes from a constant.
Let me try to send you a private message, that should allow us to exchange the code privately. Seeing your code would really help me, thanks!
This is probably tangential but doesnt combining it make it predictable?
I still feel this is being reported incorrectly, but the function is due to be replaced and I dont want to spend more time on it. I was more annoyed with sonarcloud asking me to resolve the hotspot on each and every PR, but Iām not actively working in that repo at the moment (until the function is replaced) so the repeated griefing has also abated. Thanks again for your insight and assistance.
Predictable in this case means that for example you are using a fixed seed for your random bytes function, so the chance that multiple runs of the encryption use the same IV is high. It is perfectly fine though if the IV is public.
I am glad to hear that it will not be a problem any more in the future. Have a nice day!
