sha1 removal (entirely or at least as default) ?

[lsm5]

Hello, I notice sha1 is used as the default in quite a few places in this repo. Are there are any plans to switch to sha2? I would be interested in doing a PR if you think it's worth it.

[lumjjb]

@lsm5 I don't see why not, that's a good idea! Still can change it based on env variable, so should be fine.

(unless sha1 is still needed for some HSM support - @stefanberger knows better).

[stefanberger]

pkcs11 may need it as a default: https://github.com/containers/ocicrypt/blob/main/crypto/pkcs11/pkcs11helpers.go#L44

[rhatdan]

sha1 is being dropped by fedora 37 as a supported cryto.

[stefanberger]

sha1 is being dropped by fedora 37 as a supported cryto.

And what happens to algorithms that still need it because they used it 'before', such as OAEP padding? Does the data become inaccessible?

[rhatdan]

That I do not know.

[stefanberger]

For as long as I don't have to deal with the fallout of that 'simple removal' ...

[lsm5]

For as long as I don't have to deal with the fallout of that 'simple removal' ...

would it be feasible to have a separate maintenance branch that has the current sha1 default but have main with sha2 as default?

[stefanberger]

would it be feasible to have a separate maintenance branch that has the current sha1 default but have main with sha2 as default?

Who will build two versions of their software with different default hashes? If sha1 is disabled on the host platform (disabled in OpenSSL and possibly other libraries) and other software needs it, that software and its users are out of luck -- or at least I don't know what the consequences are for 'old data'. Also, I am not sure what pkcs11 is funneling the hashes into in the golang and golang pkcs11 case and the softhsm case for example. Softhsm is using openssl afaik but I am not sure what path golang is using and whether it matters for the pkcs11 case. Just dropping the support from the underlying libraries is a bit harsh and I can only 'hope' for 'no fallout'.

Anyway, we may be able to change the default for the pkcs11 case but have to be careful. There's a comment in the code here that is wrong. I think the default is 'sha1' and hash in the JSON in the sha1 case is empty, not in the sha256 case.:

https://github.com/containers/ocicrypt/blob/v1.1.3/crypto/pkcs11/pkcs11helpers.go#L439

OAEP padding for decryption needs sha1 when it was originally created with sha1 otherwise it will not decrypt.

This occurrence of OAEPDefaultHash has to be replaced by '"sha1"'

        if hashalg == OAEPDefaultHash {
            hashalg = ""
        }

-->

                /* historic default is sha1 */
        if hashalg == "sha1" {
            hashalg = ""
        }

Then it may be possible to replace OAEPDefaultHash with "sha256":

    OAEPDefaultHash = "sha256"

[stefanberger]

When I try to run the test case with sha256:

diff --git a/crypto/pkcs11/pkcs11helpers_test.go b/crypto/pkcs11/pkcs11helpers_test.go
index ac8b9e7..50f5090 100644
--- a/crypto/pkcs11/pkcs11helpers_test.go
+++ b/crypto/pkcs11/pkcs11helpers_test.go
@@ -185,7 +185,7 @@ func TestPkcs11EncryptDecryptPubkey(t *testing.T) {

        testinput := "Hello World!"

-       os.Setenv("OCICRYPT_OAEP_HASHALG", "sha1")
+       os.Setenv("OCICRYPT_OAEP_HASHALG", "sha256")

        pubKeys := make([]interface{}, 1)
        pubKeys[0] = rsapubkey

The test case doesn't work with softhsm due to this here:

https://github.com/opendnssec/SoftHSMv2/blob/7f99bedae002f0dd04ceeb8d86d59fc4a68a69a0/src/lib/SoftHSM.cpp#L3123-L3127

[stefanberger]

I sent PR #61 to convert the pkcs11 code to default to sha256. I will have to drop the patch converting the test case due to the issue of SoftHSM not supporting anything else than sha1...

[stefanberger]

Can we close this issue now?

[lsm5]

Thanks @stefanberger @lumjjb