Use secure mode and padding scheme. Open
encryptCipher = Cipher.getInstance(ENC_TYPE_FOR_PROPS);
- Read upRead up
- Exclude checks
Encryption operation mode and the padding scheme should be chosen appropriately to guarantee data confidentiality, integrity and authenticity:
- For block cipher encryption algorithms (like AES):
- The GCM (Galois Counter Mode) mode which works internally with zero/no padding scheme, is recommended, as it is designed to provide both data authenticity (integrity) and confidentiality. Other similar modes are CCM, CWC, EAX, IAPM and OCB.
- The CBC (Cipher Block Chaining) mode by itself provides only data confidentiality, it's recommended to use it along with Message Authentication Code or similar to achieve data authenticity (integrity) too and thus to prevent padding oracle attacks.
- The ECB (Electronic Codebook) mode doesn't provide serious message confidentiality: under a given key any given plaintext block always gets encrypted to the same ciphertext block. This mode should not be used.
- For RSA encryption algorithm, the recommended padding scheme is OAEP.
Noncompliant Code Example
Cipher c1 = Cipher.getInstance("AES"); // Noncompliant: by default ECB mode is chosen Cipher c2 = Cipher.getInstance("AES/ECB/NoPadding"); // Noncompliant: ECB doesn't provide serious message confidentiality Cipher c3 = Cipher.getInstance("RSA/NONE/NoPadding"); // Noncompliant: RSA without OAEP padding scheme is not recommanded
Compliant Solution
// Recommended for block ciphers Cipher c1 = Cipher.getInstance("AES/GCM/NoPadding"); // Compliant // Recommended for RSA Cipher c2= Cipher.getInstance("RSA/None/OAEPWithSHA-1AndMGF1Padding"); // Compliant Cipher c3 = Cipher.getInstance("RSA/None/OAEPWITHSHA-256ANDMGF1PADDING"); // Compliant
See
- OWASP Top 10 2017 Category A6 - Security Misconfiguration
- MITRE, CWE-327 - Use of a Broken or Risky Cryptographic Algorithm
- CERT, MSC61-J. - Do not use insecure or weak cryptographic algorithms
- SANS Top 25 - Porous Defenses
Define a constant instead of duplicating this literal " cipher." 4 times. Open
throw new EncryptionException("Failed to obtain " + ENC_TYPE_FOR_PROPS + " cipher.");
- Read upRead up
- Exclude checks
Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.
On the other hand, constants can be referenced from many places, but only need to be updated in a single place.
Noncompliant Code Example
With the default threshold of 3:
public void run() { prepare("action1"); // Noncompliant - "action1" is duplicated 3 times execute("action1"); release("action1"); } @SuppressWarning("all") // Compliant - annotations are excluded private void method1() { /* ... */ } @SuppressWarning("all") private void method2() { /* ... */ } public String method3(String a) { System.out.println("'" + a + "'"); // Compliant - literal "'" has less than 5 characters and is excluded return ""; // Compliant - literal "" has less than 5 characters and is excluded }
Compliant Solution
private static final String ACTION_1 = "action1"; // Compliant public void run() { prepare(ACTION_1); // Compliant execute(ACTION_1); release(ACTION_1); }
Exceptions
To prevent generating some false-positives, literals having less than 5 characters are excluded.
Either re-interrupt this method or rethrow the "InterruptedException" that can be caught here. Open
} catch (IOException | InterruptedException e) {
- Read upRead up
- Exclude checks
InterruptedExceptions
should never be ignored in the code, and simply logging the exception counts in this case as "ignoring". The
throwing of the InterruptedException
clears the interrupted state of the Thread, so if the exception is not handled properly the fact
that the thread was interrupted will be lost. Instead, InterruptedExceptions
should either be rethrown - immediately or after cleaning up
the method's state - or the thread should be re-interrupted by calling Thread.interrupt()
even if this is supposed to be a
single-threaded application. Any other course of action risks delaying thread shutdown and loses the information that the thread was interrupted -
probably without finishing its task.
Similarly, the ThreadDeath
exception should also be propagated. According to its JavaDoc:
If
ThreadDeath
is caught by a method, it is important that it be rethrown so that the thread actually dies.
Noncompliant Code Example
public void run () { try { while (true) { // do stuff } }catch (InterruptedException e) { // Noncompliant; logging is not enough LOGGER.log(Level.WARN, "Interrupted!", e); } }
Compliant Solution
public void run () { try { while (true) { // do stuff } }catch (InterruptedException e) { LOGGER.log(Level.WARN, "Interrupted!", e); // Restore interrupted state... Thread.currentThread().interrupt(); } }
See
- MITRE, CWE-391 - Unchecked Error Condition
- Dealing with InterruptedException