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

Remove suprious OSX USB Reads #23

Merged
merged 4 commits into from
Dec 7, 2022
Merged

Conversation

chillyvee
Copy link
Contributor

Porting PR from: cosmos#7

Transaction approval with Ledger + OS X often fails due to:

Packets full of zeros being read
Packets out of sequence being read
This patch filters all zero filled packets and ensures each USB exchange (Send + Receive) start on proper sequence boundaries.

Typical Errors - Various chains

Error: len < 2

Modern Error

juno v11.0.0 with :
github.com/zondax/ledger-go v0.14.0
github.com/cosmos/ledger-cosmos-go v0.12.0

Error: Invalid channel

Replaced with this branch - Eliminates error

--

Previous tester feedback on cosmos/ledger-go PR based code:

https://twitter.com/KevinGarrison/status/1588151637432012800
https://twitter.com/rhinostake/status/1588161469623013377

@ftheirs
Copy link
Contributor

ftheirs commented Dec 2, 2022

Since this package is used but other projects (not only Cosmos), please correct the error codes to avoid specific names like Keplr, Ledger live or Cosmos app 🙏

@ftheirs ftheirs self-requested a review December 2, 2022 21:17
@chillyvee
Copy link
Contributor Author

Done. If message should be more generic, let us know again.

Copy link
Collaborator

@raynaudoe raynaudoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT, only some nits

apduWrapper.go Outdated
@@ -51,6 +52,8 @@ func ErrorMessage(errorCode uint16) string {
return "[APDU_CODE_INS_NOT_SUPPORTED] Instruction code not supported or invalid"
case 0x6E00:
return "[APDU_CODE_CLA_NOT_SUPPORTED] CLA not supported"
case 0x6E01:
return "[0x6E01] Ledger Connected but Chain Specific App Not Open"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add in the return the corresponding apdu code instead of 0x6E01?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could not locate the APDU description in any other source code/documentation. Do you know what repo/component runs the ledger "home screen"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented as APDU_CODE_APP_NOT_OPEN

ledger_hid.go Outdated

// Discard all zero packets from Ledger Nano X on macOS
allZeros := true
for i := 0; i < 64; i++ {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe here would be better to write:
for i := 0; i < len(buffer);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

apduWrapper.go Outdated
Comment on lines 206 to 208
if err != nil {
return nil, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you first check if err != nil right after calling DeserializePacket ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@chillyvee
Copy link
Contributor Author

chillyvee commented Dec 7, 2022

Unable to find APDU description for code 0x6E01. Can you recommend a reference?

This is the most official we could locate so far https://support.ledger.com/hc/en-us/articles/5282886278557-Solve-error-0x6E01?support=true

Does not look like a standard APDU code.

@ftheirs
Copy link
Contributor

ftheirs commented Dec 7, 2022

Hi @chillyvee! I think that this code was by Ledger because it's replied by the device itself and not from the app.
https://support.ledger.com/hc/en-us/articles/5282886278557-Solve-error-0x6E01?support=true
Something like this would be ok: APDU_CODE_APP_NOT_OPEN

@chillyvee
Copy link
Contributor Author

Implemented as APDU_CODE_APP_NOT_OPEN

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ftheirs ftheirs merged commit 4e57dab into Zondax:main Dec 7, 2022
@ftheirs
Copy link
Contributor

ftheirs commented Dec 7, 2022

Release v0.14.1 was published! Thank you @chillyvee!

@chillyvee
Copy link
Contributor Author

Thank you all!

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.

3 participants