crypto/rsa: refuse to generate and/or use keys smaller than 1024 bits

FiloSottile commented 1 month ago

crypto/rsa is unusual in that it can be secure (used with a key size > 2048) or completely insecure (used with a key size such as 512 bits, which can be broken on a laptop). Small keys are sometimes useful in tests, but having an rsa.PrivateKey value that behaves and looks exactly like a secure one but actually provides no security at all is a significant footgun.

In production, if a 512-bit key is used, it's overwhelmingly likely that the operator thinks they have security and doesn't (this happens in the real world on a regular basis) as opposed to being intentional about using fake RSA, so this feels like one of those rare occasions where breaking them is justified.

I propose we do both the following in Go 1.24:

return an error from rsa.GenerateKey if bits is less than 1024
return an error from all Sign, Verify, Encrypt, and Decrypt methods if the key is smaller than 1024 bits

GODEBUG=rsa1024min=0 reverts to the old behavior.

OpenSSL sounds on the way to doing (1) in a minor release and (2) in a major release. openssl/openssl#25092

To avoid slow key generation in tests, we can recommend using the test keys in RFC 9500. If anyone has a good idea for how to expose them, we can even provide them ready to use.

We could also disable the restriction based on testing.Testing() but I would keep that as a fallback if the amount of tests that require fixing turns out to be truly unmanageable, because generally tests are supposed to test actual behavior.

alex commented 1 month ago

For pyca/cryptography (most widely used Python cryptography library) in our last release (~3 weeks ago) we enforce a minimum of 1024-bit for RSA key generation (up from 512-bits in the previous release).

This has produced only one complaint, about a testing workload (where small keys were being used for performance).

We're also interested in enforcing key sizes in signing/verification (or perhaps in key loading), but haven't done so yet.

FiloSottile commented 1 month ago

To avoid slow key generation in tests, we can recommend using the test keys in RFC 9500. If anyone has a good idea for how to expose them, we can even provide them ready to use.

An easy answer to this is a GenerateKey or package-level example, for folks to copy-paste.

We could also disable the restriction based on testing.Testing() but I would keep that as a fallback if the amount of tests that require fixing turns out to be truly unmanageable, because generally tests are supposed to test actual behavior.

Actually, this is not a good idea. Packages can opt in to the equivalent behavior simply by doing

func init() {
    os.Setenv("GODEBUG", os.Getenv("GODEBUG") + ",rsa1024min=0")
}

in a test file (or an equivalent, test-scoped operation).

ericlagergren commented 1 month ago

We could also disable the restriction based on testing.Testing()

This also requires crypto/rsa to import testing, which isn't good.

rittneje commented 1 month ago

@FiloSottile Are you committing to supporting that GODEBUG forever? There are legitimate reasons to use smaller keys, and we shouldn't have to fork the Go standard library to work with them.

Also, I really don't think this should (only) be a GODEBUG. When smaller keys are intentional, it is going to be for some specific use case. Having this toggle for the entire application is too coarse grain. It would be far preferable to be able to opt-in per function/method call.

mcpherrinm commented 1 month ago

Do you have an examples of those legitimate use-cases? I admit they may exist but besides test-case reasons, I am not aware of why you’d want to use RSA < 1024. Even 1024 is a stretch these days for anything that requires actual security. I almost want 1024 gated behind a GODEBUG too

rittneje commented 1 month ago

@mcpherrinm We have some edge devices that can only hardware accelerate 256-bit. I thought they were RSA, but turns out they are elliptic (P-256), so we're good!

mcpherrinm commented 1 month ago

Great: P-256 elliptic curves have strength greater than RSA-2048 and are well within the margin of safety today.

Confusion about key strength for different algorithms is one of the common reasons for accidental use of RSA-512. For example, https://github.com/hashicorp/consul/issues/20112 defaulted to a key size of 256, which was suitable for P-256 but totally inappropriate if Consul was configured to use RSA but didn’t have a key size set.

This proposal would have prevented that issue.

FiloSottile commented 1 month ago

A recent example of a real world security incident due to production usage of RSA 512-bit keys.

https://rya.nc/vpp-hack.html

(This incident prompted this proposal, but it was not public at the time.)

rsc commented 1 month ago

os.Setenv("GODEBUG", os.Getenv("GODEBUG") + ",rsa1024min=0")

No need for this in a test. Tests can write a line at the top of the source file:

//go:debug rsa1024min=0

rsc commented 1 month ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

clausecker commented 1 month ago

There should be a way to still generate and use smaller keys, e.g. to interface with legacy embedded devices that require such. It would be kind of painful for such applications to have to use a separate RSA implementation just to sidestep the restriction.

mcpherrinm commented 1 month ago

Do you have an example of a device that requires RSA keys smaller than 1024 bits? What key sizes does it support?

ryancdotorg commented 1 month ago

Use these as test keys:

clausecker commented 1 month ago

Do you have an example of a device that requires RSA keys smaller than 1024 bits? What key sizes does it support?

For example, signing the firmware of TI calculators requires 512-bit RSA keys.

ryancdotorg commented 1 month ago

Signing TI calculator firmware requires special tools as it has a nonstandard signature format, so this isn't a good justification for supporting 512 bit RSA, and in any event requires neither key generation nor signature verification.

clausecker commented 1 month ago

You asked for an example. It just seems to me that gimping a cryptographical primitive like this with no good way out is not a particularly good idea.

ryancdotorg commented 1 month ago

It's been made clear repeatedly that providing support is a dangerously bad idea, and so there must be a reason that outweighs that in order to keep it.

ianlancetaylor commented 1 month ago

@clausecker It seems to me that the GODEBUG workaround is sufficient for that kind of example.

FiloSottile commented 1 month ago

I don't think we are committing to keeping the GODEBUG forever. We might, but I don't think we need to promise that now. If there are common non-testing use cases for crypto/rsa with broken keys, I'd like to hear about them to inform that decision. So far none have come up in this thread. (The TI firmware key story is fascinating, but apparently not something crypto/rsa is used for.)

Another option we have before permanent GODEBUG support is a third-party insecurersa package that provides GenerateKey, Sign, Verify, Encrypt, and Decrypt. Since we are not blocking unmarshaling and marshaling of weak keys, and thanks to the Signer interface, I think such a package could be used as a drop-in anywhere crypto/rsa is used.

clausecker commented 1 month ago

I don't think we are committing to keeping the GODEBUG forever. We might, but I don't think we need to promise that now. If there are common non-testing use cases for crypto/rsa with broken keys, I'd like to hear about them to inform that decision. So far none have come up in this thread. (The TI firmware key story is fascinating, but apparently not something crypto/rsa is used for.)

Another option we have before permanent GODEBUG support is a third-party insecurersa package that provides GenerateKey, Sign, Verify, Encrypt, and Decrypt. Since we are not blocking unmarshaling and marshaling of weak keys, and thanks to the Signer interface, I think such a package could be used as a drop-in anywhere crypto/rsa is used.

Another option could be to have a global policy variable to set the least key with accepted by the package. This would allow weak keys and permit applications to restrict the key size even further. On the other hand, global state is probably not a good design pattern here.

rsc commented 2 weeks ago

There has been some discussion here but probably not any compelling reasons not to change this as the default. Tools that care can set //go:debug lines in main. It doesn't seem like there are individual packages that will care.

In the future we could consider a crypto/insecurersa as a separate proposal if more motivating cases arose.

rsc commented 2 weeks ago

Have all remaining concerns about this proposal been addressed?

The proposal is to change crypto/rsa to reject <1024-bit RSA keys by default.

Return an error from rsa.GenerateKey if bits is less than 1024
Return an error from all Sign, Verify, Encrypt, and Decrypt methods if the key is smaller than 1024 bits

Setting GODEBUG=rsa1024min=0 reverts to the old behavior.

rsc commented 1 week ago

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

The proposal is to change crypto/rsa to reject <1024-bit RSA keys by default.

Return an error from rsa.GenerateKey if bits is less than 1024
Return an error from all Sign, Verify, Encrypt, and Decrypt methods if the key is smaller than 1024 bits

Setting GODEBUG=rsa1024min=0 reverts to the old behavior.

rsc commented 4 days ago

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

The proposal is to change crypto/rsa to reject <1024-bit RSA keys by default.

Return an error from rsa.GenerateKey if bits is less than 1024
Return an error from all Sign, Verify, Encrypt, and Decrypt methods if the key is smaller than 1024 bits

Setting GODEBUG=rsa1024min=0 reverts to the old behavior.

golang / go

crypto/rsa: refuse to generate and/or use keys smaller than 1024 bits #68762