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

btcutil/base58: fix bug with range #1799

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

egonelbre
Copy link

The decoding was using a range over the string instead of iterating
over the bytes.

The decoding was using a range over the string instead of iterating
over the bytes.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1771996489

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 79.676%

Totals Coverage Status
Change from base Build 1753768655: 0.0%
Covered Lines: 1231
Relevant Lines: 1545

💛 - Coveralls

@schjonhaug
Copy link

Is this PR fixing the crash that occurs if you try to decode something with an emoji? What is the allowed character set here?

For example, using example-Decode, if you try to decode a string with 🤡, it’ll crash:

package main

import (
	"fmt"

	"github.com/btcsuite/btcd/btcutil/base58"
)

func main() {
	// Decode example modified base58 encoded data.
	encoded := "25JnwSn7XKfNQ🤡"
	decoded := base58.Decode(encoded)

	// Show the decoded data.
	fmt.Println("Decoded Data:", string(decoded))

}

Result:

Output:

panic: runtime error: index out of range [10095] with length 256

goroutine 1 [running]:
github.com/btcsuite/btcd/btcutil/base58.Decode({0x49fff5, 0x10})
	/tmp/gopath2191683068/pkg/mod/github.com/btcsuite/btcd/btcutil@v1.1.0/base58/base58.go:58 +0x395
main.main()
	/tmp/sandbox2173415571/prog.go:12 +0x29

@egonelbre
Copy link
Author

Yes, it's a fix for runes that are larger 0xFF, which does include emoji. The valid alphabet is 123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz -- i.e. base58 alphabet used by bitcoin.

@ssnickolay
Copy link

Why this is not merged yet? I'm using the lib to check BTC address is valid and the issue could crash the app ;[

@chappjc
Copy link
Contributor

chappjc commented Apr 23, 2022

Similar fix downstream: https://github.com/decred/base58/blob/5b84bad1ebfd283724c012ec3e0e02f6f80b0ee2/base58.go#L25
Worth merging this PR or a direct port of that solution.

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

Successfully merging this pull request may close these issues.

5 participants