Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMS: Add support for sha-1WithRSAEncryption *digest* algorithm + aliases. #1500

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kevinhwang
Copy link

@kevinhwang kevinhwang commented Oct 2, 2023

This adds support to the CMS libraries for the sha-1WithRSAEncryption digest algorithm (OID 1.3.14.3.2.29) and its aliases. This is not to be confused with the SHA1withRSA signature algorithm which is currently already supported.

Per RFC 3279 2.2.1 and RFC 8017 A.2.4, this digest algorithm computes the SHA-1 digest on the encoded DigestInfo whose digestAlgorithm field is set to the id-sha1 OID.

Bouncy Castle doesn't currently recognize this digest algorithm (either for building CMSSignedData or verifying) and fails with

org.bouncycastle.cms.CMSException: can't create digest calculator: exception on setup: java.security.NoSuchAlgorithmException: no such algorithm: 1.3.14.3.2.29 for provider BC

In versions 1.69 and earlier, recognition for OID 1.3.14.3.2.29 in OperatorHelper.java was present, but the data encoding was not performed correctly (setting OID 1.3.14.3.2.29 during the data encoding step, rather than OID 1.3.14.3.2.26 as RFC 3279 requires), so the computed digest would be wrong and not verify.

@kevinhwang kevinhwang force-pushed the sha-1WithRSAEncryption branch 5 times, most recently from dad4e9a to 73c4b78 Compare October 3, 2023 15:35
@kevinhwang kevinhwang changed the title CMS: Add support for sha-1WithRSAEncryption *digest* algorithm. CMS: Add support for sha-1WithRSAEncryption *digest* algorithm + aliases. Oct 3, 2023
@dghgit
Copy link
Contributor

dghgit commented Oct 17, 2023

I've had a chance to have a quick look at this, essentially we'd be adding support for a specific kind of broken CMS signed data message.

sha-1WithRSAEncryption is a signature algorithm, the reason for the nomenclature lies in the original drafts of PKCS#7 where the algorithm used for processing the hash was referred to an an "encryption algorithm" since the process of signing with RSA is the same as "encrypting with the private key". Neither of the OIDs being added to map to SHA-1 are, or ever have been, considered to be identifying the SHA-1 hash algorithm, both RFCs are explicit about requiring 1.3.14.3.2.26 as being the OID used to represent SHA-1. The move to calling it a signatureAlgorithm was a result of other signing algorithms being required which made it obvious that the RSA centric view of the universe was not always correct.

It doesn't look like this would present a security issue, but I would prefer to keep the number of non-compliant OIDs to a minimum. Are you saying that you have message where both the OIW and PKCS definitions for SHA-1withRSA are used to represent the SHA-1 OID? Or is it just one variant that is present (I'd guess the OIW arc) and the other is given "for completeness"?

@kevinhwang
Copy link
Author

kevinhwang commented Oct 17, 2023

Are you saying that you have message where both the OIW and PKCS definitions for SHA-1withRSA are used to represent the SHA-1 OID

Yup, unfortunately we have clients sending CMS messages where the the digest algorithm OID (what BouncyCastle's CMS library calls the "ContentDigestAlgorithm") is sha-1WithRSAEncryption, even though they really should be setting it as SHA1 (because that's what it really is, they're just using a weird name / alias for it), and we're trying to support processing and verifying these messages.

Apparently googling around there are other (non-Java) pkcs7 implementations that run into this so provide support for these aliases.

@dghgit
Copy link
Contributor

dghgit commented Oct 18, 2023

We'd be better off if people just did it correctly. In some situations it is actually possible to weaken the security of the message when issues like this have to be dealt with. One last question, do they use sha-1WithRSAEncryption as the encryption algorithm in the signed data message or is it just rsaEncryption?

@kevinhwang
Copy link
Author

kevinhwang commented Oct 18, 2023

One last question, do they use sha-1WithRSAEncryption as the encryption algorithm in the signed data message or is it just rsaEncryption?

Do you mean the signature algorithm? They appear to use rsaEncryption as the identifier. Here's an example of a partial ASN.1 dump (just the metadata, actual data content redacted):

image

@dghgit
Copy link
Contributor

dghgit commented Oct 22, 2023

So I've pushed an update to SignerInformation.java which should deal with this - in a limited fashion. Is it possible to provide a sample of one of these messages?

@kevinhwang
Copy link
Author

kevinhwang commented Oct 23, 2023

So I've pushed an update to SignerInformation.java which should deal with this - in a limited fashion.

Thanks! It does seem like it allows decoding and verification if CMS messages using the weird digest algo name. This does seem like it'll allow us to handle these messages being sent.

Unfortunately, it still doesn't allow you to sign messages using the OID, which would be helpful for writing code for tests. :)

Trying to use it with CMSSignedDataGenerator / JcaSignerInfoGeneratorBuilder still gives you:

org.bouncycastle.operator.OperatorCreationException: exception on setup: java.security.NoSuchAlgorithmException: no such algorithm: 1.3.14.3.2.29 for provider BC

Is it possible in addition for the changes you already made for you to pull in the digestOidMap changes to MessageDigestUtils.java? That allows the generators to resolve the alias correctly.

Is it possible to provide a sample of one of these messages?

Here's an example of a random CMS message using this:

-----BEGIN PKCS7-----
MIAGCSqGSIb3DQEHAqCAMIACAQExCTAHBgUrDgMCHTCABgkqhkiG9w0BBwGggCSA
BBNNeSBzcGVjaWFsIG1lc3NhZ2UyAAAAAAAAoIAwggSZMIIDTaADAgECAhQAoX1E
JJLxftRRUu06RXzDwMewAzBBBgkqhkiG9w0BAQowNKAPMA0GCWCGSAFlAwQCAQUA
oRwwGgYJKoZIhvcNAQEIMA0GCWCGSAFlAwQCAQUAogMCASAwSDELMAkGA1UEBhMC
VVMxEzARBgNVBAoTCkdvb2dsZSBMTEMxJDAiBgNVBAMTG0dFTS1XaW5kb3dzLXVz
LXdlc3QxLUdZSm8yNTAeFw0yMzEwMTcyMTU2MjlaFw0yNDAxMjUyMTU2MjhaMBQx
EjAQBgNVBAMMCWRldmljZV9pZDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoC
ggEBALGMmPlHHlrXysf43O6cKbDta0zmo5bWwCzir78rkfyUsSdBDC+GLRhq2bxr
AW8nYn6wDy0a2r2QMoj9SS4my38o2Urtu+XtsQHu0NG+hKxm7KLSRncpvqcfFc6A
FQ8eYbbkYRVRJm5r7jsYWE9IB7Kv46rUWofieuYHdDyPvKU3unmxlegKFDWcFVmo
cx4BiMGZ5zv+BtLfHxCRTf5k9r1y13gmTnwZzI+iwc9ZixTHJD6OejvvEcbjqPUm
q9OhBrx/eUZqDCjHoBcjtYuj24RAhZu55/GP0xQedvZfasEA70XLD5wkF1oM/vQl
Cu9STP0en4y75YMefDjRmygpVdcCAwEAAaOCAUUwggFBMA4GA1UdDwEB/wQEAwID
uDATBgNVHSUEDDAKBggrBgEFBQcDAjAMBgNVHRMBAf8EAjAAMB0GA1UdDgQWBBQ7
eoXi4IwFUvD++kLjKcWIbZM+tDAfBgNVHSMEGDAWgBTs9EezkU1W31lKwtOxtGEI
RaJSuTCBjQYIKwYBBQUHAQEEgYAwfjB8BggrBgEFBQcwAoZwaHR0cDovL3ByaXZh
dGVjYS1jb250ZW50LTY1MGE2MmE2LTAwMDAtMmU3Mi1hZGY1LWQ0ZjU0N2ZhY2Fh
MC5zdG9yYWdlLmdvb2dsZWFwaXMuY29tLzZiYjBlZmIwZjk5MmZmNmZkYTFkL2Nh
LmNydDA8BgNVHREENTAzhjFkYXRhOkNnbGtaWFpwWTJWZmFXUWdxK09QeVBvU09n
WUlnT0xKckFaQWdhK05BZz09MEEGCSqGSIb3DQEBCjA0oA8wDQYJYIZIAWUDBAIB
BQChHDAaBgkqhkiG9w0BAQgwDQYJYIZIAWUDBAIBBQCiAwIBIAOCAQEAroRz0SiP
2IMkwcZ0ffz/d4Z9RE9x7lDve2hsmwKqYnKzYmiueVognNGfI3zp613+NJN73Au4
qeCHUMmq+rik2m2qMy/1PqzUHthqpGONXfckw18+XpTqmSsBm+wGBvfjFAFi1e/Q
x64ABjLyGfws9/fiZgOkCc/ewDo+BI2S8vaYMRTVhKtfLZQODXTLQlalFX2JM0B6
KYOfOmfmNM/W0FQLOetbprSqCsA2Di2e3/obnh0N+ft2q3Ayvh9dEPNOnp0W1hAD
VAHhoq4HQKCsaJuc93pjIDgoFnFg5jBeKq2ZUDJc32N21iEPK2ylrUVnUVZx87wK
laDjdqbmrN0R2gAAMYIBhTCCAYECAQEwYDBIMQswCQYDVQQGEwJVUzETMBEGA1UE
ChMKR29vZ2xlIExMQzEkMCIGA1UEAxMbR0VNLVdpbmRvd3MtdXMtd2VzdDEtR1lK
bzI1AhQAoX1EJJLxftRRUu06RXzDwMewAzAHBgUrDgMCHTANBgkqhkiG9w0BAQEF
AASCAQBIB6dz25EcS9/UpTMp0zX3W9dGdlsJ+uoq6vqpHnpSk7D3aEh/AYG7rZse
g2RG6eB18ZaC2Z//5tIYFCHEl0uE8IFXlRYyTeYZew0beos9SuyP3Opounzt011y
mBAYzbDmNA8C7KEvogQoCUE0WE4Er7jSUp086mCzywyLKfaU9sVpwpDFQs5g77IT
KlFWXY3oulQJRGJAC8x0nSqGZRuSfc/I6nH4MTmxHg1gImFlcejNkiMXHMOz9qky
0u1ZKG4NPZtDnPMV6hR03vyyieDG9UgOldfzuI8gyOc8MQ/INVIW63p5BU3/sDqB
417wi5DHOj7Yp/ZuWeC6LBflQu+vAAAAAAAA
-----END PKCS7-----

…t algorithm.

Per RFC 3279 / RFC 8017, this digest algorithm computes the SHA-1 digest on the encoded `DigestInfo` whose `digestAlgorithm` is set to the `id-sha1` OID.
@dghgit
Copy link
Contributor

dghgit commented Oct 23, 2023

Thanks for the sample. With hindsight, I've got mixed feelings about the fact it works, I note it's using signed attributes, I guess since you've generated it locally - but for that particular use case it does mean we're weakening an already weak algorithm further.

With further changes, I hate break bad news, but the generator is resolving the alias correctly - producing messages like this should not be encouraged (I'll leave the idea of using SHA-1 in general now for another discussion...).

In the (hopefully) unlikely event you're actually supposed to send broken messages to who ever was responsible for this in the first place, one interesting difference between the generated sample and the message the dump was provided of early, is the original message used a raw signature and no signing attributes. You can actually create one of these using

JcaJcaSimpleSignerInfoGeneratorBuilder.setDirectSignature()

and then doing (for the simplest case):

    SignedData sd = SignedData.getInstance(ContentInfo.getInstance(s.toASN1Structure()).getContent());
    SignerInfo sInfo = SignerInfo.getInstance(sd.getSignerInfos().getObjectAt(0));
    sInfo = new SignerInfo(sInfo.getSID(), new AlgorithmIdentifier(OIWObjectIdentifiers.sha1WithRSA, DERNull.INSTANCE), sInfo.getAuthenticatedAttributes(), sInfo.getDigestEncryptionAlgorithm(), sInfo.getEncryptedDigest(), sInfo.getUnauthenticatedAttributes());

    sd = new SignedData(new DERSet(new AlgorithmIdentifier(OIWObjectIdentifiers.sha1WithRSA, DERNull.INSTANCE)),   sd.getEncapContentInfo(), sd.getCertificates(), sd.getCRLs(), new DERSet(sInfo));

    s = new CMSSignedData(new ContentInfo(PKCSObjectIdentifiers.signedData, sd));

One further question, are these message types always direct signatures? If possible, I would rather go back to failing for signatures using signed attributes, people having to rely on SHA-1 based signatures already have enough issues without us further compromising the security of the signature.

@kevinhwang
Copy link
Author

kevinhwang commented Oct 23, 2023

I note it's using signed attributes, I guess since you've generated it locally

Ah you're right, I generated it locally with signed attributes. I wanted to avoid posting a real example of messages we're seeing because of PII.

one interesting difference between the generated sample and the message the dump was provided of early, is the original message used a raw signature and no signing attributes. You can actually create one of these using

Ah you're right, using direct signature works:

val cert: X509Certificate = // ...
val privateKey: PrivateKey = // ...

val contentSigner: ContentSigner =
  JcaContentSignerBuilder("SHA1WithRSAEncryption")
    .setProvider(BouncyCastleProvider())
    .build(privateKey)
val content: CMSTypedData = CMSProcessableByteArray("My message".toByteArray())
val signedData: CMSSignedData =
  CMSSignedDataGenerator()
    .apply {
      addSignerInfoGenerator(
        JcaSignerInfoGeneratorBuilder(
          JcaDigestCalculatorProviderBuilder().setProvider(BouncyCastleProvider()).build()
        )
          .setDirectSignature(true)
          .setContentDigest(AlgorithmIdentifier(OIWObjectIdentifiers.sha1WithRSA))
          .build(contentSigner, cert)
      )
      addCertificates(JcaCertStore(setOf(cert)))
    }
    .generate(content, true)

I updated my example in my comment above to be a CMS message using direct signature w/ the sha1WithRsa digest algo alias.

One further question, are these message types always direct signatures?

I've only looked at a few samples of CMS messages we get, but of the ones I've seen they don't seem to have signed attributes. Still it's hard to be sure that no client would send them...

vanitasvitae pushed a commit to pgpainless/bc-java that referenced this pull request Jan 2, 2024
vanitasvitae pushed a commit to pgpainless/bc-java that referenced this pull request Jan 2, 2024
added check for signed attributes on dodgey SHA-1 OID relates to github bcgit#1500
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants