Audit fixes (#32) (#33)

* add flare CLA extra verification * remove not needed DER signature from response * update zxlib * bump version * update fuzzer * update docs * update docs
Zondax · Dec 23, 2024 · f78227d · f78227d
2 parents 41f01e6 + 6d61786
commit f78227d
Show file tree

Hide file tree

Showing 20 changed files with 102 additions and 22 deletions.
diff --git a/app/Makefile.version b/app/Makefile.version
@@ -3,4 +3,4 @@ APPVERSION_M=2
 # This is the minor version
 APPVERSION_N=4
 # This is the patch version
-APPVERSION_P=2
+APPVERSION_P=3
diff --git a/app/src/apdu_handler.c b/app/src/apdu_handler.c
@@ -242,23 +242,35 @@ void handleApdu(volatile uint32_t *flags, volatile uint32_t *tx, uint32_t rx) {
             } else {
                 switch (instruction) {
                     case INS_GET_VERSION: {
+                        if (cla != CLA) {
+                            THROW(APDU_CODE_COMMAND_NOT_ALLOWED);
+                        }
                         handle_getversion(flags, tx);
                         break;
                     }
 
                     case INS_GET_ADDR: {
+                        if (cla != CLA) {
+                            THROW(APDU_CODE_COMMAND_NOT_ALLOWED);
+                        }
                         CHECK_PIN_VALIDATED()
                         handleGetAddr(flags, tx, rx);
                         break;
                     }
 
                     case INS_SIGN: {
+                        if (cla != CLA) {
+                            THROW(APDU_CODE_COMMAND_NOT_ALLOWED);
+                        }
                         CHECK_PIN_VALIDATED()
                         handleSign(flags, tx, rx);
                         break;
                     }
 
                     case INS_SIGN_HASH: {
+                        if (cla != CLA) {
+                            THROW(APDU_CODE_COMMAND_NOT_ALLOWED);
+                        }
                         CHECK_PIN_VALIDATED()
                         handleSignHash(flags, tx, rx);
                         break;

diff --git a/app/src/evm/crypto_eth.c b/app/src/evm/crypto_eth.c
@@ -118,8 +118,7 @@ zxerr_t _sign(uint8_t *output, uint16_t outputLen, const uint8_t *message, uint1
     const err_convert_e err_c =
         convertDERtoRSV(signature->der_signature, tmpInfo, signature->r, signature->s, &signature->v);
     if (err_c == no_error) {
-        *sigSize =
-            sizeof_field(signature_t, r) + sizeof_field(signature_t, s) + sizeof_field(signature_t, v) + signatureLength;
+        *sigSize = sizeof_field(signature_t, r) + sizeof_field(signature_t, s) + sizeof_field(signature_t, v);
         if (info != NULL) {
             *info = tmpInfo;
         }

diff --git a/deps/ledger-zxlib b/deps/ledger-zxlib
diff --git a/docs/APDUSPEC.md b/docs/APDUSPEC.md
@@ -44,7 +44,7 @@ The general structure of commands and responses is as follows:
 
 ## Command definition
 
-### GET_DEVICE_INFO
+### INS_GET_DEVICE_INFO
 
 #### Command
 
@@ -70,7 +70,7 @@ The general structure of commands and responses is as follows:
 
 ---
 
-### GET_VERSION
+### INS_GET_VERSION
 
 #### Command
 
@@ -211,6 +211,10 @@ All other packets/chunks contain data chunks that are described below
 
 ---
 
+## ETH INSTRUCTIONS
+
+For eth instructions the derivation path length can vary between 3 and 5 elements.
+
 ### INS_GET_ADDR_ETH
 
 #### Command
@@ -220,7 +224,7 @@ All other packets/chunks contain data chunks that are described below
 | CLA     | byte (1)        | Application Identifier    | 0xE0              |
 | INS     | byte (1)        | Instruction ID            | 0x02              |
 | P1      | byte (1)        | Request User confirmation | No = 0            |
-| P2      | byte (1)        | Chain code               | no chain code - 0x0 / chain code - 0x01           |
+| P2      | byte (1)        | Chain code                | no chain code - 0x0 / chain code - 0x01           |
 | L       | byte (1)        | Bytes in payload          | (depends)         |
 | Path[0] | byte (4)        | Derivation Path Data      | 0x8000002c        |
 | Path[1] | byte (4)        | Derivation Path Data      | 0x8000003c        |
@@ -280,3 +284,48 @@ All other packets/chunks contain data chunks that are described below
 | ------- | --------- | ----------- | ------------------------ |
 | SIG     | byte (65) | Signature   |                          |
 | SW1-SW2 | byte (2)  | Return code | see list of return codes |
+
+---
+
+### INS_SIGN_PERSONAL_MESSAGE
+
+#### Command
+
+| Field | Type     | Content                | Expected  |
+| ----- | -------- | ---------------------- | --------- |
+| CLA   | byte (1) | Application Identifier | 0xE0      |
+| INS   | byte (1) | Instruction ID         | 0x08      |
+| P1    | byte (1) | Payload desc           | 0x0 = first  |
+|       |          |                        | 0x80 = more   |
+|       |          |                        |   |
+| P2    | byte (1) | ----                   | not used  |
+| L     | byte (1) | Bytes in the payload       | (depends) |
+
+The first packet/chunk includes the derivation path but it can also include some bytes of the message to be signed.
+
+All other packets/chunks contain data chunks that are described below
+
+##### First Packet
+
+| Field   | Type     | Content              | Expected |
+| ------- | -------- | -------------------- | -------- |
+| Path[0] | byte (4) | Derivation Path Data | 44       |
+| Path[1] | byte (4) | Derivation Path Data | 60       |
+| Path[2] | byte (4) | Derivation Path Data | ?        |
+| Path[3] | byte (4) | Derivation Path Data | ?        |
+| Path[4] | byte (4) | Derivation Path Data | ?        |
+| Msg size| byte (4) | Size of msg to sign | ?        |
+| Msg | bytes... | Msg to Sign |          |
+
+##### Other Chunks/Packets
+
+| Field   | Type     | Content         | Expected |
+| ------- | -------- | --------------- | -------- |
+| Msg | bytes... | Msg to Sign |          |
+
+#### Response
+
+| Field   | Type      | Content     | Note                     |
+| ------- | --------- | ----------- | ------------------------ |
+| SIG     | byte (65) | Signature   |                          |
+| SW1-SW2 | byte (2)  | Return code | see list of return codes |
diff --git a/fuzz/parser_parse.cpp b/fuzz/parser_parse.cpp
@@ -3,7 +3,9 @@
 #include <cstdio>
 
 #include "parser.h"
-#include "zxformat.h"
+#include "parser_common.h"
+#include "parser_txdef.h"
+#include "zxmacros_x64.h"
 
 #ifdef NDEBUG
 #error "This fuzz target won't work correctly with NDEBUG defined, which will cause asserts to be eliminated"

diff --git a/fuzz/run-fuzz-crashes.py b/fuzz/run-fuzz-crashes.py
@@ -20,7 +20,7 @@
 
     artifact_dir = os.path.join('fuzz', 'corpora', f'{fuzzer}-artifacts')
     corpus_dir = os.path.join('fuzz', 'corpora', f'{fuzzer}')
-    fuzz_path = os.path.join(f'build/bin/fuzz-{fuzzer}')
+    fuzz_path = os.path.join(f'build/fuzz-{fuzzer}')
 
     os.makedirs(artifact_dir, exist_ok=True)
     os.makedirs(corpus_dir, exist_ok=True)

diff --git a/tests/expected_output.cpp b/tests/expected_output.cpp
@@ -17,11 +17,13 @@
 #include <coin.h>
 #include <fmt/core.h>
 
-#include <iostream>
+#include <cstdint>
+#include <string>
+#include <vector>
 
-#include "testcases.h"
+#include "json/value.h"
 #include "zxformat.h"
-#include "zxmacros.h"
+#include "zxmacros_x64.h"
 const uint32_t fieldSize = 39;
 
 template <typename S, typename... Args>

diff --git a/tests/parser_impl.cpp b/tests/parser_impl.cpp
@@ -14,20 +14,22 @@
  *  limitations under the License.
  ********************************************************************************/
 
-#include "parser_impl.h"
-
 #include <hexutils.h>
+#include <string.h>
 
-#include <iostream>
-#include <vector>
+#include <cstdint>
+#include <string>
 
 #include "bech32.h"
 #include "coin.h"
 #include "crypto_helper.h"
-#include "gmock/gmock.h"
+#include "gtest/gtest.h"
 #include "hexutils.h"
 #include "parser.h"
+#include "parser_common.h"
 #include "parser_txdef.h"
+#include "segwit_addr.h"
+#include "zxerror.h"
 
 extern "C" {
 #include "ripemd160.h"

diff --git a/tests/ui_tests.cpp b/tests/ui_tests.cpp
@@ -15,16 +15,27 @@
  ********************************************************************************/
 
 #include <hexutils.h>
-#include <json/json.h>
+#include <json/config.h>
+#include <json/reader.h>
+#include <json/value.h>
 #include <parser_txdef.h>
 
+#include <algorithm>
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
 #include <fstream>
 #include <iostream>
+#include <sstream>
+#include <string>
+#include <vector>
 
 #include "app_mode.h"
 #include "expected_output.h"
 #include "gmock/gmock.h"
+#include "gtest/gtest.h"
 #include "parser.h"
+#include "parser_common.h"
 #include "parser_eth.h"
 #include "testcases.h"
 #include "utils/common.h"

diff --git a/tests/utils/common.cpp b/tests/utils/common.cpp
@@ -17,9 +17,12 @@
 
 #include <parser.h>
 
+#include <cstdint>
 #include <sstream>
 #include <string>
+#include <vector>
 
+#include "parser_common.h"
 #include "parser_eth.h"
 
 std::vector<std::string> dumpUI(parser_context_t *ctx, uint16_t maxKeyLen, uint16_t maxValueLen, bool is_eth) {

diff --git a/tests_zemu/package.json b/tests_zemu/package.json
@@ -28,15 +28,15 @@
     "@types/jest": "^29.5.12",
     "@types/ledgerhq__hw-transport": "^4.21.4",
     "@types/secp256k1": "^4.0.6",
-    "@typescript-eslint/eslint-plugin": "^8.17.0",
-    "@typescript-eslint/parser": "^8.17.0",
+    "@typescript-eslint/eslint-plugin": "^8.18.1",
+    "@typescript-eslint/parser": "^8.18.1",
     "blakejs": "^1.1.1",
     "crypto-js": "4.2.0",
     "ed25519-supercop": "^2.0.1",
-    "eslint": "^9.16.0",
+    "eslint": "^9.17.0",
     "eslint-config-prettier": "^9.1.0",
     "eslint-plugin-import": "^2.24.2",
-    "eslint-plugin-jest": "^28.9.0",
+    "eslint-plugin-jest": "^28.10.0",
     "eslint-plugin-prettier": "^5.1.3",
     "jest": "29.7.0",
     "js-sha256": "0.11.0",

diff --git a/tests_zemu/snapshots/fl-mainmenu/00004.png b/tests_zemu/snapshots/fl-mainmenu/00004.png
diff --git a/tests_zemu/snapshots/s-mainmenu/00005.png b/tests_zemu/snapshots/s-mainmenu/00005.png
diff --git a/tests_zemu/snapshots/s-mainmenu/00009.png b/tests_zemu/snapshots/s-mainmenu/00009.png
diff --git a/tests_zemu/snapshots/sp-mainmenu/00005.png b/tests_zemu/snapshots/sp-mainmenu/00005.png
diff --git a/tests_zemu/snapshots/sp-mainmenu/00009.png b/tests_zemu/snapshots/sp-mainmenu/00009.png
diff --git a/tests_zemu/snapshots/st-mainmenu/00004.png b/tests_zemu/snapshots/st-mainmenu/00004.png
diff --git a/tests_zemu/snapshots/x-mainmenu/00005.png b/tests_zemu/snapshots/x-mainmenu/00005.png
diff --git a/tests_zemu/snapshots/x-mainmenu/00009.png b/tests_zemu/snapshots/x-mainmenu/00009.png
+18 −0		app/ui/view.c
+8 −0		app/ui/view.h
+22 −0		app/ui/view_internal.h
+107 −7		app/ui/view_s.c
+92 −0		app/ui/view_x.c
+2 −2		include/zxversion.h