encoding/asn1: valid GeneralizedTime with UTC offset of +0000 not parsed

[dcormier]

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

$ go version
go version go1.12.7 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

macOS 10.14.5

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/[user]/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/[user]/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.7/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.7/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/84/_6l41bt970l9fmsrwc_p1gv00000gn/T/go-build217266963=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Attempted to upgrade to TLS on a server I don't control.

It appears that the time format is valid, according to the documentation I've found (here, among other places).

Public cert from that server included in this failing test:

func TestUTCOffset(t *testing.T) {
	certPEM := []byte(`-----BEGIN CERTIFICATE-----
MIIDNzCCAqCgAwIBAgIJAOG5Q5oZboH9MA0GCSqGSIb3DQEBBAUAMHQxCzAJBgNV
BAYTAmNhMREwDwYDVQQHEwhJbm5pc2ZpbDENMAsGA1UEChMESkpFSTEZMBcGA1UE
AxMQSkpFSSBXZWJBZG1pbiBDQTEoMCYGCSqGSIb3DQEJARYZc3VwcG9ydEBwYWNl
dGVjaG5pY2FsLmNvbTAiFw0wODExMTIwMjE3NTJaFxEzNjAzMjkwMjE3MjkrMDAw
MDBFMQswCQYDVQQGEwJjYTERMA8GA1UEBxMISW5uaXNmaWwxDTALBgNVBAoTBEpK
RUkxFDASBgNVBAMTC2d3LmpqZWkuY29tMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCB
iQKBgQCpGszlFBX7X8t2ruSxDgmFHmunwbpbXlqT+Ekh/TBP/I4qwbepPY+nL9jo
0+ngO7cENWnXLc1B2N32uXakD0ygJzgN6ftbwX0nMWBOG5dcc+TQCl518q9aTAEx
R1LFXXvAM1uCknjYINnyzbs7xFxdhIVAZG6m/hcPPtiu6c1WnQIDAQABo4H7MIH4
MB0GA1UdDgQWBBRmqWzvNfSxFKPuGh1xRA6MPotYqjCBpgYDVR0jBIGeMIGbgBSH
ssHbNYjHbMAf3QJ9/EExdAbmTaF4pHYwdDELMAkGA1UEBhMCY2ExETAPBgNVBAcT
CElubmlzZmlsMQ0wCwYDVQQKEwRKSkVJMRkwFwYDVQQDExBKSkVJIFdlYkFkbWlu
IENBMSgwJgYJKoZIhvcNAQkBFhlzdXBwb3J0QHBhY2V0ZWNobmljYWwuY29tggkA
4blDmhlugfwwFgYDVR0RBA8wDYILZ3cuamplaS5jb20wCQYDVR0TBAIwADALBgNV
HQ8EBAMCBeAwDQYJKoZIhvcNAQEEBQADgYEAAsbIrUXDZ9bnPsauS/0iIZQhJWDc
gQg8UrWOHrdjh/bVG0Cgv5kx6EGE60Q5OuvyQU1wcAN8YgqgttrlLWjWIMV1/lZD
IovhMR42FRMpYtmW+YDVRY70fPqFAGox1r5/6fi7TCXKZmkNFQ0SoayW6xQmtqct
48cZbI2/iiwqeVE=
-----END CERTIFICATE-----`)

	certDERBlock, _ := pem.Decode(certPEM)
	_, err := x509.ParseCertificate(certDERBlock.Bytes)
	if err != nil {
		t.Fatal(err)
	}
}

What did you expect to see?

The certificate would get parsed.

What did you see instead?

asn1: time did not serialize back to the original value and may be invalid: given "360329021729+0000", but serialized as "360329021729Z"

The problem looks like it may be in the same area as #15842.

[qbradq]

Issue appears to be a duplicate of #15842 . The patch appears to have been ready during the 1.11 time frame but hasn't made the cut since. I replied to the associated CL to see if someone can make a call.

[ALTree]

FWIW I tested the reproducer here on the patch in #15842 and it doesn't seem to fix it, so the patch may fix #15842 but it's possibly not 100% complete.

[dcormier]

Yeah, the patch for #15842 just makes it so encoding/asn1 will parse in a date with fractional seconds rather than dropping the fraction part. Then when the package formats the parsed date as a string to compare with the original it works for that case.

The problem here is that when encoding/asn1 formats the parsed date and compares it to the original string, that original +0000 offset gets formatted as Z, so it doesn't match the original string. That happens right here.

[knieriem]

My patch for #15842 has the purpose of enabling the parsing of fractional digits in GeneralizedTime (From my understanding of X.690, p. 20, fractional digits are allowed in GeneralizedTime, but not in UTCTime).

This issue (33192), though, is related to UTCTime, not GeneralizedTime. (So the patch for #15842 won't fix this issue already for the reason that a different function is called for parsing).
If you list the ASN.1 data from the certificate above using

openssl x509 -in cert.pem -outform der | openssl asn1parse -inform der

the output will contain the line

174:d=3  hl=2 l=  17 prim: UTCTIME           :360329021729+0000

I wonder if the +0000 suffix is correct, since X.609 states that UTCTimes in DER encoding must end in Z. See https://forums.openvpn.net/viewtopic.php?t=21873#p62295 for a related discussion; RFC5820 (and RFC2459), which is referenced there, also requires UTCTime with a Z suffix.

One approach would be to make asn1.go:parseUTCTime conservatively accept a +0000 suffix. The if condition in asn1.go:345 could be extended by a test like

&& !(strings.HasSuffix(s, "+0000") && strings.HasSuffix(serialized, "Z"))

Since parseUTCTime already accepts non-standard time differentials like +0200, fixing the equality-test for +0000 and Z appears reasonable IMHO.

For completeness, please also see the fix for #19890, which could complement a fix for this issue, addressing the marshaling side.