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

encoding/asn1: invalid DER encodings of GeneralizedTime when time is not UTC #69782

Open
woodruffw opened this issue Oct 4, 2024 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@woodruffw
Copy link

Go version

go version go1.23.2 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/william/Library/Caches/go-build'
GOENV='/Users/william/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/william/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/william/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.23.2/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.23.2/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.2'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/william/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/k6/zk7vnkms1m73w0vrxjm7b2s40000gn/T/go-build3515925324=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

My colleague @DarkaMaul noticed an invalid GeneralizedTime encoding while tracking down some DER decoding failures in the responses produced by a Go implementation of an RFC 3161 Time Stamp Authority.

What did you see happen?

We observed DER encodings of GeneralizedTime objects with explicit timezone offsets, e.g.:

GeneralizedTime 2024-10-04 11:04:31 UTC+02:00

This is an invalid DER encoding of a GeneralizedTime, per the DER encoding rules defined in ITU-T X.690. In particular, DER requires that all GeneralizedTime encodings be UTC time with the Z designator per X.690 11.7.1:

The encoding shall terminate with a "Z", as described in the ITU-T Rec. X.680 | ISO/IEC 8824-1 clause on GeneralizedTime.

(Ref: https://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf, page 19)

After looking into it, we determined that the codebase was using encoding/asn1's Marshal implementation, in particular for marshalling time.Time objects into GeneralizedTime encodings.

For example:

// eContent within SignedData is TSTInfo
type tstInfo struct {
	// .. snip
	Time           time.Time        `asn1:"generalized"`
	// .. snip
}

Permalink: https://github.com/digitorus/timestamp/blob/220c5c2851b7435eea999de3daa773601a7ca126/rfc3161_struct.go#L57

We then checked the underlying Marshal implementation and its GeneralizedTime helper (appendGeneralizedTime), and confirmed that it emits a relative offset instead of normalizing to UTC when the origin time.Time is not already UTC:

func appendGeneralizedTime(dst []byte, t time.Time) (ret []byte, err error) {
	year := t.Year()
	if year < 0 || year > 9999 {
		return nil, StructuralError{"cannot represent time as GeneralizedTime"}
	}

	dst = appendFourDigits(dst, year)

	return appendTimeCommon(dst, t), nil
}

func appendTimeCommon(dst []byte, t time.Time) []byte {
	_, month, day := t.Date()

	dst = appendTwoDigits(dst, int(month))
	dst = appendTwoDigits(dst, day)

	hour, min, sec := t.Clock()

	dst = appendTwoDigits(dst, hour)
	dst = appendTwoDigits(dst, min)
	dst = appendTwoDigits(dst, sec)

	_, offset := t.Zone()

	switch {
	case offset/60 == 0:
		return append(dst, 'Z')
	case offset > 0:
		dst = append(dst, '+')
	case offset < 0:
		dst = append(dst, '-')
	}

	offsetMinutes := offset / 60
	if offsetMinutes < 0 {
		offsetMinutes = -offsetMinutes
	}

	dst = appendTwoDigits(dst, offsetMinutes/60)
	dst = appendTwoDigits(dst, offsetMinutes%60)

	return dst
}

Ref: https://cs.opensource.google/go/go/+/refs/tags/go1.23.2:src/encoding/asn1/marshal.go;l=405-448

Based on the blame, this offset encoding has been present since at least 2011 and possibly earlier.

What did you expect to see?

We expected encoding/asn1 to produce only valid DER encodings, which in this case means producing a GeneralizedTime with only a Z timezone component, and no relative timezone offsets.

To achieve this, we believe the Marshal implementation can be tweaked to call UTC() before performing encoding, which would normalize the time.Time into UTC form. The special-casing around relative offsets could then be removed entirely, as all encoded times would be UTC.

Similarly, we believe (but haven't concretely observed) that encoding/asn1's Unmarshal accepts invalid DER encodings of GeneralizedTimes, per its format string:

// parseGeneralizedTime parses the GeneralizedTime from the given byte slice
// and returns the resulting time.
func parseGeneralizedTime(bytes []byte) (ret time.Time, err error) {
	const formatStr = "20060102150405.999999999Z0700"
	s := string(bytes)

	if ret, err = time.Parse(formatStr, s); err != nil {
		return
	}

	if serialized := ret.Format(formatStr); serialized != s {
		err = fmt.Errorf("asn1: time did not serialize back to the original value and may be invalid: given %q, but serialized as %q", s, serialized)
	}

	return
}

Ref: https://cs.opensource.google/go/go/+/refs/tags/go1.23.2:src/encoding/asn1/asn1.go;l=368-383

If our understanding of time.Parse is correct, this will admit multiple invalid DER encodings:

  • The fractional component .999999999 will allow trailing zeroes, which are allowed in BER but not in DER;
  • The timezone component Z0700 allows both Z and relative timezone offsets, when only Z is allowed in DER.
@woodruffw
Copy link
Author

Here is the related downstream bug we observed: sigstore/timestamp-authority#846

@mknyszek
Copy link
Contributor

mknyszek commented Oct 4, 2024

CC @golang/security via https://dev.golang.org/owners

@mknyszek mknyszek added this to the Backlog milestone Oct 4, 2024
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 4, 2024
@woodruffw
Copy link
Author

#19890 looks like a pre-existing report of the invalid DER encoding with Marshal, although I can't find a matching pre-existing for the overly permissive parsing in Unmarshal.

@mateusz834
Copy link
Member

I guess that the same issue also affects the (*cryptobyte.Builder).AddASN1GeneralizedTime and (*cryptobyte.String).ReadASN1GeneralizedTime in the x/crypto/cryptobyte package.

@woodruffw
Copy link
Author

I guess that the same issue also affects the (*cryptobyte.Builder).AddASN1GeneralizedTime and (*cryptobyte.String).ReadASN1GeneralizedTime in the x/crypto/cryptobyte package.

Yep, looks like it:

const generalizedTimeFormatStr = "20060102150405Z0700"

// AddASN1GeneralizedTime appends a DER-encoded ASN.1 GENERALIZEDTIME.
func (b *Builder) AddASN1GeneralizedTime(t time.Time) {
	if t.Year() < 0 || t.Year() > 9999 {
		b.err = fmt.Errorf("cryptobyte: cannot represent %v as a GeneralizedTime", t)
		return
	}
	b.AddASN1(asn1.GeneralizedTime, func(c *Builder) {
		c.AddBytes([]byte(t.Format(generalizedTimeFormatStr)))
	})
}

Ref: https://cs.opensource.google/go/x/crypto/+/master:cryptobyte/asn1.go;l=107-118;drc=a0819fbb0244af70857f03b6984e1d4f93e6cabf?q=AddASN1GeneralizedTime&ss=go%2Fx%2Fcrypto

@rolandshoemaker
Copy link
Member

Huh, I'm surprised this has survived for so long. We should probably fix it 🙃.

@woodruffw
Copy link
Author

This CL (from the related issue above) looks like it would address the Marshal side of things, although that leaves the Unmarshal side: https://go-review.googlesource.com/c/go/+/146117

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants