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: accepts non-minimal OID encoding #36881

Closed
jsha opened this issue Jan 29, 2020 · 7 comments
Closed

encoding/asn1: accepts non-minimal OID encoding #36881

jsha opened this issue Jan 29, 2020 · 7 comments

Comments

@jsha
Copy link

@jsha jsha commented Jan 29, 2020

go version go1.13.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jsha/.cache/go-build"
GOENV="/home/jsha/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jsha/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/jsha/go1.13"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/jsha/go1.13/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build221529794=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Passed an invalidly-encoded OID to asn1.Unmarshal

What did you expect to see?

Parse error.

What did you see instead?

Successful parse.

Here's an example program demonstrating the problem, along with a reference to the ASN.1 spec:

https://play.golang.org/p/ETqZ6Kxz16G

package main

import (
	"encoding/asn1"
	"encoding/hex"
	"fmt"
	"log"
)

func main() {
	var o asn1.ObjectIdentifier
	// Correct encoding; each component of the ObjectIdentifier is encoded
	// minimally.
	s, err := hex.DecodeString("06092a864886f70d01010b")
	if err != nil {
		log.Fatal(err)
	}
	_, err = asn1.Unmarshal(s, &o)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(o)
	// Incorrect encoding; the 0x80 byte encodes a zero prefix in base 128, which
	// violate's X.690's requirement to minimally-encode OIDs (which applies
	// to both BER and DER). Ref: X.690 2015, §8.19.2 "The subidentifier shall be
	// encoded in the fewest possible octets, that is, the leading octet of the
	// subidentifier shall not have the value [0x80]"
	s, err = hex.DecodeString("060a2a80864886f70d01010b")
	if err != nil {
		log.Fatal(err)
	}
	_, err = asn1.Unmarshal(s, &o)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(o)
}
@cagedmantis cagedmantis added this to the Backlog milestone Feb 7, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Feb 7, 2020

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Feb 19, 2020

Do we know of frequent occurrences of this in the wild? (Trying to understand if we'll cause breakage.)

@jsha
Copy link
Author

@jsha jsha commented Feb 19, 2020

I haven't seen this in the wild, but I also haven't looked. Just occurred to me to try out Go's behavior when I read that section of the spec.

@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Feb 19, 2020
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Feb 19, 2020

Let's fix this early so it has time to bake. I'd take a CL for it!

rolandshoemaker added a commit to rolandshoemaker/go that referenced this issue Apr 6, 2020
Reject base 128 encoded integers that aren't using minimal encoding,
specifically if the leading octet of an encoded integer is 0x80. This
only affects parsing of tags and OIDs, both of which expect this
encoding (see X.690 8.1.2.4.2 and 8.19.2).

Fixes golang#36881

Change-Id: I969cf48ac1fba7e56bac334672806a0784d3e123
rolandshoemaker added a commit to rolandshoemaker/go that referenced this issue Apr 6, 2020
Reject base 128 encoded integers that aren't using minimal encoding,
specifically if the leading octet of an encoded integer is 0x80. This
only affects parsing of tags and OIDs, both of which expect this
encoding (see X.690 8.1.2.4.2 and 8.19.2).

Fixes golang#36881

Change-Id: I969cf48ac1fba7e56bac334672806a0784d3e123
rolandshoemaker added a commit to rolandshoemaker/go that referenced this issue Apr 6, 2020
Reject base 128 encoded integers that aren't using minimal encoding,
specifically if the leading octet of an encoded integer is 0x80. This
only affects parsing of tags and OIDs, both of which expect this
encoding (see X.690 8.1.2.4.2 and 8.19.2).

Fixes golang#36881

Change-Id: I969cf48ac1fba7e56bac334672806a0784d3e123
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 6, 2020

Change https://golang.org/cl/227320 mentions this issue: encoding/asn1: only accept minimally encoded base 128 integers

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jun 30, 2020

@rolandshoemaker @FiloSottile @jsha could we perhaps send a release note documenting this update? It is a new change that'll surprise some folks.

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Jun 30, 2020

Yup, will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.