x/crypto/cryptobyte: support reading implicitly-tagged values

[aarongable]

Go version

go version go1.21.5 linux/amd64

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

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/aaron/.cache/go-build'
GOENV='/home/aaron/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/aaron/.local/share/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/aaron/.local/share/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.5'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3470374486=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Attempted to use ReadOptionalASN1Boolean to read the onlyContainsCACerts and onlyContainsUserCerts fields of a CRL's IssuingDistributionPoint extension.

You can see my attempt, failure, and resulting investigation here. I also have a Go playground which minimally reproduces the situation:

func example(extBytes []byte) error {
	idpExt := cryptobyte.String(extBytes)
	if !idpExt.ReadASN1(&idpExt, asn1.SEQUENCE) {
		return errors.New("Failed to read IssuingDistributionPoint distributionPoint")
	}

	var onlyContainsCACerts bool
	onlyContainsCACertsTag := asn1.Tag(2).ContextSpecific()
	if !idpExt.ReadOptionalASN1Boolean(&onlyContainsCACerts, onlyContainsCACertsTag, false) {
		return errors.New("Failed to read IssuingDistributionPoint onlyContainsCACerts")
	}

	return nil
}

func main() {
	// IMPLICIT encoding
	// Sequence of length 3, containing...
	// Context-specific tag 2 of length 1, containing...
	// A byte of all ones (the representation of the boolean TRUE).
	idpExtImplicit := []byte{0x30, 0x03, 0x82, 0x01, 0xFF}
	log.Println(example(idpExtImplicit))

	// EXPLICIT encoding
	// Sequence of length 5, containing...
	// Context-specific tag 2 of length 3, containing...
	// Boolean of length 1, containing...
	// A byte of all ones (the representation of the boolean TRUE).
	idpExtExplicit := []byte{0x30, 0x05, 0x82, 0x03, 0x01, 0x01, 0xFF}
	log.Println(example(idpExtExplicit))
}

What did you expect to see?

I expected ReadOptionalASN1Boolean to succeed when reading the onlyContainsCACerts field. That field is an optional boolean, so ReadOptionalASN1Boolean should successfully read it.

What did you see instead?

ReadOptionalASN1Boolean fails when attempting to read the onlyContainsCACerts field. This is because ReadOptionalASN1Boolean only works for explicitly-tagged fields. It's expecting to get a five-byte sequence, as in my second example above: a context-specific tag and length, wrapping a normal boolean tag, length, and value.

But the IssuingDistributionPoint extension is defined (in both X.509 and RFC 5280) in an ASN.1 module that sets DEFINITIONS IMPLICIT TAGS, so these fields are implicitly-tagged, not explicitly-tagged. They're only correctly encoded as a three bytes: a context-specific tag and length followed immediately by the single-byte boolean itself.

There are lots of IMPLICIT fields in x509, so it's somewhat concerning that the cryptobyte parser only has facilities for parsing EXPLICIT fields.

Note: Perhaps I should have expected the behavior I saw, because the documentation of the methods in question does say (emphasis added) "ReadOptionalASN1Boolean attempts to read an optional ASN.1 BOOLEAN explicitly tagged with tag...". But I think this single word is easy to miss, especially when it's being used as a term of art rather than as plain english. And regardless of the behavior of the existing methods, I still think it would be valuable to have methods which can parse implicitly-tagged fields.

[thanm]

Thanks for the report.

[thanm]

@FiloSottile @rolandshoemaker @golang/security per owners

[rolandshoemaker]

I don't think we want to add transparent support for IMPLICIT values, if you expect to parse an EXPLICIT field, but get an IMPLICIT field, that seems like it should be an error (as it is now).

We've mostly avoided adding methods for all of the IMPLICIT variations of the parsing methods, since we've not really run into them all that much as of yet. The one exception to this so far is AddASN1Int64WithTag/ReadASN1Int64WithTag.

We could add ReadASN1BooleanWithTag/ReadOptionalASN1BooleanWithTag (...or something with a better name), but I feel like going down this path risks the combinatorial explosion of adding IMPLICIT versions of everything we currently have.

I cannot form the exact API in my mind right now, but it seems like there is perhaps a nicer composable style API we could implement here that would allow us to just provide all of the pieces and allow the user to build the thing they want (without having to re-implement optional semantics etc).

[aarongable]

I don't think we want to add transparent support for IMPLICIT values, if you expect to parse an EXPLICIT field, but get an IMPLICIT field, that seems like it should be an error (as it is now).

Absolutely agreed, parsing explicit vs implicit should be segregated somehow so you don't accidentally get the wrong thing.

it seems like there is perhaps a nicer composable style API we could implement

Thinking out loud: What if the asn1 subpackage's Tag type had a facility for this?

It could become a struct, rather than just a bare uint8. From there the simple solution would be to have it carry an explicit vs implicit boolean, and methods in cryptobyte could change their behavior depending on that. But perhaps an even more exciting solution would be something like

asn1.Tag(2).ContextSpecific() // produces 0x82
asn1.Tag(2).ContextSpecific().Explicit(asn1.BOOLEAN) // produces 0x82 <placeholder> 0x01

and then the cryptobyte methods would make sure that the whole tag (both layers of it, in the explicit case) matches, and would use .Peek() to ensure that the next length byte is exactly 2 less than the length byte skipped over by the <placeholder>.

[rolandshoemaker]

Yeah Tag would probably make the most sense, although we've somewhat backed ourselves into a corner by making it just a typed uint8, so there isn't really an obvious place to hide the relevant information.

[aarongable]

Maybe the cryptobyte methods are updated to take a Tag interface, the uint8 Tag type is given a dummy method to implement that interface, and the .Explicit(inner asn1.Tag) method returns a different type which also implements that interface?

Of course, a downside of this approach is that it means that default (i.e. current) tags would be assumed to be Implicit, which is a change from current behavior :/

[rolandshoemaker]

Coming back to this, the API design of this package is clearly not ideal, and isn't particularly conducive to making these kinds of changes cleanly. There are a number of significant changes I'd like to make, but I think those need to be put off until we have the resources to do a v2 of the package.

Until we can completely overhaul the package, I think tolerating the combinatorial explosion in the number of methods is mostly acceptable. It's not exactly pretty, but this is an explicit parser, and having a ton of methods that just do what they say is probably fine.

In terms of a concrete proposal, essentially for each of the existing {Add,Read}ASN1{type}, we'll add a {Add,Read}ASN1{type}WithTag and {Add,Read}OptionalASN1{type}WithTag method that support parsing the implicitly tagged types. We could also do this in a limited manner (i.e. start by only adding support for bools). Ideally we should be able to significantly reuse code in the implementation, but this is likely to still be somewhat painful.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rolandshoemaker]

For just the Boolean type, the new APIs would be:

func (b *Builder) AddASN1BooleanWithTag(v bool, tag asn1.Tag)

func (s *String) ReadASN1BooleanWithTag(out *bool, tag asn1.Tag) bool
func (s *String) ReadOptionalASN1BooleanWithTag(out *bool, tag asn1.Tag, defaultValue bool) bool

Note ReadOptionalASN1BooleanWithTag has the same signature as the existing ReadOptionalASN1Boolean, but would work for implicitly tagged fields, rather than explicitly tagged fields.

There is an open question of whether we want to implement these APIs for the rest of the ASN1 types (BitString, Bytes, Element, Enum, GeneralizedTime, Integer, ObjectIdentifier, and UTCTime). Unless there is a current need for them (@aarongable have you run into this same problem for the other types?), I think it's reasonable to leave them out, and just implement the Boolean API for now, and then revisit this again for other types when they are needed.

[aarongable]

This API makes sense to me.

I don't think there's a strong argument for implementing the same for all types -- for example, cryptobyte currently only has "ReadOptionalASN1Foo" methods for Boolean, Integer, and OctetString, so there's prior art for not providing every possible permutation of a method.

That said, RFC 5280 does have examples of some of those types:

• The GeneralSubtrees types (used in NameConstraints extensions) has "minimum" and "maximum" fields, each of which are implicitly-tagged optional INTEGERs.
• The DistributionPoint type (used in CRLDistributionPoints extensions) has a "reasons" field which is an implicitly-tagged optional BIT STRING.
• The top-level "issuerUniqueID" and "subjectUniqueID" fields on a TBSCertificate are also both implicitly-tagged optional BIT STRINGs (but they have been profiled out of the modern PKI).
• The AuthorityKeyIdentifier type has the "keyIdentifier" field which is an implicitly-tagged optional OCTET STRING. (The go x509 lib reads this field today using a combo of PeekASN1Tag + ReadASN1, because ReadOptionalASN1OctetString doesn't work here.)

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The new API is in #64811 (comment).

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The new API is in #64811 (comment).

[pgporada]

@rolandshoemaker I have an implementation here if it helps.