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

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
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done. Unfortunate
Milestone
Unreleased

Comments

@btoews
Copy link

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

/cc @agl

@btoews
Copy link
Author

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

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

@fawkesley fawkesley mentioned this issue Oct 24, 2018
Closed
@ni4
Copy link

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 commented Feb 8, 2019

/cc @FiloSottile

Looks like people really love to fix this issue: https://mailarchive.ietf.org/arch/msg/openpgp/f--SM0L4y4kicxuLER9WLWs-5uc

@dkg
Copy link

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

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

@twiss twiss mentioned this issue May 15, 2019
Closed
@wiktor-k wiktor-k mentioned this issue Oct 1, 2019
Closed
@FiloSottile FiloSottile mentioned this issue Feb 11, 2021
Closed
@FiloSottile FiloSottile added Documentation NeedsFix The path to resolution is known, but the work has not been done. Unfortunate labels Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done. Unfortunate
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
8 participants
@dkg @mikioh @fawkesley @btoews @FiloSottile @ni4 @odeke-em @gopherbot