|
|
Created:
10 years, 6 months ago by jmpittman Modified:
10 years, 6 months ago Reviewers:
CC:
agl1, dave_cheney.net, jpsugar, hanwen-google, golang-dev Visibility:
Public. |
Descriptiongo.crypto/ssh: Implement CertTime to properly handle the "infinite" time
value ^0, which would become negative when expressed as int64.
Patch Set 1 #Patch Set 2 : diff -r 32844aa1ae54 https://code.google.com/p/go.crypto #Patch Set 3 : diff -r 32844aa1ae54 https://code.google.com/p/go.crypto #
Total comments: 4
Patch Set 4 : diff -r 32844aa1ae54 https://code.google.com/p/go.crypto #
Total comments: 6
Patch Set 5 : diff -r 32844aa1ae54 https://code.google.com/p/go.crypto #Patch Set 6 : diff -r 32844aa1ae54 https://code.google.com/p/go.crypto #
Total comments: 9
Patch Set 7 : diff -r 32844aa1ae54 https://code.google.com/p/go.crypto #MessagesTotal messages: 11
Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com, hanwen@google.com, jpsugar@google.com), I'd like you to review this change to https://code.google.com/p/go.crypto
Sign in to reply to this message.
https://codereview.appspot.com/15520047/diff/40001/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/15520047/diff/40001/ssh/certs.go#newcode63 ssh/certs.go:63: return ct == 1<<64-1 Doesn't this result in an overflow? https://codereview.appspot.com/15520047/diff/40001/ssh/keys_test.go File ssh/keys_test.go (right): https://codereview.appspot.com/15520047/diff/40001/ssh/keys_test.go#newcode49 ssh/keys_test.go:49: ValidBefore: 0xFFFFFFFFFFFFFFFF, // The end of currently representable time. Use a constant here?
Sign in to reply to this message.
https://codereview.appspot.com/15520047/diff/40001/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/15520047/diff/40001/ssh/certs.go#newcode63 ssh/certs.go:63: return ct == 1<<64-1 I do not think it does. I have not gotten an overflow in separate testing (i.e. playground). The math package defines this... MaxUint64 = 1<<64 - 1, but I did not want to import the whole math package just for this. I can get 1<<64 to overflow, but I did not have 1<<64 - 1 overflow. https://codereview.appspot.com/15520047/diff/40001/ssh/keys_test.go File ssh/keys_test.go (right): https://codereview.appspot.com/15520047/diff/40001/ssh/keys_test.go#newcode49 ssh/keys_test.go:49: ValidBefore: 0xFFFFFFFFFFFFFFFF, // The end of currently representable time. On 2013/10/22 21:45:35, jpsugar wrote: > Use a constant here? Done.
Sign in to reply to this message.
what problem is this CL solving? https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode55 ssh/certs.go:55: // handle time values above what a time.Time can represent and prevent values This comment is too large compared to what we're dealing with. time.Time goes until way beyond we all die because of petrol crisis/global warming/world war 3. It might even go beyond the time needed to brute-force RSA-2048, so it buys little additional security (I haven't checked though).
Sign in to reply to this message.
TLDR... 1. Conversion from uint64 to int64 to time.Time and then back had issues with negative times (before unix epoch) which are not allowed in certs. 2. Helps with parse/marshal when trying to work around issue 1. The field encoding is defined as a uint64, not an int64 like is becoming typical for representing time with 64bit values. It was done this way in the spec to avoid introducing a new wire encoding format for ssh (this was djm's reasoning when I asked). Also, valid times for certificates must start at unix epoch. So, a value of 0 is supposed to correspond to unix epoch. The problem this solves... In openssh, if a time frame/window is not specified for a certificate during creation, the default behavior is to set ValidAfter to all 0's and ValidBefore to all 1's (in binary form). This is known as the "forever" setting. Previously, the code was doing a direct conversion from uint64 to int64. Any value above 1<<63-1 (max for an int64) gets treated as a negative value. So, what should be considered a positive value (really far into the future) suddenly becomes the past (before unix epoch). Specifically, 0xFFFFFFFFFFFFFFFF in a uint64 is really far into the future, but that same value in an int64 is -1 or unix epoch - 1 second. In the current form, when doing time validation on a cert, this will flip flop the window so that the certificate expires before it ever becomes valid. Introducing CertTime will clamp the time range to start at unix epoch and go to the max value for int64, but never before unix epoch. So, the time window is always positive. The other issue this solves is encoding. In trying to find a good way around this issue with just a time.Time value, everything I did would screw up the parse/marshal value comparison. If you want a test for this, we already have one. The example key that jpsugar provided has validafter at 0 and validbefore at all 0xFF bytes because he never set a time window. So, the cert is intended to always be good. With the current code, the certificate would have been expired one second before it became valid. Does that make sense? https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode55 ssh/certs.go:55: // handle time values above what a time.Time can represent and prevent values I wrote two comments. One for the reader of the code and one for package documentation. I was hoping the first one explained the reasoning for the CL.
Sign in to reply to this message.
https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode55 ssh/certs.go:55: // handle time values above what a time.Time can represent and prevent values ".. to properly handle the "infinite" time value ^0, which would become negative when expressed as int64" (I don't think anyone is concerned with any other value.) Likewise, the analysis of how this will be correct 300 billion years from now can be removed. https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode66 ssh/certs.go:66: func (ct CertTime) IsTheEnd() bool { IsInfinite?
Sign in to reply to this message.
Also, can you change the description to be more explicit? Say how you fixed what issue. On Tue, Oct 22, 2013 at 2:36 PM, <jmpittman@google.com> wrote: > Reviewers: agl1, dfc, > > Message: > Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com, > hanwen@google.com, jpsugar@google.com), > > I'd like you to review this change to > https://code.google.com/p/go.crypto > > > Description: > go.crypto/ssh: Implement CertTime to fix an issue with some time > conversions. > > Please review this at https://codereview.appspot.com/15520047/ > > Affected files (+36, -9 lines): > M ssh/certs.go > M ssh/keys_test.go > > > Index: ssh/certs.go > =================================================================== > --- a/ssh/certs.go > +++ b/ssh/certs.go > @@ -35,6 +35,34 @@ > Data string > } > > +// A time.Time cannot represent seconds higher than 1<<63 - 1. However, > +// conversion from uint64 to int64 for values higher than this will result > in > +// negative int64 values and result in times before unix epoch that are not > +// intended to be used with certs. maxInt64 in unix seconds would be > +// 292277026596-12-04 15:30:07 +0000 UTC. We are safe until the year > +// 292,277,026,596 in setting this value to maxInt64 for values above > +// maxInt64. A use case for this is the "forever" setting where ValidAfter > is 0 > +// (all bytes 0x00) and ValidBefore is 1<<64 - 1 (all bytes 0xFF). OpenSSH > does > +// something similar to this by clamping to INT_MAX. > + > +// CertTime represents an unsigned 64bit time value in seconds starting > from > +// unix epoch. We use CertTime instead of time.Time in order to properly > +// handle time values above what a time.Time can represent and prevent > values > +// before unix epoch. > +type CertTime uint64 > + > +func (ct CertTime) Time() time.Time { > + const maxInt64 = 1<<63 - 1 > + if ct > maxInt64 { > + return time.Unix(maxInt64, 0) > + } > + return time.Unix(int64(ct), 0) > +} > + > +func (ct CertTime) IsTheEnd() bool { > + return ct == 1<<64-1 > +} > + > // An OpenSSHCertV01 represents an OpenSSH certificate as defined in > // [PROTOCOL.certkeys]?rev=1.8. > type OpenSSHCertV01 struct { > @@ -44,7 +72,7 @@ > Type uint32 > KeyId string > ValidPrincipals []string > - ValidAfter, ValidBefore time.Time > + ValidAfter, ValidBefore CertTime > CriticalOptions []tuple > Extensions []tuple > Reserved []byte > @@ -115,8 +143,8 @@ > r = marshalUint32(r, cert.Type) > r = marshalString(r, []byte(cert.KeyId)) > r = marshalLengthPrefixedNameList(r, cert.ValidPrincipals) > - r = marshalUint64(r, uint64(cert.ValidAfter.Unix())) > - r = marshalUint64(r, uint64(cert.ValidBefore.Unix())) > + r = marshalUint64(r, uint64(cert.ValidAfter)) > + r = marshalUint64(r, uint64(cert.ValidBefore)) > r = marshalTupleList(r, cert.CriticalOptions) > r = marshalTupleList(r, cert.Extensions) > r = marshalString(r, cert.Reserved) > @@ -195,13 +223,13 @@ > if !ok { > return > } > - cert.ValidAfter = time.Unix(int64(va), 0) > + cert.ValidAfter = CertTime(va) > > vb, in, ok := parseUint64(in) > if !ok { > return > } > - cert.ValidBefore = time.Unix(int64(vb), 0) > + cert.ValidBefore = CertTime(vb) > > if cert.CriticalOptions, in, ok = parseTupleList(in); !ok { > return > Index: ssh/keys_test.go > =================================================================== > --- a/ssh/keys_test.go > +++ b/ssh/keys_test.go > @@ -9,7 +9,6 @@ > "reflect" > "strings" > "testing" > - "time" > ) > > var ( > @@ -46,9 +45,9 @@ > Nonce: []byte{}, // To pass reflect.DeepEqual > after marshal & parse, this must be non-nil > Key: ecdsaKey.PublicKey(), > ValidPrincipals: []string{"gopher1", "gopher2"}, // > increases test coverage > - ValidAfter: time.Now().Truncate(time.Second), > - ValidBefore: > time.Now().Truncate(time.Second).Add(time.Hour), > - Reserved: []byte{}, // To pass reflect.DeepEqual > after marshal & parse, this must be non-nil > + ValidAfter: 0, // unix > epoch > + ValidBefore: 0xFFFFFFFFFFFFFFFF, // The end > of currently representable time. > + Reserved: []byte{}, // To pass > reflect.DeepEqual after marshal & parse, this must be non-nil > SignatureKey: rsaKey.PublicKey(), > } > sigBytes, _ := rsaKey.Sign(rand.Reader, testCert.BytesForSigning()) > > -- Han-Wen Nienhuys Google Munich hanwen@google.com
Sign in to reply to this message.
https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode55 ssh/certs.go:55: // handle time values above what a time.Time can represent and prevent values On 2013/10/23 15:32:50, hanwen-google wrote: > ".. to properly handle the "infinite" time value ^0, which would become negative > when expressed as int64" > > (I don't think anyone is concerned with any other value.) > > Likewise, the analysis of how this will be correct 300 billion years from now > can be removed. Done. https://codereview.appspot.com/15520047/diff/40002/ssh/certs.go#newcode66 ssh/certs.go:66: func (ct CertTime) IsTheEnd() bool { On 2013/10/23 15:32:50, hanwen-google wrote: > IsInfinite? Done.
Sign in to reply to this message.
https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode39 ssh/certs.go:39: maxUint64 = 1<<64 - 1 for future reference, there's math.MaxUint64 and math.MaxInt64. But, in this case, importing math for that seems too much. https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode43 ssh/certs.go:43: // CertTime represents an unsigned 64bit time value in seconds starting from "64-bit" https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode44 ssh/certs.go:44: // unix epoch. We use CertTime instead of time.Time in order to properly handle s/unix/UNIX/ https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode46 ssh/certs.go:46: // int64. "an int64"
Sign in to reply to this message.
https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode39 ssh/certs.go:39: maxUint64 = 1<<64 - 1 That was my reasoning for doing this. https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode43 ssh/certs.go:43: // CertTime represents an unsigned 64bit time value in seconds starting from On 2013/10/23 16:38:37, agl1 wrote: > "64-bit" Done. https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode43 ssh/certs.go:43: // CertTime represents an unsigned 64bit time value in seconds starting from On 2013/10/23 16:38:37, agl1 wrote: > "64-bit" Done. https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode44 ssh/certs.go:44: // unix epoch. We use CertTime instead of time.Time in order to properly handle On 2013/10/23 16:38:37, agl1 wrote: > s/unix/UNIX/ Done. https://codereview.appspot.com/15520047/diff/160001/ssh/certs.go#newcode46 ssh/certs.go:46: // int64. On 2013/10/23 16:38:37, agl1 wrote: > "an int64" Done.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=a7997f1dd284&repo=crypto *** go.crypto/ssh: Implement CertTime to properly handle the "infinite" time value ^0, which would become negative when expressed as int64. R=agl, dave, jpsugar, hanwen CC=golang-dev https://codereview.appspot.com/15520047 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
|