Skip to content

Conversation

@relatko
Copy link

@relatko relatko commented Jan 27, 2023

Main new features of the app:

  • The app now supports 25 various signTx actions instead of one.
    • Two actions contain Diffie-Hellman encrypted shared secret.
  • New Decode shared secret call

Tutorial for the new app is available at:
https://kb.fioprotocol.io/user-guides/ledger-fio-app-tutorial
The new features are only deployed on the beta site: https://beta-dashboard.fioprotocol.io

The folder /doc/security contains documentation for security review.

  • Contains detailed description of ledger app code changes.
  • Gives code modularization into smaller chunks. Besides this it gives suggested order in which to review the chunks. We made our review based on this order.
  • Contains app architecture description from security perspective.

relatko added 30 commits April 6, 2022 09:17
Merge ledger hq changes, change icons
Build improvements, Speculos tests
They behave similar to hash functions
The code now pases original integration tests
This way derivation path is available for DH encoding
Changing newfundsreq data content format, introduce error specific
to integrity checks.
applying parse not validate as it is supposed
Derivation path is serializes LE
Remove unnecessary endian functions
List of hashes for integrity check is not updated at the moment...

Choose a reason for hiding this comment

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

This whole file could be simplified using up to date app-boilerplate/makefile as a reference.

Choose a reason for hiding this comment

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

TRY CATCH usage can be remove using _no_throw version of os_perso_derive_ and cx_ functions.
You could also use the helper from https://github.com/LedgerHQ/ledger-secure-sdk/blob/master/lib_standard_app/crypto_helpers.h.

Copy link
Author

Choose a reason for hiding this comment

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

I refactored all code handling intermediate (not to be returned) secrets into keyDerivation.c and diffieHellman.c (+ hash.h) files where errors are handled by returning error value.

Choose a reason for hiding this comment

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

Many function can be removed if using the new standard_app lib from the SDK.
This improves the app code readability and helps removing some classical bugs.

@relatko
Copy link
Author

relatko commented Jun 10, 2023

Added guideline enforcer + fixed issues it found.
Bumped containers to most recent versions (build + speculos). Resolved breaking changes.

Fixed:
- Missing input value validation
- Potential missing secret wiping from stack after use
- String manipulation
- Pointer arithmetic on void pointer
- Trivial swich-case statements were removed
- Replaced certain other memset calls
@relatko relatko force-pushed the to_merge_with_LedgerHQ_1_0_5 branch from da25e74 to 9fddc72 Compare July 18, 2023 13:19
@tdejoigny-ledger
Copy link

hello @relatko, the CI checks shall be green to submit the PR. Please make the changes to have the CI green. Thank you.

@xchapron-ledger xchapron-ledger mentioned this pull request Jan 17, 2024
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.

4 participants