Fix: NVM Read: Enforce Objects Boundaries (#182)

rizlik · web-flow · commit 2e2281a0ea5f · 2025-10-09T09:16:49.000-04:00
* nvm_flash: checks object boundaries in reads

* test: nvm: test object bounds check in nvm reads
diff --git a/src/wh_nvm_flash.c b/src/wh_nvm_flash.c
@@ -665,6 +665,16 @@ static int nfObject_ReadDataBytes(whNvmFlashContext* context, int partition,
     start = context->directory.objects[object_index].state.start;
     startOffset = nfPartition_DataOffset(context, partition) + start;
 
+    /* object bounds checks, do both to avoid integer overflow checks */
+    if (byte_offset >=
+        (context->directory.objects[object_index].metadata.len)) {
+        return WH_ERROR_BADARGS;
+    }
+    if ((byte_offset + byte_count) >
+        (context->directory.objects[object_index].metadata.len)) {
+        return WH_ERROR_BADARGS;
+    }
+
     /* Ensure we don't read off the end of the active partition */
     if (WH_ERROR_OK != nfPartition_CheckDataRange(context, partition,
                                     startOffset * WHFU_BYTES_PER_UNIT + byte_offset,
diff --git a/src/wh_server_cert.c b/src/wh_server_cert.c
@@ -228,7 +228,6 @@ int wh_Server_CertReadTrusted(whServerContext* server, whNvmId id,
                               uint8_t* cert, uint32_t* inout_cert_len)
 {
     int           rc;
-    whNvmSize     userLen;
     whNvmMetadata meta;
 
     if ((server == NULL) || (cert == NULL) || (inout_cert_len == NULL) ||
@@ -243,17 +242,16 @@ int wh_Server_CertReadTrusted(whServerContext* server, whNvmId id,
         return rc;
     }
 
-    /* Clamp the input length to the actual length of the certificate. This will
-     * be reflected back to the user on length mismatch failure */
-    userLen         = *inout_cert_len;
-    *inout_cert_len = meta.len;
-
     /* Check if the provided buffer is large enough */
-    if (meta.len > userLen) {
+    if (meta.len > *inout_cert_len) {
         return WH_ERROR_BUFFER_SIZE;
     }
 
-    return wh_Nvm_Read(server->nvm, id, 0, userLen, cert);
+    /* Clamp the input length to the actual length of the certificate. This will
+     * be reflected back to the user on length mismatch failure */
+    *inout_cert_len = meta.len;
+
+    return wh_Nvm_Read(server->nvm, id, 0, meta.len, cert);
 }
 
 /* Verify a certificate against trusted certificates */
diff --git a/src/wh_server_nvm.c b/src/wh_server_nvm.c
@@ -43,6 +43,46 @@
 #include "wolfhsm/wh_server.h"
 #include "wolfhsm/wh_server_nvm.h"
 
+/* Handle NVM read, do access checking and clamping */
+static int _HandleNvmRead(whServerContext* server, uint8_t* out_data,
+                          whNvmSize offset, whNvmSize len, whNvmSize* out_len,
+                          whNvmId id)
+{
+    whNvmMetadata meta;
+    int32_t       rc;
+
+    if ((server == NULL) || (out_data == NULL) || (out_len == NULL)) {
+        return WH_ERROR_BADARGS;
+    }
+
+    if (len > WH_MESSAGE_NVM_MAX_READ_LEN) {
+        return WH_ERROR_ABORTED;
+    }
+
+    rc = wh_Nvm_GetMetadata(server->nvm, id, &meta);
+    if (rc != WH_ERROR_OK) {
+        return rc;
+    }
+
+    if (meta.flags & WH_NVM_FLAGS_NONEXPORTABLE) {
+        return WH_ERROR_ACCESS;
+    }
+
+    if (offset >= meta.len)
+        return WH_ERROR_BADARGS;
+
+    /* Clamp length to object size */
+    if ((offset + len) > meta.len) {
+        len = meta.len - offset;
+    }
+
+    rc = wh_Nvm_Read(server->nvm, id, offset, len, out_data);
+    if (rc != WH_ERROR_OK)
+        return rc;
+    *out_len = len;
+    return WH_ERROR_OK;
+}
+
 int wh_Server_HandleNvmRequest(whServerContext* server,
         uint16_t magic, uint16_t action, uint16_t seq,
         uint16_t req_size, const void* req_packet,
@@ -240,44 +280,30 @@ int wh_Server_HandleNvmRequest(whServerContext* server,
 
     case WH_MESSAGE_NVM_ACTION_READ:
     {
-        whMessageNvm_ReadRequest req = {0};
-        whMessageNvm_ReadResponse resp = {0};
-        uint16_t hdr_len = sizeof(resp);
-        uint8_t* data = (uint8_t*)resp_packet + hdr_len;
-        uint16_t data_len = 0;
-        whNvmMetadata             meta     = {0};
+        whMessageNvm_ReadRequest  req      = {0};
+        whMessageNvm_ReadResponse resp     = {0};
+        uint16_t                  hdr_len  = sizeof(resp);
+        uint8_t*                  data     = (uint8_t*)resp_packet + hdr_len;
+        whNvmSize                 data_len = 0;
 
         if (req_size != sizeof(req)) {
             /* Request is malformed */
             resp.rc = WH_ERROR_ABORTED;
-        } else {
+        }
+        else {
             /* Convert request struct */
-            wh_MessageNvm_TranslateReadRequest(magic,
-                    (whMessageNvm_ReadRequest*)req_packet, &req);
+            wh_MessageNvm_TranslateReadRequest(
+                magic, (whMessageNvm_ReadRequest*)req_packet, &req);
 
-            if (req.data_len > WH_MESSAGE_NVM_MAX_READ_LEN) {
-                resp.rc = WH_ERROR_ABORTED;
-            } else {
-                /* Check metadata for non-exportable flag */
-                resp.rc = wh_Nvm_GetMetadata(server->nvm, req.id, &meta);
-                if (resp.rc == 0) {
-                    if (meta.flags & WH_NVM_FLAGS_NONEXPORTABLE) {
-                        resp.rc = WH_ERROR_ACCESS;
-                    }
-                    else {
-                        /* Process the Read action */
-                        resp.rc = wh_Nvm_Read(server->nvm, req.id, req.offset,
-                                              req.data_len, data);
-                        if (resp.rc == 0) {
-                            data_len = req.data_len;
-                        }
-                    }
-                }
+            resp.rc = _HandleNvmRead(server, data, req.offset, req.data_len,
+                                     &req.data_len, req.id);
+            if (resp.rc == WH_ERROR_OK) {
+                data_len = req.data_len;
             }
         }
         /* Convert the response struct */
-        wh_MessageNvm_TranslateReadResponse(magic,
-                &resp, (whMessageNvm_ReadResponse*)resp_packet);
+        wh_MessageNvm_TranslateReadResponse(
+            magic, &resp, (whMessageNvm_ReadResponse*)resp_packet);
         *out_resp_size = sizeof(resp) + data_len;
     }; break;
 
@@ -338,6 +364,7 @@ int wh_Server_HandleNvmRequest(whServerContext* server,
         whMessageNvm_ReadDmaRequest req  = {0};
         whMessageNvm_SimpleResponse resp = {0};
         whNvmMetadata               meta = {0};
+        whNvmSize                   read_len;
         void*                       data = NULL;
 
         if (req_size != sizeof(req)) {
@@ -355,6 +382,23 @@ int wh_Server_HandleNvmRequest(whServerContext* server,
                 resp.rc = WH_ERROR_ACCESS;
             }
         }
+
+        if (resp.rc == 0) {
+            if (req.offset >= meta.len) {
+                resp.rc = WH_ERROR_BADARGS;
+            }
+        }
+
+        if (resp.rc == 0) {
+            read_len = req.data_len;
+            /* Clamp length to object size */
+            if ((req.offset + read_len) > meta.len) {
+                read_len = meta.len - req.offset;
+            }
+        }
+
+        /* use unclamped length for DMA address processing in case DMA callbacks
+         * are sensible to alignment and/or size */
         if (resp.rc == 0) {
             /* perform platform-specific host address processing */
             resp.rc = wh_Server_DmaProcessClientAddress(
@@ -363,8 +407,8 @@ int wh_Server_HandleNvmRequest(whServerContext* server,
         }
         if (resp.rc == 0) {
             /* Process the Read action */
-            resp.rc = wh_Nvm_Read(server->nvm, req.id, req.offset, req.data_len,
-                    (uint8_t*)data);
+            resp.rc = wh_Nvm_Read(server->nvm, req.id, req.offset, read_len,
+                                  (uint8_t*)data);
         }
         if (resp.rc == 0) {
             /* perform platform-specific host address processing */
diff --git a/test/wh_test_clientserver.c b/test/wh_test_clientserver.c
@@ -669,6 +669,64 @@ static int _clientServerSequentialTestConnectCb(void*           context,
                                   connected);
 }
 
+static int _testOutOfBoundsNvmReads(whClientContext* client,
+                                    whServerContext* server, whNvmId id)
+{
+    uint8_t       buffer[WOLFHSM_CFG_COMM_DATA_LEN];
+    whNvmMetadata meta;
+    whNvmSize     off, len;
+    int32_t       server_rc;
+
+    /* Get object metadata */
+    WH_TEST_RETURN_ON_FAIL(wh_Client_NvmGetMetadataRequest(client, id));
+    WH_TEST_RETURN_ON_FAIL(wh_Server_HandleRequestMessage(server));
+    WH_TEST_RETURN_ON_FAIL(wh_Client_NvmGetMetadataResponse(
+        client, &server_rc, &meta.id, &meta.access, &meta.flags, &meta.len,
+        sizeof(meta.label), meta.label));
+    WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK);
+
+    /* Try to read len + 1 bytes, should clamp to len */
+    off = 0;
+    len = meta.len + 1;
+    WH_TEST_RETURN_ON_FAIL(wh_Client_NvmReadRequest(client, id, off, len));
+    WH_TEST_RETURN_ON_FAIL(wh_Server_HandleRequestMessage(server));
+    WH_TEST_RETURN_ON_FAIL(
+        wh_Client_NvmReadResponse(client, &server_rc, &len, buffer));
+    WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK);
+    WH_TEST_ASSERT_RETURN(len == meta.len);
+
+    /* Try to read len bytes starting at 1 should clamp to len - 1 */
+    off = 1;
+    len = meta.len;
+    WH_TEST_RETURN_ON_FAIL(wh_Client_NvmReadRequest(client, id, off, len));
+    WH_TEST_RETURN_ON_FAIL(wh_Server_HandleRequestMessage(server));
+    WH_TEST_RETURN_ON_FAIL(
+        wh_Client_NvmReadResponse(client, &server_rc, &len, buffer));
+    WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK);
+    WH_TEST_ASSERT_RETURN(len == meta.len - 1);
+
+    /* Try to read starting at len - 1 len bytes, should clamp to 1 */
+    off = meta.len - 1;
+    len = meta.len;
+    WH_TEST_RETURN_ON_FAIL(wh_Client_NvmReadRequest(client, id, off, len));
+    WH_TEST_RETURN_ON_FAIL(wh_Server_HandleRequestMessage(server));
+    WH_TEST_RETURN_ON_FAIL(
+        wh_Client_NvmReadResponse(client, &server_rc, &len, buffer));
+    WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK);
+    WH_TEST_ASSERT_RETURN(len == 1);
+
+    /* Try to read starting at len, should fail */
+    off = meta.len;
+    len = 0;
+    WH_TEST_RETURN_ON_FAIL(wh_Client_NvmReadRequest(client, id, off, len));
+    WH_TEST_RETURN_ON_FAIL(wh_Server_HandleRequestMessage(server));
+    WH_TEST_RETURN_ON_FAIL(
+        wh_Client_NvmReadResponse(client, &server_rc, &len, buffer));
+    WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_BADARGS);
+
+    return WH_ERROR_OK;
+}
+
 int whTest_ClientServerSequential(void)
 {
     int ret = 0;
@@ -1016,6 +1074,9 @@ int whTest_ClientServerSequential(void)
         WH_TEST_ASSERT_RETURN(0 == memcmp(send_buffer, recv_buffer, len));
     }
 
+    /* Perform out-of-bounds read tests on first object written */
+    WH_TEST_RETURN_ON_FAIL(_testOutOfBoundsNvmReads(client, server, 20));
+
     whNvmAccess list_access = WH_NVM_ACCESS_ANY;
     whNvmFlags  list_flags  = WH_NVM_FLAGS_NONE;
     whNvmId     list_id     = 0;
