search

kaikramer / keystore-explorer

KeyStore Explorer is a free GUI replacement for the Java command-line utilities keytool and jarsigner.
https://keystore-explorer.org/
GNU General Public License v3.0
1.7k stars 275 forks source link

Signature algorithm for signed JAR reported incorrectly #293

Closed pbeast closed 2 years ago

pbeast commented 3 years ago

The reported signature algorithm for JARs signed using KSE is inaccurate

To Reproduce

  1. Sign JAR file using KSE
  2. Verify the signature using jarsigner -verify target/test.jar -verbose:summary

  3. Check the output:

    

      0 Mon Aug 23 20:29:28 IDT 2021 META-INF/ (and 17 more)
sm     5871 Mon Aug 23 20:29:28 IDT 2021 org/springframework/boot/loader/ClassPathIndexFile.class (and 100 more)
s     14598 Mon Aug 23 20:29:28 IDT 2021 META-INF/MANIFEST.MF
  13290 Mon Aug 23 20:29:28 IDT 2021 META-INF/NCR_CORPORATION_S_DIGICERT_INC_ID.SF (and 1 more)

s = signature was verified
m = entry is listed in manifest
k = at least one certificate was found in keystore

jar verified.

The signer certificate will expire on 2022-04-14. The timestamp will expire on 2031-01-06.


**Expected behavior**
      0 Mon Aug 23 20:29:28 IDT 2021 META-INF/ (and 17 more)

sm 5871 Mon Aug 23 20:29:28 IDT 2021 org/springframework/boot/loader/ClassPathIndexFile.class (and 100 more) s 14598 Mon Aug 23 20:29:28 IDT 2021 META-INF/MANIFEST.MF 13290 Mon Aug 23 20:29:28 IDT 2021 META-INF/NCR_CORPORATION_S_DIGICERT_INC_ID.SF (and 1 more)

s = signature was verified m = entry is listed in manifest k = at least one certificate was found in keystore

jar verified.

The signer certificate will expire on 2022-04-14. The timestamp will expire on 2031-01-06.



The signature algorithm reported as ```SHA256withSHA256withRSA``` instead of ```SHA256withRSA```

Link to the sample JAR - https://drive.google.com/file/d/1ppfDbJwr0wZ7GFSScGRiT78DKsdQMAFi/view?usp=sharing

**Environment**

-  Version of KSE: 5.4.4 (built from sources including latest changes)

-  Version of Java:
   openjdk version "14.0.2" 2020-07-14
   OpenJDK Runtime Environment AdoptOpenJDK (build 14.0.2+12)
   OpenJDK 64-Bit Server VM AdoptOpenJDK (build 14.0.2+12, mixed mode, sharing)
- Platform (OS): macOS Big Sur
kaikramer commented 3 years ago

Hmm, another strange bug... I have checked the signature file (META-INF\NCR_CORPORATION_S_DIGICERT_INC_ID.RSA), which is in PKCS#7/CMS (RFC 5652) format and everything seems fine. The signature has OID 1.2.840.113549.1.1.11, which is correct for SHA256WithRSA.

I am using Java 15 and jarsigner shows SHA256withSHA256withRSA as well. To be honest, I currently have no idea why this is happening...

pbeast commented 3 years ago

Yea, meanwhile, I don't have any idea too. It looks like the jarsigner reads the signature encryption algorithm as SHA256WithRSA instead of just an RSA. Then, it adds the digest algorithm (SHA256 + "With") and gets what we see. I will try to debug the jarsigner verify function to understand how it comes to such a conclusion.

If 1.2.840.113549.1.1.11 is reported as a signature encryption algorithm, that will make sense.

pbeast commented 3 years ago

Well, not that I already know how to fix that, but the responsible code (in jarsigner) is following:

SignerInfo si = p7.getSignerInfos()[0];
X509Certificate signer = si.getCertificate(p7);
String digestAlg = digestMap.get(s);
String sigAlg = AlgorithmId.makeSigAlg(
        si.getDigestAlgorithmId().getName(),
        si.getDigestEncryptionAlgorithmId().getName());

The si looks like:

image

And that is the problem - digestEncryptionAlgorithmId should be just RSA.

pbeast commented 3 years ago

If compiled with BC < 1.55 (i.e. 1.54) the issue is gone...

pbeast commented 3 years ago

Another step: Having PKCSObjectIdentifiers.rsaEncryption hardcoded the following do the job:

            JcaSignerInfoGeneratorBuilder siGeneratorBuilder = new JcaSignerInfoGeneratorBuilder(digCalcProv, new DefaultCMSSignatureEncryptionAlgorithmFinder() {
                @Override
                public AlgorithmIdentifier findEncryptionAlgorithm(AlgorithmIdentifier signatureAlgorithm) {
                    return new AlgorithmIdentifier(PKCSObjectIdentifiers.rsaEncryption, DERNull.INSTANCE);
                }
            });

The last step is to take the encryption algorithm identifier by the certificate PrivateKey

pbeast commented 3 years ago

I'm not so good with BC, so that's the best I can come with:

            JcaSignerInfoGeneratorBuilder siGeneratorBuilder = new JcaSignerInfoGeneratorBuilder(digCalcProv, new DefaultCMSSignatureEncryptionAlgorithmFinder() {
                @Override
                public AlgorithmIdentifier findEncryptionAlgorithm(AlgorithmIdentifier signatureAlgorithm) {
                    var shaRsaIdentifiers = new ASN1ObjectIdentifier[] {
                            PKCSObjectIdentifiers.sha256WithRSAEncryption,
                            PKCSObjectIdentifiers.sha384WithRSAEncryption,
                            PKCSObjectIdentifiers.sha512WithRSAEncryption
                    };
                    return Arrays.stream(shaRsaIdentifiers).noneMatch(ai -> ai == signatureAlgorithm.getAlgorithm()) ?
                            super.findEncryptionAlgorithm(signatureAlgorithm) :
                            new AlgorithmIdentifier(PKCSObjectIdentifiers.rsaEncryption, DERNull.INSTANCE);
                }
            });

Need your feedback :-)

kaikramer commented 3 years ago

Try jarsigner from a Java 16 JDK, it's fixed there:

- Signed by "CN=NCR Corporation, OU=NCR, O=NCR Corporation, L=Atlanta, ST=Georgia, C=US"
    Digest algorithm: SHA-256
    Signature algorithm: SHA256withRSA, 2048-bit key
pbeast commented 3 years ago

Interesting, what's the difference? From one side, I would stick with standard BC behavior, but I can't mandate others what SDK to use to check my binaries.

kaikramer commented 3 years ago

In Java 16 support for RSASSA-PSS and EdDSA was added to jarsigner. These signature schemes are more complicated than just combining a digest algorithm with raw RSA encryption.

pbeast commented 3 years ago

Is that mean my solution is appropriate? As it modifies signer info only for RSAs

kaikramer commented 3 years ago

Maybe it is... Not sure what BC actually does after that modification. Does it use the modified algorithm for signing or does it use the other algorithm and only write the modified OID into the CMS file?

What about SHA-1?

pbeast commented 3 years ago

The default implementation is actually a map for different cases. SHA1 is already there, along with EC variants. I'm not near the computer now, but I will post the default implementation here tomorrow. All three options I added were there before BC 1.55; then, they switched to the general approach (supported in Java SDK 16).

P.S. That information is for signer info generation. The signature itself does not affected.

kaikramer commented 3 years ago

I would assume that the mapping for SHA1withRSAEncryption in the default implementation leads to the same issue with jarsigner, so it should be part of your patch as well, right?

Hm, if the signature itself is not affected, shouldn't the verification of the signature fail then?

pbeast commented 3 years ago

I would assume that the mapping for SHA1withRSAEncryption in the default implementation leads to the same issue with jarsigner, so it should be part of your patch as well, right?

No, because SHA1withRSAEncryption is mapped to rsaEncryption.

Following is a current implementation of the DefaultCMSSignatureEncryptionAlgorithmFinder:

public class DefaultCMSSignatureEncryptionAlgorithmFinder
    implements CMSSignatureEncryptionAlgorithmFinder
{
    private static final Set RSA_PKCS1d5 = new HashSet();
    private static final Map GOST_ENC = new HashMap();

    static
    {
        RSA_PKCS1d5.add(PKCSObjectIdentifiers.md2WithRSAEncryption);
        RSA_PKCS1d5.add(PKCSObjectIdentifiers.md4WithRSAEncryption);
        RSA_PKCS1d5.add(PKCSObjectIdentifiers.md5WithRSAEncryption);
        RSA_PKCS1d5.add(PKCSObjectIdentifiers.sha1WithRSAEncryption);
        RSA_PKCS1d5.add(OIWObjectIdentifiers.md4WithRSAEncryption);
        RSA_PKCS1d5.add(OIWObjectIdentifiers.md4WithRSA);
        RSA_PKCS1d5.add(OIWObjectIdentifiers.md5WithRSA);
        RSA_PKCS1d5.add(OIWObjectIdentifiers.sha1WithRSA);
        RSA_PKCS1d5.add(TeleTrusTObjectIdentifiers.rsaSignatureWithripemd128);
        RSA_PKCS1d5.add(TeleTrusTObjectIdentifiers.rsaSignatureWithripemd160);
        RSA_PKCS1d5.add(TeleTrusTObjectIdentifiers.rsaSignatureWithripemd256);

        GOST_ENC.put(CryptoProObjectIdentifiers.gostR3411_94_with_gostR3410_2001,
            new AlgorithmIdentifier(CryptoProObjectIdentifiers.gostR3410_2001, DERNull.INSTANCE));
        GOST_ENC.put(RosstandartObjectIdentifiers.id_tc26_signwithdigest_gost_3410_12_256,
            new AlgorithmIdentifier(RosstandartObjectIdentifiers.id_tc26_gost_3410_12_256, DERNull.INSTANCE));
        GOST_ENC.put(RosstandartObjectIdentifiers.id_tc26_signwithdigest_gost_3410_12_512,
            new AlgorithmIdentifier(RosstandartObjectIdentifiers.id_tc26_gost_3410_12_512, DERNull.INSTANCE));
    }

    public AlgorithmIdentifier findEncryptionAlgorithm(AlgorithmIdentifier signatureAlgorithm)
    {
               // RFC3370 section 3.2 with RFC 5754 update
        if (RSA_PKCS1d5.contains(signatureAlgorithm.getAlgorithm()))
        {
            return new AlgorithmIdentifier(PKCSObjectIdentifiers.rsaEncryption, DERNull.INSTANCE);
        }
        // GOST signature encryption algorithms
        if (GOST_ENC.containsKey(signatureAlgorithm.getAlgorithm()))
        {
            return (AlgorithmIdentifier)GOST_ENC.get(signatureAlgorithm.getAlgorithm());
        }
        return signatureAlgorithm;
    }
}

Note the RSA_PKCS1d5.add(OIWObjectIdentifiers.sha1WithRSA); That's how I came to the idea of additional mappings and then I found once it was there. Before BC v1.55 the RSA_PKCS1d5 set had similar entries for PKCSObjectIdentifiers.sha256WithRSAEncryption, PKCSObjectIdentifiers.sha384WithRSAEncryption, and PKCSObjectIdentifiers.sha512WithRSAEncryption

So, all modern ciphers are passed as is.

Hm, if the signature itself is not affected, shouldn't the verification of the signature fail then?

Well, we have two options:

  1. The embedded information is incorrect - the signing is done using SHA256 + RSA encryption, but the information provided to SignerInfo is incorrect
  2. The embedded information is correct - the signing is done by calculation of SHA256 of the digest calculated as SHA256 of the signature file

From the security perspective, SHA256WithSHA256WithRSA is absolutely OK, just leads to uncomfortable questions :-)

Can you please check what jarsigner from Java 16 SDK says about the file signed using the patch - https://drive.google.com/file/d/1CvjqmrOojqdu0K0pOy51ZSqXy_NKxkB0/view?usp=sharing

kaikramer commented 3 years ago

The linked jar file seems fine:

C:\Program Files\AdoptOpenJDK\jdk-16.0.1.9-hotspot\bin\jarsigner.exe -verify test-signed.jar -verbose:summary

          0 Thu Aug 26 13:35:56 CEST 2021 META-INF/ (and 17 more)
sm     5871 Fri Feb 01 00:00:00 CET 1980 org/springframework/boot/loader/ClassPathIndexFile.class (and 100 more)
s     14598 Thu Aug 26 13:39:22 CEST 2021 META-INF/MANIFEST.MF
      13266 Thu Aug 26 13:39:22 CEST 2021 META-INF/SIGNATURE.SF (and 1 more)

  s = signature was verified
  m = entry is listed in manifest
  k = at least one certificate was found in keystore

- Signed by "CN=NCR Corporation, OU=NCR, O=NCR Corporation, L=Atlanta, ST=Georgia, C=US"
    Digest algorithm: SHA-256
    Signature algorithm: SHA256withRSA, 2048-bit key
  Timestamped by "CN=DigiCert Timestamp 2021, O="DigiCert, Inc.", C=US" on Do. Aug. 26 10:39:23 UTC 2021
    Timestamp digest algorithm: SHA-256
    Timestamp signature algorithm: SHA256withRSA, 2048-bit key

jar verified.

The signer certificate will expire on 2022-04-14.
The timestamp will expire on 2031-01-06.
pbeast commented 3 years ago

Great Then I think that it's OK to use the patch.

kaikramer commented 3 years ago

From the security perspective, SHA256WithSHA256WithRSA is absolutely OK, just leads to uncomfortable questions :-)

Then you can explain now that the older jarsigner versions just display wrong information. If you sign a jar with Java 16 jarsigner and verify with any older version, it also shows SHA256WithSHA256WithRSA.

pbeast commented 3 years ago

But making that change will provide “backward compatibility.”
Java 16 is way far from the most distributed SDK.

pbeast commented 3 years ago

Also, while SHA256WithSHA256WithRSA is OK, that's wrong information in our case

kaikramer commented 3 years ago

Well, it's a workaround for a bug in jarsigner. If I accept this in KSE, I want to be 100% sure that it has no negative side effects. I am going on a vacation tomorrow and then I'll have some time to think about it.

pbeast commented 3 years ago

Have a nice vacation:-)

kaikramer commented 3 years ago

In the mean time I have done some more testing and also read the relevant parts of the RFCs and I see no reason anymore not to include your patch.

For the sake of traceability I am explaining here in this ticket why this patch is ok and link to it in the code.

RFC 3370 says:

CMS implementations that include the RSA (PKCS #1 v1.5) signature algorithm MUST support the rsaEncryption signature value algorithm identifier, and CMS implementations MAY support RSA (PKCS

1 v1.5) signature value algorithm identifiers that specify both the

RSA (PKCS #1 v1.5) signature algorithm and the message digest algorithm.

In RFC 5754 this was extended for SHA-2 algorithms.

The patch changes signatureAlgorithm in the SignerInfo from e.g. sha384WithRSAEncryption (1 2 840 113549 1 1 12) to rsaEncryption (1 2 840 113549 1 1 1) , but the digestAlgorithm also contains the hash algorithm, so for PKCS#1 v1.5 signatures all required infos to verify the signature is still there:

SignerInfo ::= SEQUENCE {
  version CMSVersion,
  sid SignerIdentifier,
  digestAlgorithm DigestAlgorithmIdentifier,
  signedAttrs [0] IMPLICIT SignedAttributes OPTIONAL,
  signatureAlgorithm SignatureAlgorithmIdentifier,
  signature SignatureValue,
  unsignedAttrs [1] IMPLICIT UnsignedAttributes OPTIONAL }

digestAlgorithm:

5CB2    D: . . . . . SEQUENCE {
5CB4    9: . . . . . . OBJECT IDENTIFIER sha-384 (2 16 840 1 101 3 4 2 2)
         : . . . . . . . (NIST Algorithm)
5CBF    0: . . . . . . NULL
         : . . . . . . }

signatureAlgorithm before patch:

5D3E    9: . . . . . . OBJECT IDENTIFIER
         : . . . . . . . sha384WithRSAEncryption (1 2 840 113549 1 1 12)
         : . . . . . . . (PKCS #1)
5D49    0: . . . . . . NULL
         : . . . . . . }

signatureAlgorithm after patch:

5D3E    9: . . . . . . OBJECT IDENTIFIER rsaEncryption (1 2 840 113549 1 1 1)
         : . . . . . . . (PKCS #1)
5D49    0: . . . . . . NULL
         : . . . . . . }
pbeast commented 3 years ago

Great news!

Glad to be helpful :-)

kaikramer commented 2 years ago

Closing tickets in preparation for release of KSE 5.5.0