Skip to content

encoding/asn1: ObjectIdentifier Unmarshal error if SubOID > MaxInt32 on 64bit machine too #71679

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

Open
thebinary opened this issue Feb 12, 2025 · 5 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@thebinary
Copy link

Go version

go version go1.24.0 darwin/amd64

Output of go env in your module/workspace:

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/Users/thebinary/Library/Caches/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/Users/thebinary/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/5y/zx7mp2554txbzk7870s69nd00000gp/T/go-build178868391=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='/Volumes/DataHD/Builds/go/src/local/mysnmp/go.mod'
GOMODCACHE='/Volumes/DataHD/Builds/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Volumes/DataHD/Builds/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/thebinary/Library/Application Support/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.24.0'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

On a 64-bit machine, I tried to Marshal and then Unmarshal the asn1.ObjectIdentifier with all SubOIDs <= MaxInt32 and some SubOIDs > MaxInt32 separately

https://go.dev/play/p/4CDcLlS1LLm

What did you see happen?

For ObjectIdentifier with SubOIDs <= MaxInt32, Marshal and Unmarshal both works without any error.
But, for ObjectIdentifier with some SubOIDs > MaxInt32, Mashal works without any error but Unmarshal throws an error.

OUTPUT:

==== All SubOIDs <= MaxInt32 =====
INPUT=1.2.2147483647
Marshal bytes=[6 6 42 135 255 255 255 127] ; error=<nil>
Unmarshal OID=1.2.2147483647 ; error=<nil>

==== One SubOID > MaxInt32 =====
INPUT=1.2.2147483648
Marshal bytes=[6 6 42 136 128 128 128 0] ; error=<nil>
Unmarshal OID=1.2.0.0.0.0.0 ; error=asn1: structure error: base 128 integer too large

What did you expect to see?

Marshal works, so Unmarshal should also work.

EXPECTED OUTPUT:

==== All SubOIDs <= MaxInt32 =====
INPUT=1.2.2147483647
Marshal bytes=[6 6 42 135 255 255 255 127] ; error=<nil>
Unmarshal OID=1.2.2147483647 ; error=<nil>

==== One SubOID > MaxInt32 =====
INPUT=1.2.2147483648
Marshal bytes=[6 6 42 136 128 128 128 0] ; error=<nil>
Unmarshal OID=1.2.2147483648 ; error=<nil>
@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Feb 12, 2025
@thebinary
Copy link
Author

thebinary commented Feb 12, 2025

if ret64 > math.MaxInt32 {

                        // Ensure that the returned value fits in an int on all platforms
			if ret64 > math.MaxInt32 {
				err = StructuralError{"base 128 integer too large"}
			}

I find this check is what is limiting the Unmarshal.

The following patch resolves the issue

diff --git a/src/encoding/asn1/asn1.go b/src/encoding/asn1/asn1.go
index 488fb9b1e0..42d4e69b94 100644
--- a/src/encoding/asn1/asn1.go
+++ b/src/encoding/asn1/asn1.go
@@ -319,7 +319,7 @@ func parseBase128Int(bytes []byte, initOffset int) (ret, offset int, err error)
                if b&0x80 == 0 {
                        ret = int(ret64)
                        // Ensure that the returned value fits in an int on all platforms
-                       if ret64 > math.MaxInt32 {
+                       if ret64 > math.MaxInt {
                                err = StructuralError{"base 128 integer too large"}
                        }
                        return

This will not resolve the issue for 32-bit architecture. But Marshal and Unmarshal will be consistent on both the architecture for suboids greater than 2^31-1 i.e. working on 64-bit but not in 32-bit (which I suppose does wont work right now as well since ObjectIdentifier itself is defined as []int).

@dmitshur
Copy link
Member

CC @golang/security.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 12, 2025
@rolandshoemaker
Copy link
Member

We limit Unmarshal to 32 bit OID components so that we get consistent behavior across platforms, as such I don't expect we will change this to support larger components only on systems with 64 bit integers. Perhaps we should make Marshal fail just so we have parity, but that would be a breaking change.

Really, if you care about larger OIDs with large components, you should use the crypto/x509.OID type, which was explicitly designed to solve this problem.

@thebinary
Copy link
Author

thebinary commented Feb 13, 2025

Thank you for the info on crypto/x509.OID type.
May be including the info in documentation of asn1.ObjectIdentifier can be beneficial, for others as well, who may stumble upon this case.

Got your point. But there seems to be a catch here, in go ecosystem I have found Marshal and Unmarshal being complimentary always, which breaks in this case and so is an unexpected behavior. Which I expect is at least documented.

And for documenting unxpected behavior, I think we have two options:

  1. Make no implementation changes and document the diff in Marshal and Unmarshal of asn1.ObjectIdentifier and the info to use crypto/x509.OID for cases where SubOIDs may be greater than MaxInt32.

  2. Allow Unmarshal in 64-bit machines like Marshal (fixing inconsistency) and document the diff in asn1.ObjectIdentifier on 32/64-bit machines and the suggestion to use crypto/x509.OID for architecture consistency for cases where SubOIDs may be greater than MaxInt32.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. 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

4 participants