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

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

Open
FiloSottile opened this issue Aug 7, 2024 · 28 comments
Open
Assignees
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@FiloSottile
Copy link
Contributor

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:

  1. return an error from rsa.GenerateKey if bits is less than 1024
  2. 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.

@FiloSottile FiloSottile changed the title crypto/rsa: refuse to generate and/or use keys smaller than 1024 bits proposal: crypto/rsa: refuse to generate and/or use keys smaller than 1024 bits Aug 7, 2024
@FiloSottile FiloSottile added the Proposal-Crypto Proposal related to crypto packages or other security issues label Aug 7, 2024
@gopherbot gopherbot added this to the Proposal milestone Aug 7, 2024
@alex
Copy link
Contributor

alex commented Aug 7, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

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

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

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Aug 7, 2024
@rittneje
Copy link

rittneje commented Aug 7, 2024

@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
Copy link

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
Copy link

rittneje commented Aug 8, 2024

@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
Copy link

mcpherrinm commented Aug 8, 2024

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, hashicorp/consul#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
Copy link
Contributor Author

FiloSottile commented Aug 8, 2024 via email

@rsc
Copy link
Contributor

rsc commented Aug 13, 2024

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 rsc moved this from Incoming to Active in Proposals Aug 14, 2024
@rsc
Copy link
Contributor

rsc commented Aug 14, 2024

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
Copy link

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
Copy link

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

@ryancdotorg
Copy link

Use these as test keys:

-----BEGIN RSA XXXVATE KEY-----
MIIBUgIBAAJBAKpg//++WEAK+512+BIT+KEY+FOR+TEST+ONLY++++DANGER++//
0rO6rZovwzaBgP4SxtCsUiyDMyT/1wF9Ma0CAwEAAQJAC6topVLbdwlhBM+AMx48
2s/AZNAJWib2oa5QaA83cniaXNXolcM9NZT/UDoqwx+BJFn6rojXcWG9ORpqUXe8
iQI5AWX/GfqHy2Z0b5MpyMy9OP3iNt7AUMjS9ejCSx6z4khIWw58ZytSUnATnC2D
hcE3NTnznGzLC7l5Agh51h2n14Lg1QI5ASoI1nfb1VnWVY0OnDzWyzuRuFx5fsex
2UeNeTx2RUwn8KZ+quVGA5uKt9qsRA43B/jW9sMbxwcBAggBF1zeovyGHQI5ARLe
j5Jc58CPnHyGrZUTNLUHgFMjpZH2BUqZFEvIUPm+LXcVWpPZWi2GzERa9cRaTcsb
W6omvrw/
-----END RSA XXXVATE KEY-----

-----BEGIN RSA XXXVATE KEY-----
MIIB0gIBAAJhALfo//++WEAK+768+BIT+KEY+FOR+TEST+ONLY++++DANGER++//
+QEP155gUbI4UAxQVnD8/2pH8+ZZGtc6yXnOHUdO1ySYzQ91viHQajaXW9Vn/t34
j7Da4frVpstyswIDAQABAmAPqD8PO1nRDf5TxUvLYjbjJDiUCNPxyRDTGyRbXhOc
lVCASSMtUhoVMvfmPls7VIqwaYg8L/hVdm3v6/YfwaeOaAV+RkAWlYS1gPPVCKo2
LOnyhdw6KEuneTCccLxE7UECOQKkeSkLKa2kYy7gPZYHXbqtMy8TivyobsRsA0sU
HuLZYvokapgbAgusgxlOcZgGwUXObaS93oaecwIoRZkEXHSniZ7OOkXWFVYpEViS
2owMeur5hfdR1oAfSMYWEBm96YyKwQI5Ab8LytHzsysF2L6cNH+ds3YucxfpK9tr
iCcRbhZX6vZacJCMWQTQL0LMSHcMnrkoFKe80bggTLp9Aig8s1uJY7jnn5kQW+5/
B3mkBlN2xyTvY9cjD3TSF0axywpdGoZ3lrtBAjkBuSSwlzmJco1u3G4x4oXvHpS1
yMaGpwKJCl5N9Efni07TXH+C9Oz1wf8bMR1LxPANUqtNwUaW60Y=
-----END RSA XXXVATE KEY-----

-----BEGIN RSA XXXVATE KEY-----
MIICXQIBAAKBgQC5//++WEAK+1024+BIT+KEY+FOR+TEST+ONLY+++DANGER++//
6PoXbDDMxixMEODKThOSdUqKKF5R53uraeLezENUHrkzoIsyQgu5t93oVg7AW8IO
aMAFn6H/wzLy3wXr5CsHCINa5nM018bUR8vmGErhGR77rGxD4zFQ/CIFsQIDAQAB
AoGAP82aG+BH77x0KE0Y9ZU5nbJpaiHtTovV2mEolwht+2C8X5/tnvp7N6esQcJF
Fb7AbYVE33uOmz/nwu7GFHHZGXre8tDX4tG8dndoE+vhqplhgLHNGV/bXsVZAjxF
ZqN9hkq9H7hRlV4+xtn39dGvHp7UeuyPVYB3xBAWibdiNGUCQQEe2Nn6CtJWx0Wa
k4gxOCdPzCbQM3vveZBucGssJ4O1rZ2xjV6Wxek+Cm8ogDcrSCZ+7fhPL6rlzk/0
bzZRIMQnAkEApf9zWO4P6wqx3NAxBn+Zy1odKD0JUw/h2phVaNmjX7qx7O6A1XqO
2H8f4xodPCkxQdmDU6DVOZHXjpLNnEJWZwJBANsLgbk9lk4KMg7OZowfc3WuUl1f
U18WF8MeOdkn+547DFbPu9GrJGfqE+R7tKVqnWsEUkA2CG0g1VG1s2bUfLUCQFIT
YGEUNGKuwwq/Fb500QIu6EPBCh87txxyPai+E319vgO8WY80LfT1xjpv6wyYAXbh
qYFsAIGajs4739XnJvkCQQCWvw8mctt2BssKCAfvFx0D6rHB6kgH5pII1QAVI/q/
NImR9qVfZXLkCsQWsTqxgyRvte7ewZqwbxTmv8jWuae7
-----END RSA XXXVATE KEY-----

-----BEGIN RSA XXXVATE KEY-----
MIIDfAIBAAKBwQDA//++WEAK+1536+BIT+KEY+FOR+TEST+ONLY+++DANGER++//
2GWvEP4DM0s/ALhJzPoO8mktA8o8hkekN/UdakQEHlJCcb56ckC0hY+aC3wG90Ya
KMXavlGs0LideE46hPY4Qdc1dQwbhj6HcOe2oxf9noee7rxxP/WHpZxSUinLRV6z
quLsOe4PNdU+Gjp8QnSnyI/46OjCJwdOAr/iKVtdR8/1mQeD+veTLfXx5QLR32ns
LMy8rDzCktiMtH0CAwEAAQKBwEeQBNei6GhKCZ7Exv55JIA7esTocoJ3uIm1sOfM
xGrwYRfmh3ih2B5gWhd8swdy8GJpD0VwjCAlWh00GydgmlIkX4D5bz306BCGAckO
Bw+y93Orx4IWoTp5PFasc+/rtBA5aiOcwNWgFfZSm5F3VwoxZAU0/R9x573LNRuH
SR4z/gRL6yLfJMRYsTaDPttsNXvU6569XnlrzAeq4v49D3xPAhNe69q8cWETxJU9
zdzk0Ovjb/wWMgQFjs3yTG2rbwJhBk4JsP54R8st+yemlUk7JaEssB22gj9F6R7v
o2swuCxrQsTOUK3kpLZF252Ms8hCMhj7PZWLZeZ3UhKwJN1MB6Ipwb0gEx4JeghD
DlONLfzjRhqOV/b1jVbnQ2wP9MoRbwJgHpyGW0tWqQ9zyRSq/Fj92OIS9UlafBZV
Fi6O0LPG5KSpraRSx6c40SrybpRmUFLZF3B/ADwxYU8xcQKKzN8MuXAfhLkFLAvh
+iUAjn4SvcK3A74bNQc6p5ROIdd2zgrTAmEEwHhJb+FTquzi6EWDKohrmghGTH6r
t+iHBQv0DOvRQ5kr34zn/cff5IjONvY+4eYSQAXLqpQ/nu27a95aSnftIOl73YD5
J6BY2zU+7PXw/TKGOami3ry/ZAo1JJL6Gfn9AmAT+CZJ7jblaNguyBXXMzK+RpT5
gNXPdz4gj1TJX04ToDu0tCrwZe1RvoOSkarBIZrPiKrA+4N3KJNnVrI3fhat7jAR
hCWUm1fauELJsgMF2b1MarsS99lSsxPZTdcKCNMCYQFoV8Q4gcw7UluLHCrjlla9
zSX/NqfzKBFSrLReBultBiw4glTJOX8ozVhmK0JUdDPSkbAi//R8rTNGFWjtehqe
4BtU9ZqYTvxXSDSPCNvVDbTcEGwycSRyQKq1cLQUt+Y=
-----END RSA XXXVATE KEY-----

@clausecker
Copy link

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
Copy link

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
Copy link

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
Copy link

ryancdotorg commented Aug 15, 2024

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
Copy link
Contributor

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

@FiloSottile
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor

rsc commented Aug 28, 2024

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
Copy link
Contributor

rsc commented Aug 29, 2024

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
Copy link
Contributor

rsc commented Sep 4, 2024

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 rsc moved this from Active to Likely Accept in Proposals Sep 4, 2024
@rsc
Copy link
Contributor

rsc commented Sep 11, 2024

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.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Sep 11, 2024
@rsc rsc changed the title proposal: crypto/rsa: refuse to generate and/or use keys smaller than 1024 bits crypto/rsa: refuse to generate and/or use keys smaller than 1024 bits Sep 11, 2024
@rsc rsc modified the milestones: Proposal, Backlog Sep 11, 2024
@picatz
Copy link

picatz commented Oct 1, 2024

Stumbled upon this by accident. Love the idea, and excited to see it accepted! 🎉

FWIW, I maintain a go/analysis based linter that helps detect insecure RSA key generation: https://github.com/picatz/rsalint

@mauri870
Copy link
Member

Assigning this to me, to try and get this in the tree before the freeze.

If someone is already looking into it please let me know.

@mauri870 mauri870 self-assigned this Nov 14, 2024
@FiloSottile
Copy link
Contributor Author

@mauri870 I'm planning to land this along with the #69536-related changes. It will at least interact with them, because in FIPS 140 mode you can't use key smaller than 2048 bits.

@mauri870
Copy link
Member

Thanks @FiloSottile!

@mauri870 mauri870 assigned FiloSottile and unassigned mauri870 Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Accepted
Development

No branches or pull requests

13 participants