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

x/crypto/openpgp: Incorrect comparison when checking if PGP key is expired #22312

Open
btoews opened this issue Oct 17, 2017 · 9 comments
Open

x/crypto/openpgp: Incorrect comparison when checking if PGP key is expired #22312

btoews opened this issue Oct 17, 2017 · 9 comments

Comments

@btoews
Copy link

@btoews btoews commented Oct 17, 2017

What version of Go are you using (go version)?

go version go1.9 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

darwin amd64

What did you do? / What did you expect to see? / What did you see instead?

The KeyExpired() method on the packet.Signature struct checks if the signature creation time is after the key expiration time specified by the signature:

// KeyExpired returns whether sig is a self-signature of a key that has
// expired.
func (sig *Signature) KeyExpired(currentTime time.Time) bool {
	if sig.KeyLifetimeSecs == nil {
		return false
	}
	expiry := sig.CreationTime.Add(time.Duration(*sig.KeyLifetimeSecs) * time.Second)
	return currentTime.After(expiry)
}

According to RFC 4880 section 5.2.3.6, this method should be using the key creation time instead of the signature creation time:

5.2.3.6. Key Expiration Time

(4-octet time field)

The validity period of the key. This is the number of seconds after
the key creation time that the key expires. If this is not present
or has a value of zero, the key never expires. This is found only on
a self-signature.

These timestamps will often be the same, but not necessarily. The method is used in several places in keys.go and this behavior could cause expired keys to be used inappropriately.

@gopherbot gopherbot added this to the Unreleased milestone Oct 17, 2017
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Oct 18, 2017

/cc @agl

@btoews
Copy link
Author

@btoews btoews commented Dec 1, 2017

Any opinions here on what the correct behavior is? I'm reluctant to work around this in my app without being more sure that the current behavior is incorrect.

@fawkesley
Copy link

@fawkesley fawkesley commented Oct 24, 2018

@mastahyeti I'm with you. I believe this function is incorrect.

@ni4
Copy link

@ni4 ni4 commented Feb 7, 2019

Sure, this should be fixed. Signature creation time (together with signature expiration time subpacket) is used to allow to expire signature itself, not the key.

@mikioh mikioh changed the title x/crypto: Incorrect comparison when checking if PGP key is expired x/crypto/openpgp: Incorrect comparison when checking if PGP key is expired Feb 8, 2019
@mikioh
Copy link
Contributor

@mikioh mikioh commented Feb 8, 2019

@dkg
Copy link

@dkg dkg commented Feb 8, 2019

I agree with @mastahyeti and @paulfurley that this is a bug. the expiration date should be calculated from the key creation time.

Note that Key expiration time is distinct from Signature expiration time. I believe that Go's sig.KeyLifetimeSecs is the former, not the latter.

@dkg
Copy link

@dkg dkg commented Feb 8, 2019

I'd normally try to contribute a pull request to https://github.com/golang/crypto/, but it appears that the API here is typically invoked directly on the Signature object, which doesn't have a pointer back to the key object it refers to.

So i think there might need to be an API change to fix this bug :(

@ni4
Copy link

@ni4 ni4 commented Feb 8, 2019

Seems logical that this method should be moved to key object instead of signature. For instance, key may have multiple self-signatures for multiple user ids (or have none) and theoretical ideal implementation must take care of that.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 9, 2019

Please send a CL with a // Deprecated: annotation (https://golang.org/wiki/Deprecated) explaining that the calculation is wrong.

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
8 participants