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

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

Open
dcormier opened this issue Jul 19, 2019 · 4 comments
Open

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

dcormier opened this issue Jul 19, 2019 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@dcormier
Copy link
Contributor

dcormier commented Jul 19, 2019 •
edited

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.
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 19, 2019
@ALTree ALTree added this to the Go1.14 milestone Jul 19, 2019
@qbradq
Copy link
Contributor

qbradq commented Jul 24, 2019

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

ALTree commented Jul 24, 2019

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

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
Copy link
Contributor

knieriem commented Jul 24, 2019 •
edited

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.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@rsc @dcormier @knieriem @qbradq @ALTree