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

woodruffw commented 1 month ago

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.

gabyhelp commented 1 month ago

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

woodruffw commented 1 month ago

Here is the related downstream bug we observed: https://github.com/sigstore/timestamp-authority/issues/846

mknyszek commented 1 month ago

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

woodruffw commented 1 month ago

https://github.com/golang/go/issues/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 commented 1 month ago

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

woodruffw commented 1 month ago

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 commented 1 month ago

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

woodruffw commented 1 month ago

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

golang / go