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

Jump to bottom

crypto: Equal(PublicKey) bool methods leak to PrivateKey implementations #38190

Closed
andybons opened this issue Mar 31, 2020 · 8 comments
Closed

crypto: Equal(PublicKey) bool methods leak to PrivateKey implementations #38190

andybons opened this issue Mar 31, 2020 · 8 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone
Go1.15

Comments

@andybons
Copy link
Member

Since golang.org/cl/225460 (#21704), the following code compiles but prints an unexpected result:

package main

import (
	"crypto/rand"
	"crypto/rsa"
)

func main() {
	pk, _ := rsa.GenerateKey(rand.Reader, 512)
	println(pk.Equal(pk)) // prints false
}

This is due to PrivateKey embedding PublicKey without having its own Equal method to mask PublicKey’s.

This causes difficult to debug issues with tools like go-cmp, as it looks for an Equal method on each type recursively and finds one in this case for the concrete private key types. The diff printed by the following code will be non-empty, but the values are identical:

package main

import (
	"crypto/rand"
	"crypto/rsa"
	"fmt"
	"math/big"

	"github.com/google/go-cmp/cmp"
)

func main() {
	pk, _ := rsa.GenerateKey(rand.Reader, 512)
	bigIntCmp := cmp.Comparer(func(x, y *big.Int) bool {
		return x.Cmp(y) == 0
	})
	fmt.Printf("Diff: '%s'\n", cmp.Diff(pk, pk, bigIntCmp))
}

/cc @FiloSottile @katiehockman
@andybons andybons added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Mar 31, 2020
@andybons andybons added this to the Go1.15 milestone Mar 31, 2020
@katiehockman
Copy link
Contributor

I ran into the same issue with #33564 where I created a MarshalText method on ecdsa.PublicKey, which then leaked to ecdsa.PrivateKey because it embedded PublicKey.

That's unfortunate. @FiloSottile how do you want to proceed here? Make an Equal on the PrivateKey types too?

@katiehockman katiehockman added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 1, 2020
@rsc
Copy link
Contributor

rsc commented Apr 1, 2020

Nice bug. If a particular PrivateKey embeds PublicKey, then any time you implement PublicKey.M you should also implement PrivateKey.M to do the private key-specific behavior, whatever that is. Probably there should be a (non-doc) comment in the sources anywhere we do this, alerting people to the gotcha.

Embedding the PublicKey instead of calling the field Public was probably a mistake, but too late now.

@FiloSottile
Copy link
Contributor

Yeah, using embedding was probably a mistake in hindsight, even if I see how it made sense since the PublicKey fields are arguably PrivateKey fields as well.

Question about Equal semantics: would you expect two equivalent PrivateKeys to return true or false if one has Precomputed set and the other doesn't? I think false?

(I was hoping to avoid the question by not implementing Equal on the private keys, but yes, too late.)

@rolandshoemaker
Copy link
Member

Question about Equal semantics: would you expect two equivalent PrivateKeys to return true or false if one has Precomputed set and the other doesn't? I think false?

Naively I'd assume Equal compares only the key material and ignores the Precomputed field. I think one thing to consider is what the intended use of the method is, it seems likely people will use this to verify too keys are the same (i.e. similarly to marshaling to DER and comparing the encoding is equal) rather than comparing that the actual structs are equal. Either way this would be a good thing to document publicly.

(It could be somewhat confusing to consider it given ECDSA and EdDSA have no analog.)

@andybons andybons added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 16, 2020
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Apr 16, 2020
@andybons
Copy link
Member Author

Question about Equal semantics: would you expect two equivalent PrivateKeys to return true or false if one has Precomputed set and the other doesn't? I think false?

@rsc what do you think?

@rsc
Copy link
Contributor

rsc commented Apr 21, 2020

I would personally expect Equal to be semantic rather than picky about the exact representation. But if you can make an argument for being picky, that's OK with me too.

@FiloSottile
Copy link
Contributor

@rsc pointed out IP.Equal and Time.Equal as precedents of Equal meaning equivalent, so let's implement it by ignoring Precomputed.

@gopherbot
Copy link

Change https://golang.org/cl/231417 mentions this issue: crypto/rsa,crypto/ecdsa,crypto/ed25519: implement PrivateKey.Equal

xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
@FiloSottile @xujianhai666
crypto/rsa,crypto/ecdsa,crypto/ed25519: implement PrivateKey.Equal
298cf42 
Fixes golang#38190

Change-Id: I10766068ee18974e81b3bd78ee0b4d83cc9d1a8c
Reviewed-on: https://go-review.googlesource.com/c/go/+/231417
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
@golang golang locked and limited conversation to collaborators May 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
6 participants
@rsc @andybons @FiloSottile @rolandshoemaker @gopherbot @katiehockman