-
Notifications
You must be signed in to change notification settings - Fork 58
chore: Solution-wide code cleanup #309
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
base: develop
Are you sure you want to change the base?
Conversation
Test Results: Windows 2 files 2 suites 9s ⏱️ Results for commit a3f3ed6. |
Test Results: Ubuntu 2 files 2 suites 17s ⏱️ Results for commit a3f3ed6. |
Test Results: MacOS 2 files 2 suites 9s ⏱️ Results for commit a3f3ed6. |
Yubico.YubiKey/src/Yubico/YubiKey/Cryptography/AesUtilities.cs
Dismissed
Show dismissed
Hide dismissed
|
||
using var aesObject = CryptographyProviders.AesCreator(); | ||
#pragma warning disable CA5358 // Allow the usage of cipher mode 'ECB' per the standard | ||
aesObject.Mode = CipherMode.ECB; |
Check failure
Code scanning / CodeQL
Encryption using ECB High
Copilot Autofix
AI 10 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
Yubico.YubiKey/src/Yubico/YubiKey/Piv/Commands/TripleDesForManagementKey.cs
Dismissed
Show dismissed
Hide dismissed
Yubico.YubiKey/src/Yubico/YubiKey/Piv/Commands/TripleDesForManagementKey.cs
Dismissed
Show dismissed
Hide dismissed
Yubico.YubiKey/src/Yubico/YubiKey/Piv/Commands/TripleDesForManagementKey.cs
Dismissed
Show dismissed
Hide dismissed
var result4 = new byte[8]; | ||
|
||
var tDesObject = TripleDES.Create(); | ||
tDesObject.Mode = CipherMode.ECB; |
Check failure
Code scanning / CodeQL
Encryption using ECB High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the problem, we need to avoid the use of ECB mode and instead use a secure cipher mode such as CBC. Specifically, change the line tDesObject.Mode = CipherMode.ECB;
to use CBC: tDesObject.Mode = CipherMode.CBC;
. When using CBC mode, an initialization vector (IV) is required. Since the test currently passes null
for the IV in CreateEncryptor
, this must be changed to an actual IV value. The IV should be 8 bytes for TripleDES and, for deterministic/reproducible test results, can be set to a fixed value such as all zero bytes. Update all calls to CreateEncryptor
to use the IV argument instead of null
. Only lines relevant to cipher mode and IV usage are to be changed; all other logic remains the same.
-
Copy modified line R135 -
Copy modified line R137 -
Copy modified line R139 -
Copy modified line R143 -
Copy modified line R147
@@ -132,18 +132,19 @@ | ||
var result4 = new byte[8]; | ||
|
||
var tDesObject = TripleDES.Create(); | ||
tDesObject.Mode = CipherMode.ECB; | ||
tDesObject.Mode = CipherMode.CBC; | ||
tDesObject.Padding = PaddingMode.None; | ||
var iv = new byte[8]; // Fixed IV for deterministic test | ||
|
||
var encryptor1 = tDesObject.CreateEncryptor(keyData1, null); | ||
var encryptor1 = tDesObject.CreateEncryptor(keyData1, iv); | ||
var eLen = encryptor1.TransformBlock(dataToEncrypt, 0, 8, result1, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
var encryptor2 = tDesObject.CreateEncryptor(keyData2, null); | ||
var encryptor2 = tDesObject.CreateEncryptor(keyData2, iv); | ||
eLen = encryptor2.TransformBlock(dataToEncrypt, 0, 8, result2, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
var encryptor3 = tDesObject.CreateEncryptor(keyData3, null); | ||
var encryptor3 = tDesObject.CreateEncryptor(keyData3, iv); | ||
eLen = encryptor3.TransformBlock(dataToEncrypt, 0, 8, result3, 0); | ||
Assert.Equal(8, eLen); | ||
|
var result3 = new byte[8]; | ||
|
||
var tDesObject = TripleDES.Create(); | ||
tDesObject.Mode = CipherMode.ECB; |
Check failure
Code scanning / CodeQL
Encryption using ECB High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 17 days ago
To fix the issue, update the encryption mode from ECB to CBC, which is standard and far more secure. CBC (Cipher Block Chaining) requires an initialization vector (IV) which should be unique and random for each encryption operation, but for unit tests, a fixed IV (e.g., all zeros) can be used to produce deterministic results. The changes should be focused on the region where the mode is set (line 183), and all invocations of .CreateEncryptor()
need the IV argument to be non-null. Add an IV definition to the method, and pass it to the encryptor. No additional imports are needed, but add the IV definition and update calls as required in the affected method (lines ~157-199).
-
Copy modified line R183 -
Copy modified lines R187-R190 -
Copy modified line R194 -
Copy modified line R199
@@ -180,20 +180,23 @@ | ||
var result3 = new byte[8]; | ||
|
||
var tDesObject = TripleDES.Create(); | ||
tDesObject.Mode = CipherMode.ECB; | ||
tDesObject.Mode = CipherMode.CBC; | ||
tDesObject.Padding = PaddingMode.None; | ||
tDesObject.KeySize = 128; | ||
|
||
var encryptor1 = tDesObject.CreateEncryptor(keyData1, null); | ||
// Use a fixed IV for deterministic test results. | ||
var testIV = new byte[8]; // 8 bytes for TripleDES block size | ||
|
||
var encryptor1 = tDesObject.CreateEncryptor(keyData1, testIV); | ||
var eLen = encryptor1.TransformBlock(dataToEncrypt, 0, 8, result1, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
var encryptor2 = tDesObject.CreateEncryptor(keyData2, null); | ||
var encryptor2 = tDesObject.CreateEncryptor(keyData2, testIV); | ||
eLen = encryptor2.TransformBlock(dataToEncrypt, 0, 8, result2, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
tDesObject.KeySize = 192; | ||
var encryptor3 = tDesObject.CreateEncryptor(keyData3, null); | ||
var encryptor3 = tDesObject.CreateEncryptor(keyData3, testIV); | ||
eLen = encryptor3.TransformBlock(dataToEncrypt, 0, 8, result3, 0); | ||
Assert.Equal(8, eLen); | ||
} |
var part2 = new byte[8]; | ||
|
||
var tDesObject = TripleDES.Create(); | ||
tDesObject.Mode = CipherMode.ECB; |
Check failure
Code scanning / CodeQL
Encryption using ECB High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 17 days ago
To fix this issue, replace ECB mode with CBC mode for both the TripleDES
and DES
objects. The CBC mode is more secure as it uses an IV (Initialization Vector) to ensure identical plaintext blocks encrypt differently. For unit testing with deterministic results, set the IV to a fixed value (e.g., all zeros), which will not impact security in the context of a unit test but avoids the use of ECB.
Specifically:
- Replace
tDesObject.Mode = CipherMode.ECB;
withtDesObject.Mode = CipherMode.CBC;
and settDesObject.IV = new byte[8];
- Replace
desObject.Mode = CipherMode.ECB;
withdesObject.Mode = CipherMode.CBC;
and setdesObject.IV = new byte[8];
- When creating encryptors or decryptors, replace the
null
IV parameters with the explicitly provided IV.
These changes will preserve the test's functionality while using a more secure cipher mode. No additional imports are required.
-
Copy modified line R234 -
Copy modified lines R236-R237 -
Copy modified line R241 -
Copy modified line R246 -
Copy modified lines R248-R249 -
Copy modified line R253
@@ -231,26 +231,26 @@ | ||
var part2 = new byte[8]; | ||
|
||
var tDesObject = TripleDES.Create(); | ||
tDesObject.Mode = CipherMode.ECB; | ||
tDesObject.Mode = CipherMode.CBC; | ||
tDesObject.Padding = PaddingMode.None; | ||
|
||
var encryptor0 = tDesObject.CreateEncryptor(keyDataT, null); | ||
tDesObject.IV = new byte[8]; | ||
var encryptor0 = tDesObject.CreateEncryptor(keyDataT, tDesObject.IV); | ||
var eLen = encryptor0.TransformBlock(dataToEncrypt, 0, 8, result1, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
var encryptor1 = tDesObject.CreateEncryptor(keyData1, null); | ||
var encryptor1 = tDesObject.CreateEncryptor(keyData1, tDesObject.IV); | ||
eLen = encryptor1.TransformBlock(dataToEncrypt, 0, 8, part1, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
var desObject = DES.Create(); | ||
desObject.Mode = CipherMode.ECB; | ||
desObject.Mode = CipherMode.CBC; | ||
desObject.Padding = PaddingMode.None; | ||
|
||
var decryptor2 = desObject.CreateDecryptor(keyData3, null); | ||
desObject.IV = new byte[8]; | ||
var decryptor2 = desObject.CreateDecryptor(keyData3, desObject.IV); | ||
eLen = decryptor2.TransformBlock(part1, 0, 8, part2, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
var encryptor2 = desObject.CreateEncryptor(keyData2, null); | ||
var encryptor2 = desObject.CreateEncryptor(keyData2, desObject.IV); | ||
eLen = encryptor2.TransformBlock(part2, 0, 8, result2, 0); | ||
Assert.Equal(8, eLen); | ||
} |
Assert.Equal(8, eLen); | ||
|
||
var desObject = DES.Create(); | ||
desObject.Mode = CipherMode.ECB; |
Check failure
Code scanning / CodeQL
Encryption using ECB High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To remediate this issue, change the cipher mode from ECB to a secure alternative, typically CBC (Cipher Block Chaining) mode, which is standard for both DES/TripleDES and avoids pattern leakage vulnerabilities. CBC requires an initialization vector (IV) to provide semantic security. In test code, the IV can be set to a zeroed byte array of the appropriate length if deterministic results are desired (as commonly done in tests), or a random IV otherwise.
The required changes are:
- Replace
tDesObject.Mode = CipherMode.ECB;
withtDesObject.Mode = CipherMode.CBC;
- Replace
desObject.Mode = CipherMode.ECB;
withdesObject.Mode = CipherMode.CBC;
- When creating encryptors or decryptors, replace the
null
IV with an explicit zero IV, e.g.new byte[8]
. - Ensure the test logic remains consistent with this mode switch.
All changes are to be made in the shown regions of Yubico.YubiKey/tests/unit/Yubico/YubiKey/Piv/KeyTests.cs
.
-
Copy modified line R234 -
Copy modified lines R237-R239 -
Copy modified line R243 -
Copy modified line R248 -
Copy modified line R251 -
Copy modified line R255
@@ -231,26 +231,28 @@ | ||
var part2 = new byte[8]; | ||
|
||
var tDesObject = TripleDES.Create(); | ||
tDesObject.Mode = CipherMode.ECB; | ||
tDesObject.Mode = CipherMode.CBC; | ||
tDesObject.Padding = PaddingMode.None; | ||
|
||
var encryptor0 = tDesObject.CreateEncryptor(keyDataT, null); | ||
// Using zero IV for test determinism; CBC mode requires IV of block size (8 bytes for DES/3DES) | ||
var zeroIV = new byte[8]; | ||
var encryptor0 = tDesObject.CreateEncryptor(keyDataT, zeroIV); | ||
var eLen = encryptor0.TransformBlock(dataToEncrypt, 0, 8, result1, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
var encryptor1 = tDesObject.CreateEncryptor(keyData1, null); | ||
var encryptor1 = tDesObject.CreateEncryptor(keyData1, zeroIV); | ||
eLen = encryptor1.TransformBlock(dataToEncrypt, 0, 8, part1, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
var desObject = DES.Create(); | ||
desObject.Mode = CipherMode.ECB; | ||
desObject.Mode = CipherMode.CBC; | ||
desObject.Padding = PaddingMode.None; | ||
|
||
var decryptor2 = desObject.CreateDecryptor(keyData3, null); | ||
var decryptor2 = desObject.CreateDecryptor(keyData3, zeroIV); | ||
eLen = decryptor2.TransformBlock(part1, 0, 8, part2, 0); | ||
Assert.Equal(8, eLen); | ||
|
||
var encryptor2 = desObject.CreateEncryptor(keyData2, null); | ||
var encryptor2 = desObject.CreateEncryptor(keyData2, zeroIV); | ||
eLen = encryptor2.TransformBlock(part2, 0, 8, result2, 0); | ||
Assert.Equal(8, eLen); | ||
} |
byte[]? sw = new byte[sizeof(short)]; | ||
BinaryPrimitives.WriteInt16BigEndian(sw, SWConstants.DataNotFound); | ||
var responseApdu = new ResponseApdu(new byte[] { sw[0], sw[1] }); | ||
tripleDes.Mode = CipherMode.ECB; |
Check failure
Code scanning / CodeQL
Encryption using ECB High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 17 days ago
To address this issue, replace the insecure use of CipherMode.ECB
with a secure cipher mode—most commonly CipherMode.CBC
. CBC mode requires an Initialization Vector (IV) to ensure the same plaintext encrypts to different ciphertexts each time. In the context of this code, which appears to encrypt 8 bytes from a buffer, you can generate a random IV or use a fixed IV if deterministic outputs are needed for testing only. You must modify both the cipher mode and supply a suitable IV (instead of null
) when creating the encryptor.
Edit only within the supplied lines (lines 111–114) in Yubico.YubiKey/tests/utilities/Yubico/YubiKey/TestUtilities/HollowConnection.cs
:
- Change
CipherMode.ECB
toCipherMode.CBC
. - Generate and provide an appropriate IV (8 bytes for TripleDES) for use with CBC mode, replacing the
null
IV argument in theCreateEncryptor
call. - If you need deterministic output for tests, you can use a fixed IV (e.g., all zeros or any test vector); otherwise, generate a random value.
No extra dependencies are necessary; .NET standard libraries are sufficient.
-
Copy modified line R111 -
Copy modified lines R113-R115
@@ -108,9 +108,11 @@ | ||
|
||
using var tripleDes = CryptographyProviders.TripleDesCreator(); | ||
|
||
tripleDes.Mode = CipherMode.ECB; | ||
tripleDes.Mode = CipherMode.CBC; | ||
tripleDes.Padding = PaddingMode.None; | ||
using var encryptor = tripleDes.CreateEncryptor(keyBytes, null); | ||
// For deterministic test output, use a fixed IV. If not required, replace with a random IV. | ||
var iv = new byte[] { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }; | ||
using var encryptor = tripleDes.CreateEncryptor(keyBytes, iv); | ||
_ = encryptor.TransformBlock(data, 14, 8, responseData, 4); | ||
|
||
var responseApdu = new ResponseApdu(responseData); |
Solution wide code cleanup using Rider and our common settings