[Bug]: Legacy DES-CBC cipher is used for encryption of PKCS#7 envelopes

stv0g commented 2 months ago

Steps to Reproduce

Use sscep to enroll a certificate against scepserver.

Your Environment

OS - Linux
Version - master

Expected Behavior

AES cipher is used for envelope encryption

Actual Behavior

OpenSSL errors out due to legacy cipher:

sscep: error decrypting inner PKCS#7
40379131607F0000:error:0308010C:digital envelope routines:inner_evp_generic_fetch:unsupported:crypto/evp/evp_fetch.c:386:Global default library context, Algorithm (DES-CBC : 11), Properties ()
40379131607F0000:error:10800077:PKCS7 routines:PKCS7_decrypt:decrypt error:crypto/pkcs7/pk7_smime.c:518:

Additional Context

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

hslatman commented 2 months ago

Hi @stv0g,

I agree DES-CBC shouldn't be the default these days, but changing the default in this package is a backwards incompatible behavioral change for users of the package that might affect their system. Also, since its part of the design of the pkcs7 package, it'll affect all future operations once its set, because the pkcs7.ContentEncryptionAlgorithm variable is used as global state. It could thus even be set to something different if another part of your (or other's library) code interacted with it at another time.

Changing the behavior in this package would require at least a v2, in my opinion. We've also already discussed internally changing the default in pkcs7, but that would be a breaking change too. We're not opposed to that at all, but we want to make it do other things better too, and it takes time to do it well.

In step-ca, the main place we're using scep, and thus pkcs7, we've encapsulated the encryption operation to set (and reset) the current algorithm after its operation: https://github.com/smallstep/certificates/blob/442be8da1cd97336b3636f1a134823f7181c172b/scep/authority.go#L397-L417. It's not perfect, but it works for the most part. I suggest a similar solution could be implemented in scepserver from https://github.com/micromdm/scep. Instead of doing it per request, a config option or flag to set it for all requests could be sufficient there.

stv0g commented 2 months ago

Okay, I agree.

I think we would need a v2 of that module with an Encrypt() API which accepts the cipher as an argument.

Maybe this issue should be moved to step/pkcs7 instead?

hslatman commented 2 months ago

Yes, opening an issue in https://github.com/smallstep/pkcs7 sounds OK. In due time I'll likely add a kind of meta issue to track more changes that are not backwards compatible, and add it to that too.

stv0g commented 1 month ago

Closed in favour of https://github.com/smallstep/pkcs7/issues/30

smallstep / scep