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

x/crypto/cryptobyte: cannot read boolean values #26565

Closed
szank opened this issue Jul 24, 2018 · 4 comments
Closed

x/crypto/cryptobyte: cannot read boolean values #26565

szank opened this issue Jul 24, 2018 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@szank
Copy link

szank commented Jul 24, 2018

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.

@gopherbot gopherbot added this to the Unreleased milestone Jul 24, 2018
@bcmills
Copy link
Contributor

bcmills commented Jul 24, 2018

CC: @FiloSottile

@samuele-andreoli
Copy link

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.

@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 22, 2019
@tahkapaa
Copy link

tahkapaa commented May 10, 2020

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
Copy link
Member

Fixed by golang.org/cl/233161.

@golang golang locked and limited conversation to collaborators Oct 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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

7 participants