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

LedgerCosmos.getAddressPubKeySECP256K1 could use string operations as they are for properly checking runes instead of bytes #54

Closed
odeke-em opened this issue Oct 16, 2023 · 1 comment

Comments

@odeke-em
Copy link
Contributor

This code takes a string and then converts it to a byteslice then manually runs some operations on it

hrpBytes := []byte(hrp)
for _, b := range hrpBytes {
if !validHRPByte(b) {
return nil, "", errors.New("all characters in the HRP must be in the [33, 126] range")
}
}

and then

message := append(header, byte(len(hrpBytes)))
message = append(message, hrpBytes...)

From reading the above, it seems that Ledger ONLY deals with ASCII but I've seen them display emoji so seems that they are capable of using Unicode for future purposes like Unicode passphrases or keys, in which I could use runes (Unicode code point with a very high range) that are invalid but when turned into bytes (very narrow range) overflow and pass the error checks

Suggestions

Use string operations

func validHRPRune(r rune) bool {
	// https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki
	return r >= 33 && r <= 126
}

func (ledger *LedgerCosmos) getAddressPubKeySECP256K1(bip32Path []uint32, hrp string... {
   ...
   if strings.ContainsFunc(hrp, func(r rune) bool { return !validHRPRune(r) }) {
      return nil, "", errors.New("all runes in the HRP must be in the [33, 126] range")
   }

  ...
  message := append(header, byte(len(hrp)))
  message = append(message, hrp...)

Use bytes.ContainsFunc

   if bytes.ContainsFunc(hrpBytes, invalidHRPByte) {
      return nil, "", errors.New("all runes in the HRP must be in the [33, 126] range")
   }

/cc @elias-orijtech @julienrbrt

@jleni
Copy link
Member

jleni commented Sep 2, 2024

The restriction is due to bech32 definition in BIP173 and it is not purely a representation restriction. For this reason, code should be restricted to byte instead of runes.

https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#bech32

@jleni jleni closed this as completed Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants