Use of deprecated sha1

[nicopal]

Hi,

in the pkcs7 implementation, function NewSignedData in sign.go defaults to using SHA-1 as digest algorithm.
https://github.com/smallstep/pkcs7/blob/5e2c6a136dfaa418340bb4a7eb0d0c7421d4934c/sign.go

// NewSignedData takes data and initializes a PKCS7 SignedData struct that is
// ready to be signed via AddSigner. The digest algorithm is set to SHA1 by default
// and can be changed by calling SetDigestAlgorithm.
func NewSignedData(data []byte) (*SignedData, error) {
	content, err := asn1.Marshal(data)
	if err != nil {
		return nil, err
	}
	ci := contentInfo{
		ContentType: OIDData,
		Content:     asn1.RawValue{Class: 2, Tag: 0, Bytes: content, IsCompound: true},
	}
	sd := signedData{
		ContentInfo: ci,
		Version:     1,
	}
	return &SignedData{sd: sd, data: data, digestOid: OIDDigestAlgorithmSHA1}, nil
}

However, the use of sha1 is unadvisable according to NIST, and implementations should migrate to SHA-2 or SHA-3 as soon as possible (source: https://www.nist.gov/news-events/news/2022/12/nist-retires-sha-1-cryptographic-algorithm)

Is there a good reason this implementation defaults to SHA-1? Alternatively, would you accept a PR updating the default to SHA-2?

[hslatman]

Hi @nicopal, thank you for opening this issue. The reason it currently (still) defaults to using SHA1 is for backwards compatibility. If we were to change the default algorithm now, it's possible that systems importing the package do no longer work as a whole.

The current API for providing this type of setting in pkcs7 is lacking, but it might be possible to do something using a package level global, so that it doesn't break systems, and is an intentional change on the importer side. A package level global is not ideal though, and we've been thinking about more improvements along the lines of default algorithms and potentially backwards incompatible changes, so that'll happen at some point.

It seems possible to change it using SetDigestAlgorithm. Maybe we can expose a package global, similar to ContentEncryptionAlgorithm, so that the default can be changed that way, instead of having to call SetDigestAlgorithm every time you're using NewSignedData. Ideally the NewSignedData would take it as configuration instead of relying on a package level variable, but that would be a backwards incompatible change.

[nicopal]

Hi @hslatman thank you for looking into this. I suspected backward compatibility was the culprit (it's common). My initial thought was that perhaps NewSignedData could still support SHA1 but at least not default to it. However you have a better view of the constraints/requirements that others have towards this library and exposing the package global so that the default of SetDigestAlgorithm can be changed seems like a good solution.

[manistal]

I've opened a PR to swap the hard-coded default here:
#44

Using SHA1 as a digest is going to start breaking across environments, OpenSSL 3.4 FIPS Provider forbids it, Golang forbids it without a environment variable - Using a hard-coded default to an unsupported algorithm combination does not make sense.

My recommendation is take the easy approach here and just update the default to a SAFE default. If people need to override it, the SetDigestAlgorithm API already exists.

[manistal]

If you feel really strongly about this default, your idea to expose it as a package global would probably be fine and meet most requirements

That said, it's not really clear to me why this is a backwards compatibility issue - These are new signatures, and I cannot find any libraries that do not support RSA with SHA2-256 updated past 2005. I understand the general concern about changing defaults, but you could also go the Go STL direction and use a build tag to revert it - unsafe defaults is generally bad practice for cryptography.

[nicopal]

Good to see a change to a safe default, I hope this PR goes through soon.

[hslatman]

The PR won't go through as-is, that's for sure. We still support many older versions of Go, and SHA1 during verification is supported for those. Also note that PKCS7 (and CMS) are often used in environments where SHA1 is still in use for verifiction, so we can't just switch it in a minor version, in my opinion. It would mostly be a "silent" breaking change for the system as a whole: if you didn't know about the change, and just update the package (either directly, or transitively), the behavior of the relying system would change. There would be no indication in the form of CI breaking on a new argument, or something like that. Only when someone tests for a SHA1 signature explicitly, would it be uncovered during testing.

It is a possibility to do it for Go version 1.24 and up with e.g. //go:build go.24, because the GODEBUG x509sha1 isn't supported from Go 1.24 onwards, so it is not unexpected that it won't work in this package (in its current form) if that version is being used.

Also note that there's already a PR that allows to set the different default algorithm: #29. True, it does need the user to set it, but it does make things simpler if you rely on this package, since you only have to do it once in your program.

It is absolutely being considered for a v2.

[manistal]

@hslatman I just pushed a new commit with some compromise here

This allows us to set a build tag for a safe default, but the normal defaults are fully unchanged. It also exposes it as a package global to set it through multiple dependencies.

Let me know what you think - I like the simplicity here, and the build tag proposed aligns with the Microsoft FIPS Golang build tag so chainguard customers/msft-go customers would benefit for free.

[hslatman]

Yeah, that looks better already. I'm curious if we can (also) rely on the native Go FIPS build tags that were introduced with 1.24, next to requirefips? I'll need a bit of time to digest (hehe) the options, and to settle on a good path forward that keeps things maintainable too.

Note that we probably want the default algorithm setter to look like the one from #29 to be extra safe in highly-concurrent operations (but to be honest, there can still be issues with that approach), so I'll see if I can expedite that.

[manistal]

@hslatman The new go fips build tags are under GODEBUG which is not intended to be used by downstream libs

I understand the async concern, but this lib already has 3 package globals, I think for now, itd be better to solve the safe defaults problem, then in a future PR refactor the defaults to get mutex'ed via a setter. I dont think it makes sense to do it differently for this one new package global