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: 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
Assignees
Milestone

Comments

@andybons
Copy link
Member

@andybons andybons commented Mar 31, 2020

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

@katiehockman
Copy link
Member

@katiehockman katiehockman commented Apr 1, 2020

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?

@rsc
Copy link
Contributor

@rsc 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
Member

@FiloSottile FiloSottile commented Apr 1, 2020

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

@rolandshoemaker rolandshoemaker commented Apr 5, 2020

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

@andybons andybons commented Apr 16, 2020

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 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
Member

@FiloSottile FiloSottile commented Apr 24, 2020

@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

@gopherbot gopherbot commented May 1, 2020

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

@gopherbot gopherbot closed this in a8e83d5 May 5, 2020
xujianhai666 added a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.