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.

Sign up for GitHub

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: time.Time values can be encoded incorrectly #64892

Closed
ecnepsnai opened this issue Dec 29, 2023 · 6 comments
Closed

encoding/asn1: time.Time values can be encoded incorrectly #64892

ecnepsnai opened this issue Dec 29, 2023 · 6 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Security WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@ecnepsnai
Copy link

ecnepsnai commented Dec 29, 2023 •
edited

Go version

1.21.5

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

GO111MODULE='on'
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/ian/Library/Caches/go-build'
GOENV='/Users/ian/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/ian/Developer/go/pkg/mod'
GOOS='darwin'
GOPATH='/Users/ian/Developer/go'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go/current'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/current/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.5'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
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/72/2kjqjqwd61d83xhhzwptf_sc0000gn/T/go-build3674955566=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

The encoding/asn1 package supports marshalling time.Time structures to either a ASN.1 UTCTime or GeneralizedTime. The behaviour is controlled by using struct tags or passing parameters with asn1.MarshalWithParams, but by default the package will use UTCTime. (Aside: this seems to be a really poor choice for a default behaviour, considering UTCTime is not Y2K compliant)

However, it's possible for the asn1 package to encode an invalid GeneralizedTime value without producing an error.

Take the following example:

t := time.Date(2023, 12, 28, 16, 49, 55, 00, time.UTC)
tVal, err := asn1.MarshalWithParams(t, "tag:24")
if err != nil {
	panic(err)
}
// tVal = 98 0d 32 33 31 32 32 38 31 36 34 39 35 35 5a (..231228164955Z)

Here, we're passing the ASN1 tag for GeneralizedTime (24), but the value of tVal only uses 2 digits for the year because it's actually being encoded as UTCTime.

Despite me telling the package that I want to use GeneralizedTime, I have to explicitly tell it with a second parameter:

t := time.Date(2023, 12, 28, 16, 49, 55, 00, time.UTC)
tVal, err := asn1.MarshalWithParams(t, "tag:24,generalized")
if err != nil {
	panic(err)
}
// tVal = 18 0f 32 30 32 33 31 32 32 38 31 36 34 39 35 35 5a (..20231228164955Z)

Go playground: https://go.dev/play/p/WSxtB5cOMaQ

What did you expect to see?

I expected the asn1 package to encode the time using GeneralizedTime as I had specified using the ASN1 tag value. I also expected that the default encoding would be the Y2K-compliant GeneralizedTime. Perhaps a separate discussion is needed as to if this should be changed going forward.

What did you see instead?

A confusion between a ASN1 field that is tagged as GeneralizedTime but actually has a UTCTime value.
@mateusz834
Copy link
Member

When outside of the UTCTime range, it will be encoded as GeneralizedTime.

go/src/encoding/asn1/marshal.go

Lines 655 to 659 in 988b718

case TagUTCTime:
if params.timeType == TagGeneralizedTime || outsideUTCRange(v.Interface().(time.Time)) {
tag = TagGeneralizedTime
}
}

Despite me telling the package that I want to use GeneralizedTime, I have to explicitly tell it with a second parameter:

t := time.Date(2023, 12, 28, 16, 49, 55, 00, time.UTC)
tVal, err := asn1.MarshalWithParams(t, "tag:24,generalized")
if err != nil {
	panic(err)
}

Doesn't that make sense? There might be an implicit tag used, so tag:24 should default to UTCTime (as done currently).

@mateusz834 mateusz834 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 29, 2023
@ecnepsnai
Copy link
Author

ecnepsnai commented Dec 30, 2023 •
edited

Hi @mateusz834, thanks for replying!

So where my confusion comes from, and why I believe this is a bug, is that because I had specified the tag for GeneralizedTime (24) I expected the asn1 package to use that time format.

It doesn't make sense to me that you would want a field tagged as GeneralizedTime but with a UTCTime value, when they aren't compatible.

I feel the asn1 package should reference a tag value (if specified) when determining what time format to use, if no format was explicitly set.

@mateusz834
Copy link
Member

From the encoding/asn1 docs:

tag:x specifies the ASN.1 tag number; implies ASN.1 CONTEXT SPECIFIC

so this is not a 24 + Universal bits (24), but rather 24 + Context-specific bits (152).

@mateusz834
Copy link
Member

mateusz834 commented Dec 30, 2023 •
edited

But i think that this behavior is wrong, in my opinion this should use tag 24 + Context-specific bit, rather than 24 + Universal bit.

t := time.Date(2023, 12, 28, 16, 49, 55, 00, time.UTC)
tVal, err := asn1.MarshalWithParams(t, "tag:24,generalized")
if err != nil {
	panic(err)
}
// tVal = 18 0f 32 30 32 33 31 32 32 38 31 36 34 39 35 35 5a (..20231228164955Z)

EDIT: actually it returns the correct tag, see https://go.dev/play/p/xc0pfnyWyXY

@mateusz834 mateusz834 added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 31, 2023
@bcmills
Copy link
Contributor

bcmills commented Jan 2, 2024

(attn @FiloSottile @rolandshoemaker @golang/security per https://dev.golang.org/owners)

@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@gopherbot gopherbot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2024
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. Security WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ecnepsnai @bcmills @gopherbot @mateusz834