From d42d012970c4255173cf08990cae5676c7fc91aa Mon Sep 17 00:00:00 2001 From: Jan Mazak Date: Sun, 17 Mar 2024 16:56:42 +0100 Subject: [PATCH] fix: audit --- README.md | 12 ++++++------ fuzzing/CMakeLists.txt | 3 ++- fuzzing/README.md | 1 + src/getPublicKeys.c | 1 + src/signCVote.c | 1 + src/signMsg.c | 20 ++++++++++++++------ src/signMsg_ui.c | 3 ++- src/signMsg_ui.h | 6 +++--- src/signTxCVoteRegistration.c | 1 + src/signTxMint.c | 1 + src/signTxOutput.c | 2 ++ src/signTxOutput_ui.c | 6 ++++-- 12 files changed, 38 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index c822fa9a..b021f825 100644 --- a/README.md +++ b/README.md @@ -10,11 +10,7 @@ We recommend using the containerized build. See [Getting started](doc/build.md) ### Loading the app -`make load` - -Builds and loads the application into the connected device. Make sure to close the Ledger app on the device before running the command. - -Most common reason for a failed loading is the app taking too much space. Check `make size` (should be below 140K or so). +We recommend using the [Ledger VS Code plugin](https://marketplace.visualstudio.com/items?itemName=LedgerHQ.ledger-dev-tools) to load the app on a device. ### Debug version @@ -27,7 +23,7 @@ also comment out DEFINES += RESET_ON_CRASH -and then run `make clean load`. +The debug version is too big to fit on Nano S, but works on Speculos. ### Setup @@ -78,6 +74,10 @@ _Before merging a PR, one should make sure that:_ * `make clean load` runs without errors and warnings (except those reported for nanos-secure-sdk repo) for development build (see Debug version above) * `make analyze` does not report errors or warnings +## Running tests + +All the tests are initiated from the accompanying [ledgerjs package](https://github.com/vacuumlabs/ledgerjs-cardano-shelley) (see what [commands to run](https://github.com/vacuumlabs/ledgerjs-cardano-shelley?tab=readme-ov-file#tests)). You have to make sure that the version of ledgerjs correspond to the app version, otherwise some tests with fail (possibly resulting in odd errors) or test coverage will be incomplete. + ## How to get a transaction body computed by Ledger Ledger computes a rolling hash of the serialized transaction body, but the body itself is ordinarily not available. It is possible to acquire it from the development build by going through the following steps: diff --git a/fuzzing/CMakeLists.txt b/fuzzing/CMakeLists.txt index 6cda3a91..f4f281f6 100644 --- a/fuzzing/CMakeLists.txt +++ b/fuzzing/CMakeLists.txt @@ -142,6 +142,7 @@ add_compile_definitions( HAVE_ECDSA HAVE_EDDSA HAVE_HASH + HAVE_SHA224 HAVE_SHA256 HAVE_SHA3 @@ -181,4 +182,4 @@ foreach(harness IN LISTS harnesses) ./src/${harness}.c ) target_link_libraries(${harness} PUBLIC cardano) -endforeach() \ No newline at end of file +endforeach() diff --git a/fuzzing/README.md b/fuzzing/README.md index fb18670a..383c01ab 100644 --- a/fuzzing/README.md +++ b/fuzzing/README.md @@ -37,4 +37,5 @@ Since there is an already existing corpus, to start fuzzing with it simply do `. ## Notes + For more context regarding fuzzing check out the app-boilerplate fuzzing [README.md](https://github.com/LedgerHQ/app-boilerplate/blob/master/fuzzing/README.md) diff --git a/src/getPublicKeys.c b/src/getPublicKeys.c index 182898c0..e1615236 100644 --- a/src/getPublicKeys.c +++ b/src/getPublicKeys.c @@ -218,6 +218,7 @@ void getPublicKeys_handleAPDU( bool isNewCall ) { + ASSERT(wireDataBuffer != NULL); ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA); if (isNewCall) { diff --git a/src/signCVote.c b/src/signCVote.c index 6f275e4f..4d51e92e 100644 --- a/src/signCVote.c +++ b/src/signCVote.c @@ -278,6 +278,7 @@ void signCVote_handleAPDU( bool isNewCall ) { + ASSERT(wireDataBuffer != NULL); ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA); if (isNewCall) { diff --git a/src/signMsg.c b/src/signMsg.c index 3fbf0d43..1eb87529 100644 --- a/src/signMsg.c +++ b/src/signMsg.c @@ -193,6 +193,7 @@ static void _prepareAddressField() } } +__noinline_due_to_stack__ static size_t _createProtectedHeader(uint8_t* protectedHeaderBuffer, size_t maxSize) { // protectedHeader = { @@ -207,36 +208,41 @@ static size_t _createProtectedHeader(uint8_t* protectedHeaderBuffer, size_t maxS { size_t len = cbor_writeToken(CBOR_TYPE_MAP, 2, p, end - p); p += len; + ASSERT(p < end); } { size_t len = cbor_writeToken(CBOR_TYPE_UNSIGNED, 1, p, end - p); p += len; + ASSERT(p < end); } { size_t len = cbor_writeToken(CBOR_TYPE_NEGATIVE, -8, p, end - p); p += len; + ASSERT(p < end); } { size_t len = cbor_writeToken(CBOR_TYPE_TEXT, 7, p, end - p); p += len; + ASSERT(p < end); } - { const char* text = "address"; const size_t len = strlen(text); ASSERT(p + len < end); memmove(p, text, len); p += len; + ASSERT(p < end); } - _prepareAddressField(); { + _prepareAddressField(); + ASSERT(ctx->addressFieldSize > 0); + size_t len = cbor_writeToken(CBOR_TYPE_BYTES, ctx->addressFieldSize, p, end - p); p += len; - } - { ASSERT(p + ctx->addressFieldSize < end); memmove(p, ctx->addressField, ctx->addressFieldSize); p += ctx->addressFieldSize; + ASSERT(p < end); } const size_t protectedHeaderSize = p - protectedHeaderBuffer; @@ -250,7 +256,7 @@ static size_t _createProtectedHeader(uint8_t* protectedHeaderBuffer, size_t maxS } } END_TRY; - return -1; + return SIZE_MAX; } static void signMsg_handleConfirmAPDU(const uint8_t* wireDataBuffer MARK_UNUSED, size_t wireDataSize) @@ -283,8 +289,9 @@ static void signMsg_handleConfirmAPDU(const uint8_t* wireDataBuffer MARK_UNUSED, ASSERT(written < maxWritten); } { - uint8_t protectedHeaderBuffer[100]; // address of max 57 bytes plus a couple of small items + uint8_t protectedHeaderBuffer[100] = {0}; // address of max 57 bytes plus a couple of small items const size_t len = _createProtectedHeader(protectedHeaderBuffer, SIZEOF(protectedHeaderBuffer)); + ASSERT(len < BUFFER_SIZE_PARANOIA); written += cbor_writeToken(CBOR_TYPE_BYTES, len, sigStructure + written, maxWritten - written); ASSERT(written + len < maxWritten); memmove(sigStructure + written, protectedHeaderBuffer, len); @@ -360,6 +367,7 @@ void signMsg_handleAPDU( ) { TRACE("P1 = 0x%x, P2 = 0x%x, isNewCall = %d", p1, p2, isNewCall); + ASSERT(wireDataBuffer != NULL); ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA); VALIDATE(p2 == P2_UNUSED, ERR_INVALID_REQUEST_PARAMETERS); diff --git a/src/signMsg_ui.c b/src/signMsg_ui.c index 6406f0ed..6377a7d9 100644 --- a/src/signMsg_ui.c +++ b/src/signMsg_ui.c @@ -142,7 +142,8 @@ void _displayMsgIntro(ui_callback_fn_t* callback) ASSERT(strlen(l1) + 1 < SIZEOF(l1)); char l2[30] = {0}; - snprintf(l2, SIZEOF(l2), "%u bytes", ctx->msgLength); + ASSERT(ctx->msgLength < UINT32_MAX); + snprintf(l2, SIZEOF(l2), "%u bytes", (uint32_t)ctx->msgLength); ASSERT(strlen(l2) + 1 < SIZEOF(l2)); #ifdef HAVE_BAGL diff --git a/src/signMsg_ui.h b/src/signMsg_ui.h index d6b7b87d..2f81df50 100644 --- a/src/signMsg_ui.h +++ b/src/signMsg_ui.h @@ -1,5 +1,5 @@ -#ifndef H_CARDANO_APP_SIGN_CVOTE_UI -#define H_CARDANO_APP_SIGN_CVOTE_UI +#ifndef H_CARDANO_APP_SIGN_MSG_UI +#define H_CARDANO_APP_SIGN_MSG_UI #include "uiHelpers.h" @@ -40,4 +40,4 @@ enum { void signMsg_handleConfirm_ui_runStep(); -#endif // H_CARDANO_APP_SIGN_CVOTE_UI +#endif // H_CARDANO_APP_SIGN_MSG_UI diff --git a/src/signTxCVoteRegistration.c b/src/signTxCVoteRegistration.c index 5f080431..501adc81 100644 --- a/src/signTxCVoteRegistration.c +++ b/src/signTxCVoteRegistration.c @@ -724,6 +724,7 @@ bool signTxCVoteRegistration_isValidInstruction(uint8_t p2) void signTxCVoteRegistration_handleAPDU(uint8_t p2, const uint8_t* wireDataBuffer, size_t wireDataSize) { + ASSERT(wireDataBuffer != NULL); ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA); switch (p2) { diff --git a/src/signTxMint.c b/src/signTxMint.c index 5d6ebfe5..5957588b 100644 --- a/src/signTxMint.c +++ b/src/signTxMint.c @@ -241,6 +241,7 @@ void signTxMint_init() void signTxMint_handleAPDU(uint8_t p2, const uint8_t* wireDataBuffer, size_t wireDataSize) { + ASSERT(wireDataBuffer != NULL); ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA); switch (p2) { diff --git a/src/signTxOutput.c b/src/signTxOutput.c index 579ec09b..54e6b559 100644 --- a/src/signTxOutput.c +++ b/src/signTxOutput.c @@ -1071,6 +1071,7 @@ bool signTxOutput_isValidInstruction(uint8_t p2) void signTxOutput_handleAPDU(uint8_t p2, const uint8_t* wireDataBuffer, size_t wireDataSize) { + ASSERT(wireDataBuffer != NULL); ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA); switch (p2) { @@ -1132,6 +1133,7 @@ bool signTxCollateralOutput_isValidInstruction(uint8_t p2) void signTxCollateralOutput_handleAPDU(uint8_t p2, const uint8_t* wireDataBuffer, size_t wireDataSize) { + ASSERT(wireDataBuffer != NULL); ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA); switch (p2) { diff --git a/src/signTxOutput_ui.c b/src/signTxOutput_ui.c index 72ba1442..ff955d92 100644 --- a/src/signTxOutput_ui.c +++ b/src/signTxOutput_ui.c @@ -356,7 +356,8 @@ void signTxOutput_handleDatumInline_ui_runStep() size_t datumSize = subctx->stateData.datumRemainingBytes + subctx->stateData.datumChunkSize; // datumSize with 6 digits fits on the screen, less than max tx size // if more is needed, "bytes" can be replaced by "B" for those larger numbers - snprintf(l1, SIZEOF(l1), "Datum %u bytes", datumSize); + ASSERT(datumSize < UINT32_MAX); + snprintf(l1, SIZEOF(l1), "Datum %u bytes", (uint32_t)datumSize); ASSERT(strlen(l1) + 1 < SIZEOF(l1)); char l2[20]; @@ -397,7 +398,8 @@ void handleRefScript_ui_runStep() size_t scriptSize = subctx->stateData.refScriptRemainingBytes + subctx->stateData.refScriptChunkSize; // scriptSize with 6 digits fits on the screen, less than max tx size // if more is needed, "bytes" can be replaced by "B" for those larger numbers - snprintf(l1, SIZEOF(l1), "Script %u bytes", scriptSize); + ASSERT(scriptSize < UINT32_MAX); + snprintf(l1, SIZEOF(l1), "Script %u bytes", (uint32_t)scriptSize); ASSERT(strlen(l1) + 1 < SIZEOF(l1)); char l2[20];