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.

Sign up for GitHub

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

Jump to bottom

accounts/usbwallet: mitigate ledger app chunking issue #26773

Merged
merged 2 commits into from
Mar 7, 2023
Merged

accounts/usbwallet: mitigate ledger app chunking issue #26773

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aab53c5
fix: mitigate ledger app chunking issue
prestwich Feb 27, 2023
e61909b
lint: run gofmt
prestwich Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion accounts/usbwallet/ledger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ const (
ledgerP1InitTransactionData ledgerParam1 = 0x00 // First transaction data block for signing
ledgerP1ContTransactionData ledgerParam1 = 0x80 // Subsequent transaction data block for signing
ledgerP2DiscardAddressChainCode ledgerParam2 = 0x00 // Do not return the chain code along with the address

ledgerEip155Size int = 3 // Size of the EIP-155 chain_id,r,s in unsigned transactions
)

// errLedgerReplyInvalidHeader is the error message returned by a Ledger data exchange
Expand Down Expand Up @@ -347,9 +349,15 @@ func (w *ledgerDriver) ledgerSign(derivationPath []uint32, tx *types.Transaction
op = ledgerP1InitTransactionData
reply []byte
)

// Chunk size selection to mitigate an underlying RLP deserialization issue on the ledger app.
// https://github.com/LedgerHQ/app-ethereum/issues/409
chunk := 255
for ; len(payload)%chunk <= ledgerEip155Size; chunk-- {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I getting this right? You want to find a chunk value that makes the minimum transmitted chunk size larger than 3?

Copy link
Contributor Author

@prestwich prestwich Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. The ledger bug above triggers when a chunk boundary falls immediately between data and chainid in unsigned, legacy transactions. By ensuring that the final chunk is always at least 4 bytes large, we prevent this triggering condition from occurring

The EIP-155 unsigned TX RLP format is:
(nonce, gasprice, startgas, to, value, data, chainid, 0, 0)

The root cause of the bug appears to be that ledger does not validate the length of the RLP list. The unsigned tx RLP list contains 9 items. However, the Ledger Ethereum app completes deser of the unsigned transaction when it has received at least 6 items, and the RLP input has been exactly consumed
(no extra bytes are left in the device's RLP buffer). When the chunk boundary falls between data and chainid these conditions are met, and the ledger device prematurely starts its signing flow. The remaining 3 items chainid, 0 ,0 are in an unsent chunk. During the signing flow, this causes the chainid in the device to be 0, and the user to accidentally sign a transaction with chainid 0.

The mitigation ensures that the chunk boundary doesn't fall on the item boundary between data and chainid. If chainid is a single byte, then 3 list items are 3 bytes long. If chainid is 2 or more bytes, then ensuring the last packet is at least 4 bytes ensures that the packet boundary either includes chainid or falls in the middle of chainid. This ensures that the triggering conditions are never met, as either the chainid will be sent in its entirety, or a portion of the chainid will be sent, and the device will wait for the final packet


for len(payload) > 0 {
// Calculate the size of the next data chunk
chunk := 255
if chunk > len(payload) {
chunk = len(payload)
}
Expand Down