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.

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

Audit fixes #32

Merged
merged 7 commits into from
Dec 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/Makefile.version
Original file line number Diff line number Diff line change
Expand Up @@ -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
12 changes: 12 additions & 0 deletions app/src/apdu_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Comment on lines +271 to +273
Copy link
Member

Choose a reason for hiding this comment

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

Why are we doing this so explicit when we don't do the same on other apps? any difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is discussible, and I'm glad you point it out.
There might be a case where the transaction has ETH_CLA, and it comes with an ETH instruction not supported but that collides with a flare app instruction.
The app would treat this ETH transaction has a flare transaction.
The format of the transactions is different, so I would say it's impossible the app would parse and sign with success an ETH transaction using the flare parser.
But the auditors pointed this out, so I decided to add that extra check to make sure only flare transactions go through.
What do you think @emmanuelm41 ?

CHECK_PIN_VALIDATED()
handleSignHash(flags, tx, rx);
break;
Expand Down
3 changes: 1 addition & 2 deletions app/src/evm/crypto_eth.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

Was this signaatureLength empty (0) before? or was not this working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the rypto-sign for non-evm transactions, we are only returning the signature in the r,s,v format.
Here we should be doing the same because the DER format was not needed.

*sigSize = sizeof_field(signature_t, r) + sizeof_field(signature_t, s) + sizeof_field(signature_t, v);
if (info != NULL) {
*info = tmpInfo;
}
Expand Down
2 changes: 1 addition & 1 deletion deps/ledger-zxlib
55 changes: 52 additions & 3 deletions docs/APDUSPEC.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ The general structure of commands and responses is as follows:

## Command definition

### GET_DEVICE_INFO
### INS_GET_DEVICE_INFO

#### Command

Expand All @@ -70,7 +70,7 @@ The general structure of commands and responses is as follows:

---

### GET_VERSION
### INS_GET_VERSION

#### Command

Expand Down Expand Up @@ -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
Expand All @@ -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 |
Expand Down Expand Up @@ -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 |
4 changes: 3 additions & 1 deletion fuzz/parser_parse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion fuzz/run-fuzz-crashes.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 5 additions & 3 deletions tests/expected_output.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
12 changes: 7 additions & 5 deletions tests/parser_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
13 changes: 12 additions & 1 deletion tests/ui_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
3 changes: 3 additions & 0 deletions tests/utils/common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
8 changes: 4 additions & 4 deletions tests_zemu/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Binary file modified tests_zemu/snapshots/fl-mainmenu/00004.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/s-mainmenu/00005.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/s-mainmenu/00009.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/sp-mainmenu/00005.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/sp-mainmenu/00009.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/st-mainmenu/00004.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/x-mainmenu/00005.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/x-mainmenu/00009.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading