8255494: PKCS7 should use digest algorithm to verify the signature

Reviewed-by: valeriep

src/java.base/share/classes/sun/security/pkcs/SignerInfo.java

@@ -260,8 +260,6 @@ public class SignerInfo implements DerEncoder {
         out.write(tmp.toByteArray());
     }
 
-
-
     /*
      * Returns the (user) certificate pertaining to this SignerInfo.
      */
@@ -503,24 +501,27 @@ public class SignerInfo implements DerEncoder {
 
     /**
      * Derives the signature algorithm name from the digest algorithm
-     * name and the encryption algorithm name inside a PKCS7 SignerInfo.
+     * and the encryption algorithm inside a PKCS7 SignerInfo.
      *
-     * For old style PKCS7 files where we use RSA, DSA, EC as encAlgId
+     * The digest algorithm is in the form "DIG", and the encryption
-     * a DIGESTwithENC algorithm is returned. For new style RSASSA-PSS
+     * algorithm can be in any of the 3 forms:
-     * and EdDSA encryption, this method ensures digAlgId is compatible
+     *
-     * with the algorithm.
+     * 1. Old style key algorithm like RSA, DSA, EC, this method returns
+     *    DIGwithKEY.
+     * 2. New style signature algorithm in the form of HASHwithKEY, this
+     *    method returns DIGwithKEY. Please note this is not HASHwithKEY.
+     * 3. Modern signature algorithm like RSASSA-PSS and EdDSA, this method
+     *    returns the signature algorithm itself but ensures digAlgId is
+     *    compatible with the algorithm as described in RFC 4056 and 8419.
      *
      * @param digAlgId the digest algorithm
-     * @param encAlgId the encryption or signature algorithm
+     * @param encAlgId the encryption algorithm
      * @param directSign whether the signature is calculated on the content
      *                   directly. This makes difference for Ed448.
      */
     public static String makeSigAlg(AlgorithmId digAlgId, AlgorithmId encAlgId,
             boolean directSign) throws NoSuchAlgorithmException {
         String encAlg = encAlgId.getName();
-        if (encAlg.contains("with")) {
-            return encAlg;
-        }
         switch (encAlg) {
             case "RSASSA-PSS":
                 PSSParameterSpec spec = (PSSParameterSpec)
@@ -547,11 +548,16 @@ public class SignerInfo implements DerEncoder {
                 return encAlg;
             default:
                 String digAlg = digAlgId.getName();
+                String keyAlg = SignatureUtil.extractKeyAlgFromDwithE(encAlg);
+                if (keyAlg == null) {
+                    // The encAlg used to be only the key alg
+                    keyAlg = encAlg;
+                }
                 if (digAlg.startsWith("SHA-")) {
                     digAlg = "SHA" + digAlg.substring(4);
                 }
-                if (encAlg.equals("EC")) encAlg = "ECDSA";
+                if (keyAlg.equals("EC")) keyAlg = "ECDSA";
-                return digAlg + "with" + encAlg;
+                return digAlg + "with" + keyAlg;
         }
     }
 

src/java.base/share/classes/sun/security/util/SignatureUtil.java

@@ -282,6 +282,32 @@ public class SignatureUtil {
         }
     }
 
+    /**
+     * Extracts the key algorithm name from a signature
+     * algorithm name in either the "DIGESTwithENCRYPTION" or the
+     * "DIGESTwithENCRYPTIONandWHATEVER" format.
+     *
+     * @return the key algorithm name, or null if the input
+     *      is not in either of the formats.
+     */
+    public static String extractKeyAlgFromDwithE(String signatureAlgorithm) {
+        signatureAlgorithm = signatureAlgorithm.toUpperCase(Locale.ENGLISH);
+        int with = signatureAlgorithm.indexOf("WITH");
+        String keyAlgorithm = null;
+        if (with > 0) {
+            int and = signatureAlgorithm.indexOf("AND", with + 4);
+            if (and > 0) {
+                keyAlgorithm = signatureAlgorithm.substring(with + 4, and);
+            } else {
+                keyAlgorithm = signatureAlgorithm.substring(with + 4);
+            }
+            if (keyAlgorithm.equalsIgnoreCase("ECDSA")) {
+                keyAlgorithm = "EC";
+            }
+        }
+        return keyAlgorithm;
+    }
+
     /**
      * Returns default AlgorithmParameterSpec for a key used in a signature.
      * This is only useful for RSASSA-PSS now, which is the only algorithm

test/jdk/sun/security/pkcs/pkcs7/TwoHash.java

@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8255494
+ * @summary Make sure the signature algorithm to verify a PKCS7 block is
+ *      DIGwithENC instead of HASHwithENC.
+ * @modules java.base/sun.security.pkcs
+ *          java.base/sun.security.tools.keytool
+ *          java.base/sun.security.x509
+ */
+
+import sun.security.pkcs.PKCS7;
+import sun.security.tools.keytool.CertAndKeyGen;
+import sun.security.x509.X500Name;
+
+import java.nio.charset.StandardCharsets;
+import java.security.cert.X509Certificate;
+
+public class TwoHash {
+    public static void main(String[] args) throws Exception {
+
+        byte[] content = "Hello You fool I love you".getBytes();
+
+        CertAndKeyGen cak = new CertAndKeyGen("EC", "SHA512withECDSA");
+        cak.generate("secp256r1");
+        byte[] signature = PKCS7.generateNewSignedData(
+                "SHA256withECDSA",
+                null,
+                cak.getPrivateKey(),
+                new X509Certificate[] {cak.getSelfCertificate(new X500Name("CN=Me"), 1000)},
+                content,
+                false,
+                true, // direct sign, so that RFC 6211 check is not possible
+                null);
+
+        // The original signature should verify.
+        if (new PKCS7(signature).verify(content) == null) {
+            throw new RuntimeException("Should be verified");
+        }
+
+        // Modify the SHA256withECDSA signature algorithm (OID encoded as
+        // "06 08 2A 86 48 CE 3D 04 03 02") to SHA384withECDSA (OID encoded as
+        // "06 08 2A 86 48 CE 3D 04 03 03"). ISO_8859_1 charset is chosen
+        // because it's a strictly one byte per char encoding.
+        String s = new String(signature, StandardCharsets.ISO_8859_1);
+        String s1 = s.replace(
+                "\u0006\u0008\u002A\u0086\u0048\u00CE\u003D\u0004\u0003\u0002",
+                "\u0006\u0008\u002A\u0086\u0048\u00CE\u003D\u0004\u0003\u0003");
+        byte[] modified = s1.getBytes(StandardCharsets.ISO_8859_1);
+
+        // The modified signature should still verify because the HASH
+        // part of signature algorithm is ignored.
+        if (new PKCS7(modified).verify(content) == null) {
+            throw new RuntimeException("Should be verified");
+        }
+    }
+}