x/crypto/cryptobyte: cannot read boolean values

[szank]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10.3 darwin/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

I tried to decode a boolean asn.1 value using the cryptobyte package.
Boolean "True" value encoded using the stdlib asn.1 encoder is 0x0101FF tag 0x01 - boolean value
length 0x01 - 1 byte and the value 0xff - 0x00 is false, everything else if true.

When decoding the value using the cryptobyte package I have encountered some issues.

1. Using ReadASN1Boolean - it expects tag 0x02 - Integer instead of tag 0x01 - Boolean, so it returns false when reading 0x0101FF value

2. Using ReadOptionalASN1Boolean - it correctly expects 0x01 tag, and reads the value into a child cryptobyte string (that would contain x0FF bytes in my case), then in case the value exists, proceeds to read the remainder of the input which is not correct, it should examine the value read into the child cryptobyte string and return without advancing further.

Both bugs are trivial to fix and I am happy to submit a patch.

[bcmills]

CC: @FiloSottile

[samuele-andreoli]

Hi, I'm using the cryptobyte library and I stumbled upon this, too

What version of Go are you using?

$ go version
go1.11.5 darwin/amd64

Does this issue reproduce with the latest release?
yes

What did you do?
I tried to decode an optional boolean asn1 value using the cryptobyte package. I expected the String "0x01 0x01 0xFF" to be decoded as "true", but ReadOptionalASN1Boolean fails to read the String.
The underlying issues pointed out by szank seem to be correct.

I can't provide a playground example, so I'll just copy paste it here

package main

import (
	"fmt"

	"golang.org/x/crypto/cryptobyte"
)

func main() {
	var b bool

	// This should read the booleanDER successfully and set 'b' to TRUE
	//
	// It fails to read the optional BOOLEAN because it updates the String to check
	// the presence and then tries to read from the updated string instead of the
	// extracted child
	var booleanDER = cryptobyte.String{0x01, 0x01, 0xFF}

	if !booleanDER.ReadOptionalASN1Boolean(&b, false) {
		fmt.Println("Couldn't read booleanDER")
	} else {
		fmt.Printf("Read optional booleanDER: %t\n", b)
	}

	fmt.Println("===")

	// This should read the boolean and set 'b' to FALSE. compositeDER should then
	// be updated to point at the start of the encoded integer.
	//
	// It actually checks the presence of the boolean with a BOOLEAN tag, then
	// proceeds reading the value from the updated String as above.
	// Since ReadASN1Boolean uses the INTEGER tag, it reads the following INTEGER
	// and sets 'b' to TRUE (0xFF), updating the String again.
	// At the end of the operation compositeDER has no bytes left, due to the double
	// update.
	var compositeDER = cryptobyte.String{0x01, 0x01, 0x00, 0x02, 0x01, 0xFF}

	if !compositeDER.ReadOptionalASN1Boolean(&b, false) {
		fmt.Println("Couldn't read compositeDER")
	} else {
		fmt.Printf("Read compositeDER: %t\n", b)
		fmt.Printf("Remaining bytes: \"%X\"\n", compositeDER)
	}
}

I also agree with the last point by szank. Both fixes should be trivial and I'd be happy to contribute.

[tahkapaa]

According to ITU-T X.690:

8.2.2 If the boolean value is:
FALSE
the octet shall be zero.
If the boolean value is
TRUE
the octet shall have any non-zero value, as a sender's option.

11 Restrictions on BER employed by both CER and DER
11.1 Boolean values
If the encoding represents the boolean value TRUE, its single contents octet shall have all eight bits set to one. (Contrast
with 8.2.2.)

So the problem for ReadASN1Boolean seems to be just the wrong tag, like said previously. Added a pull request for fixing this: golang/crypto#137.

[rolandshoemaker]

Fixed by golang.org/cl/233161.