Nimiq App Version 2.0 #6
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously properties for touch events were defined which only exist for the Ledger Blue SDK. The compiler was thus emitting a warning: "excess elements in struct initializer"
Note that some of the removed glyphs are indeed used but provided by the sdk.
"Get App Name and Version" (APDU b0 01 00 00) implemented by the SDK in os_io_seproxyhal_get_app_name_and_version can be used instead.
blake2b is available in newer sdk versions such that we don't have to ship our own implementation. Reduces the app size by ~6kb.
- Replace ux_variable_display logic which was also not working correctly on Nano S (starting at wrong step for paged content) by explicitly defined steps. - Replace generic details captions and ui_elements_map by explicit steps - Replace deprecated macros - Rename steps and flows - Make fee display optional (don't display zero fee) - Remove validityStartHeight from Ledger Nano X flow - Make transaction type an enum - Move transaction type label to tx content
Adding support for displaying the message as ascii or in hex representation, apart from the hash.
Try to provide the user an as readable as possible message display format instead of falling back to hash display, unless a specific format is requested.
Previously, blindly signing up to 32 bytes of arbitrary data was allowed, which could be abused for signing messages following the Nimiq standard for signed messages, which are 32 byte Sha256 hashes. This commit fixes that. In the future, we'll further consider disallowing signing of arbitrary messages altogether even below the 31 byte threshold, by prefixing and hashing the passed message or removing the feature.
- Check that apdu length matches the advertised length. This was not checked for previously. - Consequently check that any data read from passed data does not exceed input data length. Previously such checks were missing in a few places like reading of bip32 paths or transaction parsing, apart from transaction extra data. - Check that after parsing input data there's no leftover data. This was not checked for previously in some places like public key requests and transaction parsing, apart from transaction extra data. The check of exact data lengths is especially also in preparation of adding a transaction version byte in the next commit for distinguishing legacy trans- actions from Albatross transactions. By checking for exact data lengths, we can detect if the version byte is missing, and this way reject supporting old ver- sions of the client api. Instead, devs using the client api, should update the api accordingly. On the other hand, new versions of the api can detect which version of the Ledger app they're communicating to, and accordingly send the transaction version byte or not, to keep supporting old versions of the Ledger app not updated by the user yet.
Add a transaction version byte to transaction signing requests to identify legacy or Albatross transactions, or future transactions formats. This commit only adds handling of this version byte, no support for Albatross yet. Notably, by checking for exact data lengths as of the previous commit, we can detect if the version byte is missing, and this way reject supporting old ver- sions of the client api. Instead, devs using the client api, should update the api accordingly. On the other hand, new versions of the api can detect which version of the Ledger app they're communicating to, and accordingly send the transaction version byte or not, to keep supporting old versions of the Ledger app not updated by the user yet.
Utilities for reading varints and rust Vec<u8> compatible with serde / postcard serialization. readUVarInt only supports single byte varints so far as currently we don't need higher values. These utilities are in preparation to reading staking transaction data for Albatross.
I.e. support transactions without any flags or sender data.
Includes incoming and outgoing staking transactions for all Staker related data but not for validators.
For incoming staking transactions which are meant to include a staker signature proof in their recipient data but only include the empty default proof, we replace that empty signature proof with an actually signed staker proof. For this, the same ledger account is used as staker and transaction sender, which is the most common case for regular users. This way, no separate signature request needs to be sent to the ledger to first create the staker signature proof, but both signatures are created in a single request for better UX. For advanced users, usage of a different staker than the transaction sender is still supported by providing a ready pre-signed signature proof in the request instead of an empty signature proof.
- Refactor UI and add NBGL support for Ledger Stax and Flex - Update Makefile to be compatible to newest SDK - Replace usage of constants removed in the SDK.
- share global RAM between transaction and message reviews - generate nbgl_contentTagValueList_t in a temporary buffer instead of a global buffer
Update Makefile to be based on $(BOLOS_SDK)/Makefile.standard_app, going through all the previous entries and removing the ones that are now covered by that base Makefile or other Makefiles included therein.
- This touches a lot of very old, ugly and partially problematic code inherited from the Stellar app we forked long time ago, improving at least parts of it. - Return error codes everywhere instead of throwing them. Ledger's SDK was initially based on a custom exception mechanism for THROWing exceptions which has since been deprecated and discouraged due to its issues, see developers.ledger.com/docs/device-app/develop/tips#exception-handling and developers.ledger.com/docs/device-app/architecture/cryptography/general-concepts section #avoid-exceptions-for-cryptographic-code. - Use no_throw variants of SDK methods instead of now deprecated throwing SDK methods. Note though, that the SDK can still through internally for other methods like io_exchange. - Remove all but a single remaining app-wide TRY block in main to catch any exceptions still thrown internally by SDK methods. This also includes removing an unnecessary TRY block in app_exit around code which doesn't throw anyway. - Handle INVALID_STATE exceptions thrown internally by the SDK the same way as EXCEPTION_IO_RESET, by resetting the app, instead of exiting. - Introduce an error macro for error handling with stack trace and debug message printing. - Introduce constants for error codes and status words. - Add trailing newlines to debug messages where they were missing. - On error clear the entire global data, not just the transaction context, and now also on any error, not just on status words in the 0x6000 range. - Ensure that private keys are properly cleared from memory in error cases. - Remove duplicate error handling between handle_apdu and nimiq_main. - Remove unused return values of async response handlers. - Refactor sending of async response APDUs by introducing a response handler io_send_async_response_apdu for it with additional code documentation. Also remove some code duplication by unifying handlers for request rejection. - While already changing most of the function signatures, also change method and parameter names to snake case, as that's what's most commonly used in Ledger's SDK and in the boilerplate and other apps. No names of methods that weren't touched as part of the refactoring, and not local variables, type names etc. were changed. That's left for another time. - Also rename output parameters, which are to be written by a method, to out_*. - Slightly rearrange code in main.c such that command apdu handlers and async response handlers are better grouped. This way too big file (also inherited from Stellar), should eventually be split into smaller logical units.
…sions Sanity check expressions for improper use of logical expressions, where their result, which is 0 or 1, is assigned as received error and used instead of the actually received error which gets lost in the logical expression.
Add RETURN_ERROR for returning an error non-conditionally (in contrast to conditional RETURN_ON_ERROR) with additional debug / stack trace printing.
- Exit the app via LEDGER_ASSERT everywhere where previously an ERROR_UNEXPECTED was returned. - Discourage use of ERROR_UNEXPECTED and ERROR_TRUE by emitting a compiler warning on use. - Refactor the sanity check for improper use of logical expressions in error handling with LEDGER_ASSERT instead of a manual app_exit(). - Exit the app via LEDGER_ASSERT on unexpected or unknown errors in ERROR_TO_SW.
Instead of letting apdu / request handlers output io_exchange flags, let them output a bool out_start_async_reply, because IO_ASYNCH_REQUEST was the only flag that was ever set in them, if any. Also rename io_send_async_response_apdu accordingly to io_finalize_async_reply. Additionally, add some more comments. This should improve code readability and understandability of the somewhat complicated io_exchange data flows.
- Deprecated throwing methods were already replaced by their non-throwing counterparts in the big "remove legacy THROW" refactor. - Now replace remaining deprecated methods os_memmove and os_memset. - Replace types cx_ecfp_public_key_t and cx_ecfp_private_key_t which are not marked as deprecated yet in the SDK, but are meant to change their meaning in the future.
- Avoid type error of const char * vs char * for "ed25519 seed" string literal seed key in os_derive_bip32_with_seed_no_throw by using the default value. (Otherwise, a type cast would also have worked.) - Cast char * to uint8_t * for cx_hash_update. Declaring MESSAGE_SIGNING_PREFIX as an array unsigned char const MESSAGE_SIGNING_PREFIX[] with some array initializer would have worked, too, but would have required marking it extern and defining it in a .c file for a single instance to be shared across all translation units, or to be declared as static only in the .c file where it's used, which currently is main.c which is already bloated enough. Additionally, it's less clear whether the compiler actually puts the array into the read only data section in ROM, or whether a copy of the array resides in RAM. As a string literal, it's very clear that the data sits in the ROM only. - Mark unused parameter in io_event as such with a newly added decorator macro. - Mark switch case fallthrough in io_event as such with a newly added decorator macro. - Cast parameters of nvm_write to void *. That's how the boilerplate app and other apps do it, too. - Accept the ticker in parse_amount as const char * instead of char * to avoid having to cast when passing a const string literal. - Avoid array length defined by a variable by instead making AMOUNT_MAX_SIZE a preprocessor definition. - Avoid Ledger SDK's non-standard print format %.*h implemented in os_printf.c for printing data as hex, by using our print_hex method instead.
…_amount - Define string lengths via sizeof of the possible strings, instead of magic numbers. - Move MAX_SAFE_INTEGER and AMOUNT_MAX_SIZE to constants and give them a better name and better semantics. AMOUNT_MAX_SIZE did previously not include the \0 string terminator, which worked but was less intuitive. For the sake of consistency and intuitiveness, it has now been included. Additionally, the NIM amount string length including the NIM ticker is now based on this value. - Improve the handling of string lengths and looping over indices in parse_amount, to make the code more readable and intuitive.
- In the previous usages of strncpy, the source to copy was not actually a \0 terminated string, which makes using strncpy instead of simply memmove rather pointless. - As in those cases, the specific behavior of strncpy for \0 terminated strings was not required, memmove is more efficient than strncpy, as no searching of a string terminator is involved. - No need to add strncpy to the app binary, if we don't need it.
- strcpy doesn't check the size of the destination buffer, which can lead to buffer overflows. The clang static analyzer scan-build picked up on some cases where it couldn't statically confirm that no buffer overflow occurs. None of the flagged strcpy usages was actually problematic though. - To already statically check data/buffer lengths at compile time, as opposed to only when running scan-build, a macro COPY_FIXED_SIZE has been added, which statically checks lengths of data/buffers of fixed size which are known at compile time. After the static check, a simple memcpy is performed, instead of strcpy. - In cases, where no static check is possible, i.e. cases that were flagged by scan-build, strcpy has been replaced by memcpy after ensuring that buffer lengths are checked at runtime, to silence the scan-build's warnings. - memcpy should theoretically be more performant than strcpy, as strcpy scans for a string terminator. Additionally, removing all strcpy usages, should strip it from the app bundle. However, the compiler previously already seems to have optimized usages of strcpy (for example the cases where a fixed size string is copied into a fixed size buffer), such that removing all strcpy usages didn't yield any actual benefit in this regard.
- strcpy related warnings, where scan-build couldn't statically check for buffer overflows in strcpy usages, were already addressed in the previous commit. None of those warnings were actually problematic. - Resolve a warning, where scan-build thought that variable initialized in one if block, was not initialized in a later if block with the same condition. This was not an actual problem, as the condition of the second if block was the same as the first, and the variables used in the condition don't change values in between, i.e. when the second if block ran, the first one always ran previously, initializing the variable. Nonetheless, the warning has been resolved by checking in the second if condition that the variable has been initialized. - Add reports generated by clang static analyzer scan-build to .gitignore
- List Stax and Flex as supported devices. - List test directories.
memset could be optimized away by the compiler, other than explicit_bzero.
…y-message:" This is to further ensure that no meaningful data can be blindly signed.
Change prefix from "dummy-message:" to "dummy-data:" as it's shorter, and fix the error condition, as it was the wrong way around.
…on check happy The glyph build task seems to be working perfectly fine with icons of bit depth lower than 8 bit-per-pixel, but the guidelines_enforcer enforces 8 bit-per-pixel. Note that the final app binary size is unaffected by this change, as the glyph build task reduces the color palette / bits-per-pixel again to the supported maximum number of shades of gray. As app_nimiq_64px.gif seems to have slightly changed, this commit also updates the Ragger screenshots accordingly. At the same time, some unused excess images have been removed which were not properly deleted from previous tests.
This release includes a wide range of updates and new features, including but not limited to: - Support for Albatross and Staking transactions. - Support Ledger Stax and Flex. - Flow based UX on Ledger Nano S - HTLC and Vesting Contract creation support. (These have not been updated for NBGL and Albatross yet.) - Sending from contracts as sender. - Message signing. - Display binary transaction data as hex. - General security hardening.
|
The app database update PR has been merged and all workflows on nimiq@1f8177e do now pass. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
developUpdate app version to 2.0
This release includes a wide range of updates and new features, including but not limited to:
As well as many maintenance tasks, including but not limited to:
Note that an audit report by one of the approved external auditor has already been forwarded to you, which shows no issues.
The associated update to the app database is provided here.