-
Notifications
You must be signed in to change notification settings - Fork 82
Mosip 42334 #445
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
Mosip 42334 #445
Conversation
* [MOSIP-40013] Signed-off-by: JanardhanBS-SyncByte <[email protected]> * [MOSIP-40013] Signed-off-by: JanardhanBS-SyncByte <[email protected]> --------- Signed-off-by: JanardhanBS-SyncByte <[email protected]>
* MOSIP-42334 corrected version Signed-off-by: kameshsr <[email protected]> * MOSIP-42334 Changed AES_ECB AES_GCM in doCipherOps Signed-off-by: kameshsr <[email protected]> * MOSIP-42334 Changed AES_ECB AES_GCM in doCipherOps Signed-off-by: kameshsr <[email protected]> * MOSIP-42334 Added predestroy Signed-off-by: kameshsr <[email protected]> --------- Signed-off-by: kameshsr <[email protected]>
Signed-off-by: kameshsr <[email protected]>
MOSIP-42357 corrected version
* MOSIP-42357 Corrected CryptoCore to improve performance Signed-off-by: kameshsr <[email protected]> * MOSIP-42357 Corrected keymanager util Signed-off-by: kameshsr <[email protected]> --------- Signed-off-by: kameshsr <[email protected]>
Signed-off-by: kameshsr <[email protected]>
| // Used as a hack for softhsm oeap padding decryption usecase will be when we | ||
| // will use in HSM | ||
| @SuppressWarnings("java:S106") | ||
| private static final String RSA_ECB_NO_PADDING = "RSA/ECB/NoPadding"; // NOSONAR using the padding for allowing OAEP padding in PKCS11 library |
Check failure
Code scanning / CodeQL
Use of RSA algorithm without OAEP High
initialize an RSA cipher
This specification is used to
initialize an RSA cipher
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The best solution is to avoid using "RSA/ECB/NoPadding" and instead use "RSA/ECB/OAEPWithSHA-256AndMGF1Padding" (or similar, depending on what is supported). This ensures that padding and unpadding are handled by trusted, tested library functions, which drastically lowers the risk of padding oracle and similar attacks. The main required change is in the cipher instantiation within asymmetricDecrypt:
Replace all references in this method to "RSA/ECB/NoPadding" (RSA_ECB_NO_PADDING) with "RSA/ECB/OAEPWithSHA-256AndMGF1Padding", using standard OAEP parameters with SHA-256 as the hash and MGF1.
- The constant
RSA_ECB_NO_PADDINGon line 85 can be replaced or a new appropriately named constant can be added (e.g.,RSA_ECB_OAEP_PADDING). - The cipher initialization in
asymmetricDecryptshould be updated to use the OAEP algorithm and, if required, consistently provide the OAEP parameters (with SHA-256, MGF1, and default PSource). - Remove the call to the custom/deprecated
unpadOAEPPadding, as OAEP unpadding will be handled by the Cipher. - Remove the workaround comment and related code blocks.
The changes are localized to asymmetricDecrypt and the relevant constant, with no change to the rest of the class functionality.
-
Copy modified line R85 -
Copy modified lines R370-R372 -
Copy modified lines R380-R387 -
Copy modified line R392
| @@ -82,7 +82,7 @@ | ||
| // Used as a hack for softhsm oeap padding decryption usecase will be when we | ||
| // will use in HSM | ||
| @SuppressWarnings("java:S106") | ||
| private static final String RSA_ECB_NO_PADDING = "RSA/ECB/NoPadding"; // NOSONAR using the padding for allowing OAEP padding in PKCS11 library | ||
| private static final String RSA_ECB_OAEP_PADDING = "RSA/ECB/OAEPWithSHA-256AndMGF1Padding"; | ||
|
|
||
| private static final String PKCS11_STORE_TYPE = "PKCS11"; | ||
|
|
||
| @@ -367,8 +367,9 @@ | ||
| CryptoUtils.verifyData(data); | ||
| Cipher cipher; | ||
| try { | ||
| cipher = Objects.isNull(storeType) ? Cipher.getInstance(RSA_ECB_NO_PADDING) : // NOSONAR using the padding for allowing OAEP padding in PKCS11 library | ||
| Cipher.getInstance(RSA_ECB_NO_PADDING, storeType); // NOSONAR using the padding for allowing OAEP padding in PKCS11 library | ||
| cipher = Objects.isNull(storeType) | ||
| ? Cipher.getInstance(RSA_ECB_OAEP_PADDING) | ||
| : Cipher.getInstance(RSA_ECB_OAEP_PADDING, storeType); | ||
| } catch (java.security.NoSuchAlgorithmException | NoSuchPaddingException | NoSuchProviderException e) { | ||
| throw new NoSuchAlgorithmException( | ||
| SecurityExceptionCodeConstant.MOSIP_NO_SUCH_ALGORITHM_EXCEPTION.getErrorCode(), | ||
| @@ -376,24 +377,19 @@ | ||
| } | ||
|
|
||
| try { | ||
| cipher.init(Cipher.DECRYPT_MODE, privateKey); | ||
| } catch (java.security.InvalidKeyException e) { | ||
| OAEPParameterSpec oaepParams = new OAEPParameterSpec( | ||
| HASH_ALGO, | ||
| MGF1, | ||
| new MGF1ParameterSpec(HASH_ALGO), | ||
| PSpecified.DEFAULT | ||
| ); | ||
| cipher.init(Cipher.DECRYPT_MODE, privateKey, oaepParams); | ||
| } catch (java.security.InvalidKeyException | InvalidAlgorithmParameterException e) { | ||
| throw new InvalidKeyException(SecurityExceptionCodeConstant.MOSIP_INVALID_KEY_EXCEPTION.getErrorCode(), | ||
| e.getMessage(), e); | ||
| } | ||
| /* | ||
| * This is a hack of removing OEAP padding after decryption with NO Padding as | ||
| * SoftHSM does not support it.Will be removed after HSM implementation | ||
| */ | ||
| byte[] paddedPlainText = doFinal(data, cipher); | ||
| if (paddedPlainText.length < asymmetricKeyLength / 8) { | ||
| byte[] tempPipe = new byte[asymmetricKeyLength / 8]; | ||
| System.arraycopy(paddedPlainText, 0, tempPipe, tempPipe.length - paddedPlainText.length, | ||
| paddedPlainText.length); | ||
| paddedPlainText = tempPipe; | ||
| } | ||
|
|
||
| return unpadOAEPPadding(paddedPlainText, keyModulus); | ||
| return doFinal(data, cipher); | ||
| } | ||
|
|
||
| // This is a hack of removing OEAP padding after decryption with NO Padding as |
| CryptoUtils.verifyData(data); | ||
| Cipher cipher; | ||
| try { | ||
| cipher = Objects.isNull(storeType) ? Cipher.getInstance(RSA_ECB_NO_PADDING) : // NOSONAR using the padding for allowing OAEP padding in PKCS11 library |
Check failure
Code scanning / CodeQL
Use of RSA algorithm without OAEP High
initialize an RSA cipher
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To properly fix the vulnerability, the RSA cipher initialization must use OAEP padding instead of no padding. Specifically, in the asymmetricDecrypt method at line 370, update the cipher initialization from RSA_ECB_NO_PADDING (likely "RSA/ECB/NoPadding") to "RSA/ECB/OAEPWithSHA-256AndMGF1Padding". Additionally, OAEP padding should be configured via an OAEPParameterSpec to ensure MGF1 and a hash function (like SHA-256) are used. This change applies unless the keystore type or HSM provider truly does not support OAEP; in such cases, a clear fallback and possibly a blocking exception with a strong warning should be used instead of proceeding with insecure decryption.
Edit the cipher initialization block and also the cipher.init(...) call on line 379 to supply the OAEP parameters for key decryption. Remove or update the subsequent manual unpadding, since the cipher should now handle padding natively.
- Edit the cipher
getInstancecalls (line 370/371) to request OAEP padding. - Add/support OAEP parameters via
OAEPParameterSpecas needed. - Remove or bypass the manual OAEP unpadding (
unpadOAEPPadding) since padding will be handled by the cipher. - Ensure imports for OAEP are present (
javax.crypto.spec.OAEPParameterSpecetc.). - Make sure any callers (e.g.,
asymmetricDecryptoverloads) calling this are not broken by the change.
-
Copy modified lines R370-R375 -
Copy modified lines R381-R382 -
Copy modified lines R384-R386 -
Copy modified line R389
| @@ -367,33 +367,26 @@ | ||
| CryptoUtils.verifyData(data); | ||
| Cipher cipher; | ||
| try { | ||
| cipher = Objects.isNull(storeType) ? Cipher.getInstance(RSA_ECB_NO_PADDING) : // NOSONAR using the padding for allowing OAEP padding in PKCS11 library | ||
| Cipher.getInstance(RSA_ECB_NO_PADDING, storeType); // NOSONAR using the padding for allowing OAEP padding in PKCS11 library | ||
| } catch (java.security.NoSuchAlgorithmException | NoSuchPaddingException | NoSuchProviderException e) { | ||
| // Use OAEP padding for RSA decryption | ||
| OAEPParameterSpec oaepParams = new OAEPParameterSpec("SHA-256", "MGF1", MGF1ParameterSpec.SHA256, PSpecified.DEFAULT); | ||
| cipher = Objects.isNull(storeType) ? Cipher.getInstance("RSA/ECB/OAEPWithSHA-256AndMGF1Padding") : | ||
| Cipher.getInstance("RSA/ECB/OAEPWithSHA-256AndMGF1Padding", storeType); | ||
| cipher.init(Cipher.DECRYPT_MODE, privateKey, oaepParams); | ||
| } catch (java.security.NoSuchAlgorithmException | NoSuchPaddingException | NoSuchProviderException | java.security.InvalidKeyException | InvalidAlgorithmParameterException e) { | ||
| throw new NoSuchAlgorithmException( | ||
| SecurityExceptionCodeConstant.MOSIP_NO_SUCH_ALGORITHM_EXCEPTION.getErrorCode(), | ||
| SecurityExceptionCodeConstant.MOSIP_NO_SUCH_ALGORITHM_EXCEPTION.getErrorMessage(), e); | ||
| } | ||
|
|
||
| try { | ||
| cipher.init(Cipher.DECRYPT_MODE, privateKey); | ||
| } catch (java.security.InvalidKeyException e) { | ||
| throw new InvalidKeyException(SecurityExceptionCodeConstant.MOSIP_INVALID_KEY_EXCEPTION.getErrorCode(), | ||
| e.getMessage(), e); | ||
| } | ||
| /* | ||
| * This is a hack of removing OEAP padding after decryption with NO Padding as | ||
| * SoftHSM does not support it.Will be removed after HSM implementation | ||
| */ | ||
| byte[] paddedPlainText = doFinal(data, cipher); | ||
| if (paddedPlainText.length < asymmetricKeyLength / 8) { | ||
| byte[] plainText = doFinal(data, cipher); | ||
| if (plainText.length < asymmetricKeyLength / 8) { | ||
| byte[] tempPipe = new byte[asymmetricKeyLength / 8]; | ||
| System.arraycopy(paddedPlainText, 0, tempPipe, tempPipe.length - paddedPlainText.length, | ||
| paddedPlainText.length); | ||
| paddedPlainText = tempPipe; | ||
| System.arraycopy(plainText, 0, tempPipe, tempPipe.length - plainText.length, | ||
| plainText.length); | ||
| plainText = tempPipe; | ||
| } | ||
|
|
||
| return unpadOAEPPadding(paddedPlainText, keyModulus); | ||
| return plainText; | ||
| } | ||
|
|
||
| // This is a hack of removing OEAP padding after decryption with NO Padding as |
| Cipher cipher; | ||
| try { | ||
| cipher = Objects.isNull(storeType) ? Cipher.getInstance(RSA_ECB_NO_PADDING) : // NOSONAR using the padding for allowing OAEP padding in PKCS11 library | ||
| Cipher.getInstance(RSA_ECB_NO_PADDING, storeType); // NOSONAR using the padding for allowing OAEP padding in PKCS11 library |
Check failure
Code scanning / CodeQL
Use of RSA algorithm without OAEP High
initialize an RSA cipher
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The best fix is to use OAEP padding in the Cipher instance, i.e., replace RSA_ECB_NO_PADDING with a specification that uses OAEP padding (e.g., "RSA/ECB/OAEPWithSHA-256AndMGF1Padding" or similar, depending on your environment and provider). If storeType is not provided, use OAEP padding by default; if SoftHSM now supports OAEP, use it there too.
If OAEP is not supported by the existing provider, the underlying HSM or PKCS#11 module may need to be upgraded. If so, fallback logic (which is discouraged) could remain temporarily, but ONLY after all feasible efforts to upgrade to OAEP support are made.
In the code region around lines 370-371, change how the Cipher is instantiated to always use OAEP padding, removing any use of "NoPadding". You may also need to adapt how manual OAEP unpadding is done—ideally, remove it because the cipher now handles the padding properly.
Specifically:
- Change
RSA_ECB_NO_PADDINGto an OAEP padding string, e.g.,"RSA/ECB/OAEPWithSHA-256AndMGF1Padding" - Remove manual OAEP unpadding if possible
- Adjust imports only if necessary, but you have all needed OAEP classes imported.
-
Copy modified lines R370-R371 -
Copy modified line R396
| @@ -367,8 +367,8 @@ | ||
| CryptoUtils.verifyData(data); | ||
| Cipher cipher; | ||
| try { | ||
| cipher = Objects.isNull(storeType) ? Cipher.getInstance(RSA_ECB_NO_PADDING) : // NOSONAR using the padding for allowing OAEP padding in PKCS11 library | ||
| Cipher.getInstance(RSA_ECB_NO_PADDING, storeType); // NOSONAR using the padding for allowing OAEP padding in PKCS11 library | ||
| cipher = Objects.isNull(storeType) ? Cipher.getInstance("RSA/ECB/OAEPWithSHA-256AndMGF1Padding") : | ||
| Cipher.getInstance("RSA/ECB/OAEPWithSHA-256AndMGF1Padding", storeType); | ||
| } catch (java.security.NoSuchAlgorithmException | NoSuchPaddingException | NoSuchProviderException e) { | ||
| throw new NoSuchAlgorithmException( | ||
| SecurityExceptionCodeConstant.MOSIP_NO_SUCH_ALGORITHM_EXCEPTION.getErrorCode(), | ||
| @@ -393,7 +393,7 @@ | ||
| paddedPlainText = tempPipe; | ||
| } | ||
|
|
||
| return unpadOAEPPadding(paddedPlainText, keyModulus); | ||
| return paddedPlainText; | ||
| } | ||
|
|
||
| // This is a hack of removing OEAP padding after decryption with NO Padding as |
|
|
||
| CIPHER_AES_ECB_NO_PADDING = ThreadLocal.withInitial(() -> { | ||
| try { | ||
| return Cipher.getInstance(WRAPPING_TRANSFORMATION); |
Check failure
Code scanning / CodeQL
Use of a broken or risky cryptographic algorithm High
AES/ECB/NoPadding
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, the cipher transformation should be changed from "AES/ECB/NoPadding" to "AES/GCM/NoPadding". GCM is a modern, secure, authenticated mode for AES, mitigating the risks inherent in ECB.
- Edit the
WRAPPING_TRANSFORMATIONstring on line 43 to"AES/GCM/NoPadding". - The cipher initialization at line 80 (
Cipher.getInstance(WRAPPING_TRANSFORMATION)) will now use GCM. - Any use of
Ciphercreated through this thread-local should be audited (outside your snippet) to ensure it properly provides and expects the GCM mode, specifically setting a secure random IV for GCM mode (e.g., viaGCMParameterSpec). You should at minimum add an inline note indicating that callers must set a secure IV and GCM parameters before use. - No extra dependencies are needed; Java's standard library supports GCM mode from Java 8 onwards.
- No other code blocks in the provided snippet show compatibility concerns, but if logic is later found that assumes ECB (e.g., block sizes, lack of IV), further refactoring may be required.
-
Copy modified lines R43-R45 -
Copy modified line R82
| @@ -40,7 +40,9 @@ | ||
|
|
||
| private static final String CREATED_BY = "System"; | ||
|
|
||
| private static final String WRAPPING_TRANSFORMATION = "AES/ECB/NoPadding"; // NOSONAR using the key wrapping | ||
| // Use AES/GCM/NoPadding for secure authenticated encryption. | ||
| // IMPORTANT: When using GCM, always initialize the Cipher with a secure random IV and GCMParameterSpec. | ||
| private static final String WRAPPING_TRANSFORMATION = "AES/GCM/NoPadding"; | ||
|
|
||
| @Value("${zkcrypto.random.key.generate.count}") | ||
| private long noOfKeysRequire; | ||
| @@ -77,6 +79,7 @@ | ||
|
|
||
| CIPHER_AES_ECB_NO_PADDING = ThreadLocal.withInitial(() -> { | ||
| try { | ||
| // Cipher will use AES/GCM/NoPadding; callers must initialize with a secure IV and GCMParameterSpec. | ||
| return Cipher.getInstance(WRAPPING_TRANSFORMATION); | ||
| } catch (Exception e) { | ||
| throw new IllegalStateException("Unable to initialize Cipher with AES/ECB/NoPadding", e); |
No description provided.