Security fixes based on static analysis (#683)

Security fixes based on static analysis. Signed-off-by: Benny <[email protected]>
fido-device-onboard · Mar 19, 2024 · 9efd074 · 9efd074
1 parent b2137da
commit 9efd074
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 8 deletions.
diff --git a/protocol/src/main/java/org/fidoalliance/fdo/protocol/OnDieCertSignatureFunction.java b/protocol/src/main/java/org/fidoalliance/fdo/protocol/OnDieCertSignatureFunction.java
@@ -46,6 +46,10 @@
 public class OnDieCertSignatureFunction implements CertSignatureFunction {
 
   private final CertificateFactory certFactory;
+
+  private static  final String certType = "X.509";
+
+  private static final String connectionScheme = "http";
   private static final LoggerService logger = new LoggerService(OnDieCertSignatureFunction.class);
 
   /**
@@ -54,7 +58,7 @@ public class OnDieCertSignatureFunction implements CertSignatureFunction {
   public OnDieCertSignatureFunction() throws IOException {
     try {
       certFactory = CertificateFactory.getInstance(
-          "X.509", // TODO Const.X509_ALG_NAME,
+          certType, // TODO Const.X509_ALG_NAME,
           new BouncyCastleFipsProvider());
     } catch (CertificateException e) {
       throw new IOException(e);
@@ -304,11 +308,11 @@ public boolean checkRevocations(Certificate[] certificateList) {
                       GeneralNames.getInstance(dp.getDistributionPoint().getName()).getNames();
               for (GeneralName generalName : generalNames) {
                 String name = generalName.toString();
-                byte[] crlBytes = certManager.getCertificate(name.substring(name.indexOf("http")));
+                byte[] crlBytes = certManager.getCertificate(
+                        name.substring(name.indexOf(connectionScheme)));
                 if (crlBytes == null) {
-                  // TODO logger.info("CRL: " + generalName.getName().toString()
-                  //        + " not found in cache for cert: "
-                  //        + x509cert.getIssuerX500Principal().getName());
+                  logger.info("CRL: " + generalName.getName().toString()
+                          + " not found in cache for cert:");
                   return false;
                 } else {
                   CRL crl = certificateFactory.generateCRL(new ByteArrayInputStream(crlBytes));

diff --git a/protocol/src/main/java/org/fidoalliance/fdo/protocol/PemFormatter.java b/protocol/src/main/java/org/fidoalliance/fdo/protocol/PemFormatter.java
@@ -14,6 +14,7 @@
 import java.security.cert.Certificate;
 import java.security.interfaces.ECPrivateKey;
 import java.security.interfaces.RSAPrivateKey;
+import java.util.Arrays;
 import java.util.List;
 import org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider;
 import org.bouncycastle.openssl.PKCS8Generator;
@@ -96,14 +97,18 @@ public static String format(PublicKey publicKey) throws IOException {
   public static String formatKey(PrivateKey key, SecureRandom random, String password)
       throws IOException {
 
+    char[] passwordChars = password.toCharArray();
+    password = null;
+
     try (StringWriter writer = new StringWriter();
         PemWriter pemWriter = new PemWriter(writer)) {
 
       JceOpenSSLPKCS8EncryptorBuilder encryptorBuilder = new JceOpenSSLPKCS8EncryptorBuilder(
           PKCS8Generator.AES_256_CBC);
       encryptorBuilder.setProvider(new BouncyCastleFipsProvider());
       encryptorBuilder.setRandom(random);
-      encryptorBuilder.setPasssword(password.toCharArray());
+      encryptorBuilder.setPasssword(passwordChars);
+      Arrays.fill(passwordChars, '\0');
 
       OutputEncryptor oe = null;
       try {

diff --git a/protocol/src/main/java/org/fidoalliance/fdo/protocol/StandardCryptoService.java b/protocol/src/main/java/org/fidoalliance/fdo/protocol/StandardCryptoService.java
@@ -101,19 +101,32 @@ public class StandardCryptoService implements CryptoService {
 
   public static final String X509_ALG_NAME = "X.509";
   public static final String VALIDATOR_ALG_NAME = "PKIX";
+  public static final String RSA_CIPHER_SUITE = "RSA/NONE/OAEPWithSHA256AndMGF1Padding";
 
   private static final Provider BCFIPS = getInitializedProvider();
   protected static final SecureRandom random = getInitializedRandom();
 
 
   private static SecureRandom getInitializedRandom() {
 
+    SecureRandom entropySource = new SecureRandom();
+    // Create a unique nonce with the current time and a random value
+    long timestamp = System.currentTimeMillis();
+    byte[] randomBytes = new byte[16];
+    entropySource.nextBytes(randomBytes);
+
+    ByteBuffer buffer = ByteBuffer.allocate(Long.BYTES + randomBytes.length);
+    buffer.putLong(timestamp);
+    buffer.put(randomBytes);
+    byte[] nonce = buffer.array();
+
     // DRBG -- Discrete Random Bit Generator.
     EntropySourceProvider entSource = new BasicEntropySourceProvider(new SecureRandom(), true);
     FipsDRBG.Builder drgbBldr = FipsDRBG.SHA512_HMAC.fromEntropySource(entSource)
             .setSecurityStrength(256)
             .setEntropyBitsRequired(256);
-    return drgbBldr.build("nonce".getBytes(StandardCharsets.UTF_8), false);
+
+    return drgbBldr.build(nonce, false);
 
   }
 
@@ -480,7 +493,7 @@ protected KexMessage getAsymkexMessage(int randomSize,
 
         byte[] xb;
         try {
-          Cipher cipher = Cipher.getInstance("RSA/NONE/OAEPWithSHA256AndMGF1Padding",
+          Cipher cipher = Cipher.getInstance(RSA_CIPHER_SUITE,
               getProvider());
           cipher.init(Cipher.ENCRYPT_MODE, decodeKey(ownerKey), getSecureRandom());
           xb = cipher.doFinal(b);