Navigation Menu

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

Jump to bottom

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
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@jsha
Copy link

jsha commented Jan 29, 2020 •
edited 

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 cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 7, 2020
@cagedmantis
Copy link
Contributor

/cc @FiloSottile @agl

@FiloSottile
Copy link
Contributor

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

@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 19, 2020
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 19, 2020
@jsha
Copy link
Author

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 FiloSottile added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Feb 19, 2020
@FiloSottile
Copy link
Contributor

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
@rolandshoemaker
encoding/asn1: only accept minimally encoded base 128 encoded integers
0ca00e6 
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
@rolandshoemaker
encoding/asn1: only accept minimally encoded base 128 integers
c27be6c 
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 rolandshoemaker mentioned this issue Apr 6, 2020
Closed
rolandshoemaker added a commit to rolandshoemaker/go that referenced this issue Apr 6, 2020
@rolandshoemaker
encoding/asn1: only accept minimally encoded base 128 integers
b525816 
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

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

@sebastien-rosset sebastien-rosset mentioned this issue Apr 29, 2020
Open
@odeke-em
Copy link
Member

odeke-em commented Jun 30, 2020 •
edited

@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

Yup, will do.

@nickysemenza nickysemenza mentioned this issue Nov 13, 2020
Closed
@golang golang locked and limited conversation to collaborators Jun 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

Successfully merging a pull request may close this issue.

encoding/asn1: only accept minimally encoded base 128 integers rolandshoemaker/go
6 participants
@cagedmantis @jsha @FiloSottile @odeke-em @rolandshoemaker @gopherbot