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

avoid using uninitialized data from G_io_apdu_buffer #6

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

avoid using uninitialized data from G_io_apdu_buffer #6

wants to merge 3 commits into from

Conversation

zeldovich
Copy link
Collaborator

No description provided.

@zeldovich zeldovich requested a review from justicz December 19, 2019 19:47
@justicz
Copy link
Contributor

justicz commented Dec 19, 2019 •
edited
Loading

This appears to not be true on the S, but on the ledger X, if I send a new transaction to be signed over bluetooth, but the current transaction has not been approved/denied yet, then the tx approval flow starts over. In that situation, I think we'd fail to clear the buffer.

@justicz justicz mentioned this pull request Jan 3, 2020
Merged
justicz
justicz reviewed Jan 16, 2020
View reviewed changes
src/main.c Outdated
@@ -40,9 +40,23 @@ struct txn current_txn;
static uint8_t msgpack_buf[1024];
static unsigned int msgpack_next_off;

/* The SDK exposes no information about the length of the received
Copy link
Contributor

Choose a reason for hiding this comment

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

io_exchange appears to return a length, but it's unclear to me if an attacker can make it seem artifically large.

justicz
justicz approved these changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@justicz justicz left a comment

Choose a reason for hiding this comment

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

I think this is a worthwhile change, though it doesn't quite capture all of the ways that old G_io_apdu_buffer data can end up being displayed on the screen.

@zeldovich
Copy link
Collaborator Author

zeldovich commented Jan 22, 2020 •
edited
Loading

Looking at this a bit more, it looks like there is G_io_apdu_length, which seems to be the real number of bytes that were placed into G_io_apdu_buffer. What do you think of changing our top-level APDU dispatcher to make sure that we do not parse more than G_io_apdu_length bytes out of G_io_apdu_buffer, and skip the whole memset business?

@justicz
Copy link
Contributor

justicz commented Jan 22, 2020

That sounds reasonable! If there is a reliable source of received length then this becomes easy.

@zeldovich
fix the out-of-bounds check to avoid UB
08ed394 
i think this is purely a pedantic change (our actual invocations of
copy_and_advance could never trigger UB), but might as well use the
UB-free code pattern.
@zeldovich
Copy link
Collaborator Author

Can you check if this code works on the fancy new bluetooth Ledger hardware?

(Actually, I'll be in the office later today -- do you have any extras of the new Ledger devices?)

@justicz
Copy link
Contributor

justicz commented Jan 23, 2020

Hm, just tested this over BLE. It appears to work when the short transaction is sent first, however if I first send a normal payment msgpack payload, approve, and subsequently send a too-short keyreg command, the transaction type is displayed as "Unknown" and the subsequent fields are zeroed out.

gmalouf pushed a commit that referenced this pull request Aug 3, 2022
@ftheirs
Merge pull request #6 from Zondax/improvements
a1ea6fe 
Improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justicz justicz justicz approved these changes

Assignees

@id-ms id-ms

Labels
external contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zeldovich @justicz @algorandskiy @chaihoang @id-ms @yaovi-a