Ensure generated RSA key primes satisfy P < Q constraint by jam-awake · Pull Request #305 · ProtonMail/go-crypto

jam-awake · 2026-03-19T22:28:26Z

Adds a wrapper around rsa.GenerateKey that swaps the P and Q primes if P > Q and recomputes the precomputed values
Applies this change to generateRSAKeyWithPrimes as well but only if the key has 2 prime numbers because I don't know what's safe to do in this context.

Questions I have:

Given the duplication of these two functions' implementations, should this be consolidated somewhere? Or is it fine to have the duplication?
I'm not sure what tests should be written that verifies this work, so I've omitted those. Is that fine?

Fixes #184

Also, per your contributing guidelines:

I certify that the contribution was created in whole by me

I asked an AI to investigate the problem and attempt to fix it. I then reviewed what it produced and edited it to something that I feel like I would have written. I'm not familiar with Go or cryptography, but this issue seems small enough that this method seemed appropriate.

andgal-siren · 2026-03-20T16:07:18Z

IMO since the established practice is to duplicate code between v1 and v2 it is reasonable to follow suit and duplicate the additional code, rather than attempting to deduplicate only these particular changes.

It would be nice to have a test for p<q, however since this will only fail at random it may not be appropriate to include it in the unit tests here...

jam-awake · 2026-03-20T16:41:19Z

IMO since the established practice is to duplicate code between v1 and v2 it is reasonable to follow suit and duplicate the additional code, rather than attempting to deduplicate only these particular changes.

👍

It would be nice to have a test for p<q, however since this will only fail at random it may not be appropriate to include it in the unit tests here...

Yeah, that's why I didn't include them. That being said, https://github.com/jam-awake/gpg-verify-bug does have a few RSA keys exhibiting this very issue. Is it possible to force rsa.generateKey to create bad keys via generateRSAKeyWithPrimes?

twiss · 2026-03-20T16:57:06Z

Thanks for the PR!

If you pass a deterministic config.Rand, you could test this deterministically (i.e. the same keys will be generated every run).

Also, could you please cite the new RFC 9580 rather than RFC 4880? 🙏

jam-awake · 2026-03-20T21:47:00Z

Also, could you please cite the new RFC 9580 rather than RFC 4880?

Done. I should note there are other references to RFC 4880 in the repo, but I don't plan on fixing those in this PR.

If you pass a deterministic config.Rand, you could test this deterministically (i.e. the same keys will be generated every run).

I misread this. I saw config and thought that meant passing in a config value. And then I was confused as to how I would accomplish that since neither function takes such an argument. (Instead of thinking about that more, I focused on figuring out how to write a test.) But you're just saying I should provide a deterministic io.Reader. That would allow me to test both functions. Ok.

I also looked through the test file and saw ReadArmoredKeyRing. That will make it easier to use the primes from https://github.com/jam-awake/gpg-verify-bug.

Let me work on that, but I don't anticipate getting to this again until sometime next week.

jam-awake · 2026-03-27T14:36:27Z

+
+	for _, testCase := range tests {
+		t.Run(testCase.name, func(t *testing.T) {
+			for i := 0; i < 20; i++ {


Here's the context behind this loop.

generateRSAKey/generateRSAKeyWithPrimes calls rand.Prime to generate a prime number using Io.Reader. rand.Prime does so by running the following loop:

Read data from the Reader. If there's not enough to fill the buffer, return io.ErrUnexpectedEOF.

Generate a new number from the read data

Call ProbablyPrime on that number. If it passes, return the number. Otherwise, jump to step 1.

Running this test without this loop will sometimes succeed and sometimes fail with Failed to generate RSA key: unexpected EOF. Presumably, a failing ProbablyPrime check will cause more data to be read from the reader than what it contains. And once we reach the end, the reader's EOF is converted to that error and rand.Prime returns.

At first, I thought one or both of p or q are not actually prime numbers. However, when I call ProbablyPrime(1_000) on p and q, neither number reports an issue. So, perhaps the issue is with the DeterministicReader implementation? I'm not sure.

Regardless, looping was my temporary solution to this problem. Per its docs, ProbablyPrime has a (1/4)^20 chance of returning a false positive (i.e. saying a number is a prime when it's non-prime). I thought attempting to generate a key 20 times might somehow cancel out that issue.

ProbablyPrime is itself randomized, so this makes some sense to me.

jam-awake · 2026-03-27T14:37:13Z

@twiss I believe I've addressed all your feedback aside from my comment above.

jam-awake · 2026-04-01T13:55:45Z

@twiss Any updates on this?

jam-awake · 2026-04-08T15:32:38Z

@twiss Any updates on this?

lubux · 2026-04-09T14:38:28Z

+	// RFC 9580 section 5.5.5.1 requires p < q for RSA keys.
+	// The Go standard library does not guarantee this, so we swap if needed
+	// and recompute the precomputed values.
+	if len(key.Primes) == 2 && key.Primes[0].Cmp(key.Primes[1]) > 0 {


Cmp is not constant time and exposes a timing side-channel on the secret primes. But should be fine for key generation in this case.

I'm reading this comment as, "This would be a problem, but not in this particular usage." Is that correct?

- Adds a wrapper around `rsa.GenerateKey` that swaps the P and Q primes if P > Q and recomputes the precomputed values - Applies this change to `generateRSAKeyWithPrimes` as well but only if the key has 2 prime numbers because I don't know what's safe to do in this context. - Adds tests verifying prime swap occurs using 2048-bit key from https://github.com/jam-awake/gpg-verify-bug

jam-awake · 2026-04-09T16:21:03Z

I apologize for force pushing, but since my previous commits were unsigned, this PR would not have been mergeable.

In fixing that problem, I also collapsed the commits into a single one. The PR is unchanged, but now it's mergeable.

jam-awake · 2026-04-27T13:22:19Z

@lubux Any updates on this PR?

jam-awake · 2026-05-01T14:31:45Z

@lubux The PR builds and is approved, but I can't merge it. I'm assuming one of the maintainers here needs to do so?

twiss

Apologies for the long silence on my part, I was off last month. One more nitpick below.

twiss · 2026-05-04T11:29:14Z

+
+	for _, testCase := range tests {
+		t.Run(testCase.name, func(t *testing.T) {
+			for i := 0; i < 20; i++ {


ProbablyPrime is itself randomized, so this makes some sense to me.

jam-awake · 2026-05-11T16:04:18Z

@twiss I've addressed your feedback. Anything else?

twiss

👍 Thanks!

jam-awake · 2026-05-12T12:28:33Z

CI builds. I'm assuming one of the repo's maintainers here needs to merge the PR. Is that correct?

jam-awake commented Mar 27, 2026

View reviewed changes

lubux reviewed Apr 9, 2026

View reviewed changes

lubux approved these changes Apr 9, 2026

View reviewed changes

jam-awake force-pushed the jam/fix-rsa-p-q-order branch 2 times, most recently from 542fba8 to 4b8e1ad Compare April 9, 2026 16:13

jam-awake force-pushed the jam/fix-rsa-p-q-order branch from 4b8e1ad to bed1dc3 Compare April 9, 2026 16:19

lubux previously approved these changes Apr 29, 2026

View reviewed changes

twiss reviewed May 4, 2026

View reviewed changes

Use bytes.NewReader

ae50ee2

jam-awake dismissed lubux’s stale review via ae50ee2 May 5, 2026 14:33

Merge branch 'main' into jam/fix-rsa-p-q-order

a68fdf4

twiss approved these changes May 12, 2026

View reviewed changes

twiss merged commit 681b4e5 into ProtonMail:main May 12, 2026
9 checks passed

jam-awake deleted the jam/fix-rsa-p-q-order branch May 12, 2026 13:09

Conversation

jam-awake commented Mar 19, 2026

Uh oh!

andgal-siren commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jam-awake commented Mar 20, 2026

Uh oh!

twiss commented Mar 20, 2026

Uh oh!

jam-awake commented Mar 20, 2026

Uh oh!

jam-awake Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

twiss May 4, 2026

Choose a reason for hiding this comment

Uh oh!

jam-awake commented Mar 27, 2026

Uh oh!

jam-awake commented Apr 1, 2026

Uh oh!

jam-awake commented Apr 8, 2026

Uh oh!

lubux Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jam-awake Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

jam-awake commented Apr 9, 2026

Uh oh!

jam-awake commented Apr 27, 2026

Uh oh!

jam-awake commented May 1, 2026

Uh oh!

twiss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

twiss May 4, 2026

Choose a reason for hiding this comment

Uh oh!

jam-awake commented May 11, 2026

Uh oh!

twiss left a comment

Choose a reason for hiding this comment

Uh oh!

jam-awake commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andgal-siren commented Mar 20, 2026 •

edited

Loading

lubux Apr 9, 2026 •

edited

Loading