Coming here from an audit of dependencies for cosmos-sdk, and per #15, I see this code

In there we have many of these patterns

Problem

While the code surely works, we could improve it:
a) It invokes defer right before a return, that defeats the purpose of a defer and return
b) It duplicates defer ledgerAPI.Close() all around

Suggestion

We could improve it using a named return and check on an error as Go supports that, and that'll make for less, yet simple, more efficient and less bug prone code

--- before.go	2020-11-24 13:34:07.000000000 -0800
+++ after.go	2020-11-24 13:34:13.000000000 -0800
@@ -1,26 +1,25 @@
-func FindLedgerCosmosUserApp() (*LedgerCosmos, error) {
+func FindLedgerCosmosUserApp() (_ *LedgerCosmos, err error) {
 	ledgerAPI, err := ledger_go.FindLedger()
-
 	if err != nil {
 		return nil, err
 	}
+	defer func() {
+		if err != nil {
+			ledgerAPI.Close()
+		}
+	}()
 
-	app := LedgerCosmos{ledgerAPI, VersionInfo{}}
+	app := &LedgerCosmos{ledgerAPI, VersionInfo{}}
 	appVersion, err := app.GetVersion()
-
 	if err != nil {
-		defer ledgerAPI.Close()
 		if err.Error() == "[APDU_CODE_CLA_NOT_SUPPORTED] Class not supported" {
 			return nil, fmt.Errorf("are you sure the Cosmos app is open?")
 		}
 		return nil, err
 	}
 
-	err = app.CheckVersion(*appVersion)
-	if err != nil {
-		defer ledgerAPI.Close()
+	if err := app.CheckVersion(*appVersion); err != nil {
 		return nil, err
 	}
-
-	return &app, err
-}
+	return app, err
+}

Only the master branch here is up to date with zondax/ledger-cosmos-go

If we would like to continue to maintain this fork, then we should tag a new release.

This code has what looks like C-style coding styles and is quite pedantic to declare the byteslice and the error, then switch between the various callers whose return signatures are ([]byte, error) instead of an immediate return

Suggested fix

func (ledger *LedgerCosmos) GetBip32bytes(bip32Path []uint32, hardenCount int) ([]byte, error) { 
 	switch major := ledger.version.Major; major { 
 	case 1: 
 		return GetBip32bytesv1(bip32Path, 3) 
 	case 2: 
 		return GetBip32bytesv2(bip32Path, 3) 
 	default: 
 		return nil, fmt.Errorf("Ledger version: %d is not supported", major) 
 	} 
 } 

/cc @elias-orijtech @julienrbrt

Please take a look at this length check

It clearly guards that length of the HRP should be less than 83, but the error message says hrp len should be <10

/cc @elias-orijtech @julienrbrt

go install -mod=readonly -tags "netgo ledger" -ldflags '-X github.com/cosmos/cosmos-sdk/version.Name=gaia -X github.com/cosmos/cosmos-sdk/version.ServerName=gaiad -X github.com/cosmos/cosmos-sdk/version.ClientName=gaiacli -X github.com/cosmos/cosmos-sdk/version.Version=0.0.0-14-gce9f584 -X github.com/cosmos/cosmos-sdk/version.Commit=ce9f584b36912ff9947f25e309f4b269785b0304 -X "github.com/cosmos/cosmos-sdk/version.BuildTags=netgo,ledger"' ./cmd/gaiacli

github.com/zondax/hid

In file included from E:\goSpace\pkg\mod\github.com\zondax\[email protected]\hid_enabled.go:40:0:
E:\goSpace\pkg\mod\github.com\zondax\[email protected]/hidapi/windows/hid.c: In function 'hid_enumerate':
E:\goSpace\pkg\mod\github.com\zondax\[email protected]/hidapi/windows/hid.c:431:9: warning: unknown option after '#pragma GCC diagnostic' kind [-Wpragmas]
#pragma GCC diagnostic ignored "-Wstringop-overflow"

When I build it on window, it occur above problems.

The code in

has 3 problems:
a) If the error's string matches "[APDU_CODE_CLA_NOT_SUPPORTED] Class not supported", the ledger won't be closed
b) It misuses defer; no point in invoking defer right before returning, without calling code that could potentially panic
c) If any error is encountered later in the chain, we have a leak, the Ledger won't be .Close()'d yet we shall return nil as per

Suggestion

This could could use named returns and checking if the err is non-nil then unconditionally invoking (*Ledger).Close() so:

--- before.go	2020-11-24 13:26:39.000000000 -0800
+++ after.go	2020-11-24 13:26:52.000000000 -0800
@@ -1,27 +1,26 @@
-func FindLedgerTendermintValidatorApp() (*LedgerTendermintValidator, error) {
+func FindLedgerTendermintValidatorApp() (_ *LedgerTendermintValidator, err error) {
 	ledgerAPI, err := ledger_go.FindLedger()
-
 	if err != nil {
 		return nil, err
 	}
+	defer func() {
+		if err != nil {
+			ledgerAPI.Close()
+		}
+	}()
 
-	ledgerCosmosValidatorApp := LedgerTendermintValidator{ledgerAPI}
-
+	ledgerCosmosValidatorApp := &LedgerTendermintValidator{ledgerAPI}
 	appVersion, err := ledgerCosmosValidatorApp.GetVersion()
-
 	if err != nil {
 		if err.Error() == "[APDU_CODE_CLA_NOT_SUPPORTED] Class not supported" {
-			return nil, fmt.Errorf("are you sure the Tendermint Validator app is open?")
+			err = fmt.Errorf("are you sure the Tendermint Validator app is open?")
 		}
-		defer ledgerAPI.Close()
 		return nil, err
 	}
 
 	req := RequiredTendermintValidatorAppVersion()
-	err = CheckVersion(*appVersion, req)
-	if err !=nil {
+	if err := CheckVersion(*appVersion, req); err != nil {
 		return nil, err
 	}
-
-	return &ledgerCosmosValidatorApp, err
+	return ledgerCosmosValidatorApp, err
 }

This switch

Suggestion

 default: 
 	return fmt.Errorf("Ledger device version: %d is not supported", version.Major)

/cc @elias-orijtech @julienrbrt

this repository depends on zondax/ledger-go but we maintain a fork of ledger-go in the cosmos org.

Reckon that if we want to keep maintaining these forks, we should merge:

• cosmos/ledger-go#6

and use cosmos/ledger-go as the canonical import here to ensure that we don't end up with import spaghetti.

Currently this repo shows that was forked from jsnathan/ledger-goclient but that's incorrect. Originally, it was forked from Zondax/ledger-cosmos-go but that repository will be deprecated and all the work and updates will be done here.
Check how to remove forked from legend in order to mark this repo as primary repository for this Go package.

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

and then

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

Related to cosmos/cosmos-sdk#4280

If we grep for fmt.Errorf, we can see all the uses are with static strings

$ git grep 'fmt.Errorf'
common.go:              return nil, fmt.Errorf("maximum bip32 depth = 10")
common.go:              return nil, fmt.Errorf("path should contain 5 elements")
user_app.go:                    return nil, fmt.Errorf("are you sure the Cosmos app is open?")
user_app.go:            return fmt.Errorf("App version is not supported")
user_app.go:            return nil, fmt.Errorf("invalid response")
user_app.go:            return nil, fmt.Errorf("App version is not supported")
user_app.go:            return nil, fmt.Errorf("App version is not supported")
user_app.go:                                    return nil, fmt.Errorf("Not enough tokens were provided")
user_app.go:                                    return nil, fmt.Errorf("Unexpected character in JSON string")
user_app.go:                                    return nil, fmt.Errorf("The JSON string is not a complete.")
user_app.go:                            return nil, fmt.Errorf(errorMsg)
user_app.go:                                    return nil, fmt.Errorf("Not enough tokens were provided")
user_app.go:                                    return nil, fmt.Errorf("Unexpected character in JSON string")
user_app.go:                                    return nil, fmt.Errorf("The JSON string is not a complete.")
user_app.go:                            return nil, fmt.Errorf(errorMsg)
user_app.go:                            return nil, fmt.Errorf(errorMsg)
user_app.go:            return nil, "", fmt.Errorf("hrp len should be <10")
user_app.go:                    return nil, "", fmt.Errorf("all characters in the HRP must be in the [33, 126] range")
user_app.go:            return nil, "", fmt.Errorf("Invalid response")
validator_app.go:                       return nil, fmt.Errorf("are you sure the Tendermint Validator app is open?")
validator_app.go:               return nil, fmt.Errorf("invalid response")
validator_app.go:               return nil, fmt.Errorf("invalid response. Too short")

These should instead use the "errors" package to cut out the dependencies used as well as binary/app size.

This issue is related to cosmos/gaia#1801

• align with any changes done in cosmos/ledger-go, move them to zondax/ledger-go
• deprecate cosmos/ledger-go
• release a new version of zondax/ledger-go
• update ledger-cosmos-go to point to zondax/ledger-go
• release ledger-cosmos-go
• rework fork relations so cosmos/ledger-cosmos-go is the main repo

Test ledger integrations with the effects of

cosmos/cosmos-sdk#6213

The projects, who are using cosmos-sdk, has no option for the their ledger app versioning.
For the terra, it should make its own ledger application because BIP32 CoinType is different. But below Version checking mechanism force us to use specific version in our custom ledger app

This code

by this point has a nil error and just confuses the reader who might be thinking that err was set from above but really it is nil. That return should be turned into return app, nil

/cc @elias-orijtech @julienrbrt

The observed behavior so far is something in MacOS Venture 13.6 upgrade seems to have created a situation where cosmos binaries built after the upgrade cannot detect a ledger.

Oddly binaries built before the upgrade on the same machine detect the ledger just fine.

I've tried various go versions like 1.19, 1.20 and 1.21 and the problem persists.

Error message will be something like failed to retrieve device: ledger nano S: hidapi: failed to open device

All the binaries I've tried have been using ledger-cosmos-go 0.12.2.