Max APDU buffer size checks (#276)

- Renaming empty buffer error to more generic invalid buffer error
- Added max buffer size checks on powHSM, Ledger UI bootloader and Ledger UI heartbeat main I/O logic
- Removed lower untrusted SGX & TCPSigner I/O layer disconnection on inconsistent request
- Added request buffer emptying after an inconsistent request to untrusted SGX & TCPSigner I/O layer
- Added new RawCommand test type to firmware test framework
- Added test cases for empty APDU and oversized APDU to firmware tests
- Added test cases for Ledger UI bootloader and Ledger UI heartbeat

firmware/src/common/src/apdu.h

@@ -88,7 +88,7 @@
 
 // Generic error codes used by both Signer and UI
 typedef enum {
-    ERR_EMPTY_BUFFER = 0x6982,
+    ERR_INVALID_BUFFER = 0x6982,
     ERR_INS_NOT_SUPPORTED = 0x6D00,
     APDU_OK = 0x9000,
 } err_code_generic_t;

firmware/src/ledger/ui/src/bootloader.c

@@ -86,10 +86,10 @@ unsigned int bootloader_process_apdu(volatile unsigned int rx,
                                      bootloader_mode_t mode) {
     unsigned int tx = 0;
 
-    // no apdu received, well, reset the session, and reset the
-    // bootloader configuration
-    if (rx == 0) {
-        THROW(ERR_EMPTY_BUFFER);
+    // no apdu received, or bigger-than-apdu-buffer bytes received =>
+    // well, reset the session, and reset the bootloader configuration
+    if (rx == 0 || rx > APDU_TOTAL_SIZE) {
+        THROW(ERR_INVALID_BUFFER);
     }
 
     if (APDU_CLA() != CLA) {

firmware/src/ledger/ui/src/ui_heartbeat.c

@@ -192,10 +192,10 @@ unsigned int ui_heartbeat_process_apdu(ui_heartbeat_t *ui_heartbeat_ctx,
                                        volatile unsigned int rx) {
     unsigned int tx = 0;
 
-    // no apdu received, well, reset the session, and reset the
-    // bootloader configuration
-    if (rx == 0) {
-        THROW(ERR_EMPTY_BUFFER);
+    // no apdu received, or bigger-than-apdu-buffer bytes received =>
+    // well, reset the session, and reset the heartbeat configuration
+    if (rx == 0 || rx > APDU_TOTAL_SIZE) {
+        THROW(ERR_INVALID_BUFFER);
     }
 
     if (APDU_CLA() != CLA) {

firmware/src/ledger/ui/test/bootloader/test_bootloader.c

@@ -593,10 +593,35 @@ void test_empty_buffer() {
     BEGIN_TRY {
         TRY {
             bootloader_process_apdu(rx, G_bootloader_mode);
-            // bootloader_process_apdu should throw ERR_EMPTY_BUFFER
+            // bootloader_process_apdu should throw ERR_INVALID_BUFFER
             ASSERT_FAIL();
         }
-        CATCH(ERR_EMPTY_BUFFER) {
+        CATCH(ERR_INVALID_BUFFER) {
+            return;
+        }
+        CATCH_OTHER(e) {
+            ASSERT_FAIL();
+        }
+        FINALLY {
+        }
+    }
+    END_TRY;
+}
+
+void test_buffer_too_big() {
+    printf("Test buffer too big...\n");
+
+    unsigned int rx;
+    reset_flags();
+    G_bootloader_mode = BOOTLOADER_MODE_DEFAULT;
+    rx = 1000;
+    BEGIN_TRY {
+        TRY {
+            bootloader_process_apdu(rx, G_bootloader_mode);
+            // bootloader_process_apdu should throw ERR_INVALID_BUFFER
+            ASSERT_FAIL();
+        }
+        CATCH(ERR_INVALID_BUFFER) {
             return;
         }
         CATCH_OTHER(e) {
@@ -690,6 +715,7 @@ int main() {
     test_end_nosig();
     test_invalid_command();
     test_empty_buffer();
+    test_buffer_too_big();
     test_no_cla();
     test_onboard_mode();
 

firmware/src/ledger/ui/test/ui_heartbeat/test_ui_heartbeat.c

@@ -68,7 +68,7 @@ static const unsigned char MOCK_SIGNER_HASH[] = {
 static sigaut_signer_t M_signer_info;
 
 unsigned int M_io_exchange_call_number;
-unsigned char M_apdu[sizeof(G_io_apdu_buffer)];
+unsigned char M_apdu[sizeof(G_io_apdu_buffer) * 2];
 unsigned short M_apdu_size;
 unsigned int M_txd;
 unsigned short M_last_exception;
@@ -147,7 +147,10 @@ unsigned short io_exchange(unsigned char channel_and_flags,
     assert(0 == channel_and_flags);
     assert(0 == tx_len);
 
-    memcpy(G_io_apdu_buffer, M_apdu, M_apdu_size);
+    memcpy(G_io_apdu_buffer,
+           M_apdu,
+           M_apdu_size > sizeof(G_io_apdu_buffer) ? sizeof(G_io_apdu_buffer)
+                                                  : M_apdu_size);
     return M_apdu_size;
 }
 
@@ -392,6 +395,23 @@ void test_empty_apdu() {
     assert_error(0x6982);
 }
 
+void test_apdu_too_big() {
+    printf("Test APDU too big...\n");
+
+    setup();
+
+    set_mock_apdu(
+        "\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99"
+        "\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99"
+        "\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99"
+        "\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99"
+        "\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99"
+        "\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99\x99");
+    ui_heartbeat_main(&M_wider_context.ui_heartbeat);
+
+    assert_error(0x6982);
+}
+
 void test_invalid_cla() {
     printf("Test invalid CLA...\n");
 
@@ -417,6 +437,7 @@ int main() {
     test_op_pubkey();
     test_op_invalid_op();
     test_empty_apdu();
+    test_apdu_too_big();
     test_invalid_cla();
     return 0;
 }

firmware/src/powhsm/src/hsm.c

@@ -92,9 +92,9 @@ static unsigned int hsm_process_command(volatile unsigned int rx) {
     unsigned int tx = 0;
     uint8_t pubkey_length;
 
-    // No apdu received
-    if (rx == 0) {
-        THROW(ERR_EMPTY_BUFFER);
+    // No apdu received, or bigger-than-apdu-buffer bytes received
+    if (rx == 0 || rx > APDU_TOTAL_SIZE) {
+        THROW(ERR_INVALID_BUFFER);
     }
 
     // Zero out commonly read APDU buffer offsets,

firmware/src/sgx/src/untrusted/io.c

@@ -27,6 +27,7 @@
 #include <unistd.h>
 #include <netdb.h>
 #include <netinet/tcp.h>
+#include <sys/ioctl.h>
 
 #include "io.h"
 #include "log.h"
@@ -168,15 +169,28 @@ unsigned short io_exchange(unsigned short tx) {
             if (rx > 0) {
                 // Read APDU from socket
                 readlen = read(connfd, io_apdu_buffer, sizeof(io_apdu_buffer));
-                if (readlen < 0 || (unsigned int)readlen != rx) {
-                    LOG("Error reading APDU (got %d bytes != %d bytes). "
-                        "Disconnected\n",
-                        readlen,
-                        rx);
+                if (readlen < 0) {
+                    LOG("Error reading APDU. Disconnected\n");
                     close_and_reset_fd(&connfd);
                     continue;
+                } else if ((unsigned int)readlen != rx) {
+                    LOG("Warning: APDU read length mismatch "
+                        "(got %d bytes, expected %u bytes). "
+                        "Resetting request buffer\n",
+                        readlen,
+                        rx);
+                    // Empty the request buffer
+                    int bytes_available;
+                    char c;
+                    if (ioctl(connfd, FIONREAD, &bytes_available) < 0) {
+                        LOG("Error peeking APDU. Disconnected\n");
+                        close_and_reset_fd(&connfd);
+                        continue;
+                    }
+                    while (bytes_available--)
+                        read(connfd, &c, 1);
                 }
-                LOG_HEX("I/O <=", io_apdu_buffer, rx);
+                LOG_HEX("I/O <=", io_apdu_buffer, readlen);
             } else {
                 // Empty packet
                 LOG("I/O <= <EMPTY MESSAGE>\n");

firmware/src/tcpsigner/src/hsmsim_io.c

@@ -32,6 +32,7 @@
 #include <netinet/tcp.h>
 #include <sys/socket.h>
 #include <sys/types.h>
+#include <sys/ioctl.h>
 
 #include "hal/log.h"
 #include "hsmsim_io.h"
@@ -238,22 +239,35 @@ static unsigned short io_exchange_server(unsigned short tx) {
 
         // Read APDU length
         // (encoded in 4 bytes network byte-order)
-        // (compatibility with LegerBlue commTCP.py)
+        // (compatibility with LedgerBlue commTCP.py)
         readlen = read(connfd, &rx_net, sizeof(rx_net));
         if (readlen == sizeof(rx_net)) {
             rx = ntohl(rx_net);
             if (rx > 0) {
                 // Read APDU from socket
                 readlen = read(connfd, io_apdu_buffer, sizeof(io_apdu_buffer));
-                if (readlen != rx) {
-                    LOG("Error reading APDU (got %d bytes != %d bytes). "
-                        "Disconnected\n",
-                        readlen,
-                        rx);
+                if (readlen < 0) {
+                    LOG("Error reading APDU. Disconnected\n");
                     connfd = 0;
                     continue;
+                } else if ((unsigned int)readlen != rx) {
+                    LOG("Warning: APDU read length mismatch "
+                        "(got %d bytes, expected %u bytes). "
+                        "Resetting request buffer\n",
+                        readlen,
+                        rx);
+                    // Empty the request buffer
+                    int bytes_available;
+                    char c;
+                    if (ioctl(connfd, FIONREAD, &bytes_available) < 0) {
+                        LOG("Error peeking APDU. Disconnected\n");
+                        connfd = 0;
+                        continue;
+                    }
+                    while (bytes_available--)
+                        read(connfd, &c, 1);
                 }
-                LOG_HEX("Dongle <=", io_apdu_buffer, rx);
+                LOG_HEX("Dongle <=", io_apdu_buffer, readlen);
             } else {
                 // Empty packet
                 LOG("Dongle <= <EMPTY MESSAGE>\n");

firmware/test/cases/__init__.py

@@ -35,3 +35,4 @@
 from .nvm_stats import NvmStats
 from .admin_is_onboarded import AdminIsOnboarded
 from .heartbeat import Heartbeat
+from .raw_command import RawCommand

firmware/test/cases/raw_command.py

@@ -0,0 +1,85 @@
+# The MIT License (MIT)
+#
+# Copyright (c) 2021 RSK Labs Ltd
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy of
+# this software and associated documentation files (the "Software"), to deal in
+# the Software without restriction, including without limitation the rights to
+# use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
+# of the Software, and to permit persons to whom the Software is furnished to do
+# so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in all
+# copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+# SOFTWARE.
+
+from .case import TestCase, TestCaseError
+from ledgerblue.comm import CommException
+from comm.utils import is_nonempty_hex_string
+
+
+class RawCommand(TestCase):
+    VALUE_FALSE = 0
+    VALUE_TRUE = 1
+
+    @classmethod
+    def op_name(cls):
+        return "rawCommand"
+
+    def __init__(self, spec):
+        if not is_nonempty_hex_string(spec.get("command")) and spec.get("command") != "":
+            raise TestCaseError(f"Invalid raw command: {spec.get("command")}")
+        self.command = bytes.fromhex(spec.get("command"))
+
+        # Override default constructor behavior for expectation
+        self.expected = spec.get("expected")
+        spec["expected"] = True
+
+        super().__init__(spec)
+
+        # Normal result expectation
+        if not is_nonempty_hex_string(self.expected):
+            self.expected = None
+        else:
+            self.expected = bytes.fromhex(self.expected)
+
+        # Exception expectation
+        self.exception = spec.get("exception")
+        if self.exception is not None:
+            self.exception_desc = self.exception
+            self.exception = self._parse_int(self.exception)
+
+    def run(self, dongle, debug, run_args):
+        try:
+            result = dongle.dongle.exchange(self.command)
+            if self.expected is not None and self.expected != result:
+                raise TestCaseError("Unexpected raw command result: "
+                                    f"expected {self.expected.hex()} but got "
+                                    f"{result.hex()}")
+            if self.exception is not None:
+                raise TestCaseError(f"Expected command to raise an {self.exception_desc} "
+                                    f"exception but got 0x{result.hex()} as "
+                                    "result instead")
+        except TestCaseError as e:
+            raise e
+        except (RuntimeError, CommException) as e:
+            if self.exception is not None:
+                if type(e) == CommException:
+                    error_code = e.sw
+                    error_code_desc = hex(error_code)
+                else:
+                    error_code = None
+                    error_code_desc = "no error"
+
+                if self.exception != error_code:
+                    raise TestCaseError("Expected error code {self.exception_desc} "
+                                        f"but got {error_code_desc}")
+            else:
+                raise TestCaseError(str(e))

firmware/test/resources/900-larger-than-apdu-command-fails.json

@@ -0,0 +1,7 @@
+{
+    "name": "Larger than APDU command fails",
+    "operation": "rawCommand",
+    "command": "804300aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+    "exception": "0x6982",
+    "runOn": "simulator"
+}

firmware/test/resources/901-empty-apdu-command-fails.json

@@ -0,0 +1,7 @@
+{
+    "name": "Empty APDU command fails",
+    "operation": "rawCommand",
+    "command": "",
+    "exception": "0x6982",
+    "runOn": "simulator"
+}