Secure Java RSA Crypto Not Recognized By CodeQL

[HaleenUptain]

trafficstars

Description of the false positive

CodeQL is generating false positive alerts on Java applications that implement RSA cryptography securely. It is risky for Developers to get in the habit of just “Dismissing Alerts”. Instead, I would rather work with the GHAS Engineering team so the CodeQL scanner is continuously improved to better recognize and handle correctly without generating these alerts in the first place.

Code samples or links to source code

Not only does this code generate "Use of a broken or risky cryptographic algorithm" and "Use of a potentially broken or risky cryptographic algorithm" false positive alerts, examples on how to implement Java using RSA algorithm with recommended padding scheme is missing from these two alert recommendations. In addition to eliminating these two false positives, adding this Java RSA example to these two alert recommendations would be helpful.

// JAVA RSA Example
String ALGORITHM_NAME = "RSA" ;
String PADDING_SCHEME = "OAEPWITHSHA-512ANDMGF1PADDING" ;
String MODE_OF_OPERATION = "ECB" ; // This essentially means none behind the scenes for RSA

Cipher encryptCipher = Cipher.getInstance(ALGORITHM_NAME + "/" + MODE_OF_OPERATION + "/" + PADDING_SCHEME);
encryptCipher.init(Cipher.ENCRYPT_MODE, publicKey);

import java.io.DataInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.security.KeyFactory;
import java.util.Base64;
import javax.crypto.Cipher;
import java.security.spec.X509EncodedKeySpec;
import java.nio.charset.StandardCharsets;
import java.security.interfaces.RSAPrivateKey;
import java.security.interfaces.RSAPublicKey;
import java.security.spec.PKCS8EncodedKeySpec;

public class JavaRSAEncryptionTest 
{
    public static void main( String[] args )
    {
        System.out.println( "Java RSA Encryption Test!" );
        
        String ALGORITHM_NAME = "RSA" ;
        String PADDING_SCHEME = "OAEPWITHSHA-512ANDMGF1PADDING" ;
        String MODE_OF_OPERATION = "ECB" ; // This essentially means none behind the scene
        
        File publicKeyFile = new File("public_key.der");
        File privateKeyFile = new File("private_key.der");
       
        try
        {
           // Read in keys
        	
           // Get the private key
           FileInputStream privateKeyFis = new FileInputStream(privateKeyFile);
           DataInputStream privateKeyDis = new DataInputStream(privateKeyFis);
           byte[] privateKeyBytes = new byte[(int) privateKeyFile.length()];
           privateKeyDis.readFully(privateKeyBytes);
           privateKeyDis.close();
        	  
           PKCS8EncodedKeySpec privateKeySpec = new PKCS8EncodedKeySpec(privateKeyBytes);
           KeyFactory keyFactoryPrivate = KeyFactory.getInstance(ALGORITHM_NAME);
           RSAPrivateKey privateKey = (RSAPrivateKey) keyFactoryPrivate.generatePrivate(privateKeySpec);
                         
           // Get the public key
           FileInputStream publicKeyFis = new FileInputStream(publicKeyFile);
           DataInputStream publicKeyDis = new DataInputStream(publicKeyFis);
           byte[] publicKeyBytes = new byte[(int) publicKeyFile.length()];
           publicKeyDis.readFully(publicKeyBytes);
           publicKeyDis.close();
        	  
           X509EncodedKeySpec publicKeySpec = new X509EncodedKeySpec(publicKeyBytes);
           KeyFactory keyFactoryPublic = KeyFactory.getInstance(ALGORITHM_NAME);
           RSAPublicKey publicKey = (RSAPublicKey) keyFactoryPublic.generatePublic(publicKeySpec);
                    
           // Encrypt test string.
           String secretMessage = "Test secret message";
        
           Cipher encryptCipher = Cipher.getInstance(ALGORITHM_NAME + "/" + MODE_OF_OPERATION + "/" + PADDING_SCHEME);
           encryptCipher.init(Cipher.ENCRYPT_MODE, publicKey);
          
           byte[] secretMessageBytes = secretMessage.getBytes(StandardCharsets.UTF_8);
           byte[] encryptedMessageBytes = encryptCipher.doFinal(secretMessageBytes);
        
           String encodedMessage = Base64.getEncoder().encodeToString(encryptedMessageBytes);
        
           Cipher decryptCipher = Cipher.getInstance(ALGORITHM_NAME + "/" + MODE_OF_OPERATION + "/" + PADDING_SCHEME);
           decryptCipher.init(Cipher.DECRYPT_MODE, privateKey);
           byte[] decryptedMessageBytes = decryptCipher.doFinal(encryptedMessageBytes);
           String decryptedMessage = new String(decryptedMessageBytes, StandardCharsets.UTF_8);
        
           if (secretMessage.equals(decryptedMessage))
           {
        	  System.out.println("Secret message decrypted successfully.");
           }
           else
           {
              System.out.println("Secret message decryption failed.");
           }
        }
        catch (Exception e)
        {
        	System.out.println(e.getMessage());
        }
    }
}

[atorralba]

Hey @HaleenUptain, thanks for letting us know about this FP! After review, it looks like you're right: we're flagging the use of the string ECB in cryptographic algorithms as problematic, but we fail to check whether it co-occurs with RSA.

We're going to track this internally and fix the issue, together with considering your suggestions about the recommendations and fix examples, when we have capacity for it.

Thanks again!

[RobbingDaHood]

I just read this https://crypto.stackexchange.com/a/26539 and for me knowing very little about cryptography then it does seem like this is not insecure.

You could still argue that it is far better to write RSA/none/OAEPWITHSHA-512ANDMGF1PADDING instead of RSA/ECB/OAEPWITHSHA-512ANDMGF1PADDING: So the change could be to remove this from the two existing alerts and then create a new alert suggesting to change the MODE_OF_OPERATION to none in case of RSA: Because that is what is implicitly done. I do not have enough experience with the design of alerts to know if that would be a very low severity alert?