fix(zcash): defer checked batch cleanup after signing
What the developer wrote
Authored by Adam Tucker
fix(zcash): defer checked batch cleanup after signing
What changed, and why it matters
This patch changes when a Zcash batch signing screen frees a sensitive internal data structure. Instead of destroying the data immediately when the page is cleared, it now schedules the cleanup to run after the signing operation has finished. The comment in the code says this prevents the destructor from 'overtaking' the signing task, which suggests the old code could have freed memory while signing still needed it. That kind of use-after-free or premature-free bug can corrupt data or crash the device during a transaction, and in security-sensitive signing code it could theoretically affect signature correctness or leak secrets.
Treat as a likely security fix for a use-after-free / premature-free in the Zcash signing flow. Review whether any released firmware versions contain the synchronous free path and assess if a security advisory or firmware update is warranted. Verify that AsyncExecute guarantees FIFO ordering and that no other callers free g_checkedBatch while signing is pending.
Security signals we found
Use-after-free / premature-free risk in cryptographic signing path
Async deferred cleanup introduced to avoid race with FIFO signing task
Inline comment explicitly describes security-relevant ordering constraint
Change affects Zcash batch signing UI flow
Evidence from the diff
In gui_zcash_batch_widgets.c, ClearPageData() previously called FreeCheckedBatch() synchronously, which invoked free_zcash_checked_pczt() on g_checkedBatch. The new code adds a ZcashCheckedBatchCleanup_t wrapper and an async task FreeCheckedBatchAsync executed via AsyncExecute on the same FIFO task queue. The inline comment states: ‘Signing uses this pointer on the same FIFO task, so its destructor cannot overtake it.’ This implies the original synchronous free could run before the signing task completed, creating a time-of-use/time-of-free issue. The patch defers destruction until after signing, eliminating that race. No direct exploit path is visible in the diff, but freeing a structure that is still referenced by an in-flight signing operation is a memory-safety defect in cryptographic code.
Changed components
src/ui/gui_widgets/multi/cypherpunk/gui_zcash_batch_widgets.cZcash batch signing UIZcashCheckedPczt / g_checkedBatch lifecycleInspect captured patch +32 / −1
diff --git a/src/ui/gui_widgets/multi/cypherpunk/gui_zcash_batch_widgets.c b/src/ui/gui_widgets/multi/cypherpunk/gui_zcash_batch_widgets.c
index be36947..29ebee6 100644
--- a/src/ui/gui_widgets/multi/cypherpunk/gui_zcash_batch_widgets.c
+++ b/src/ui/gui_widgets/multi/cypherpunk/gui_zcash_batch_widgets.c
@@ -12,6 +12,7 @@
#include "gui_zcash.h"
#include "keystore.h"
#include "screen_manager.h"
+#include "fetch_sensitive_data_task.h"
#include "general/eapdu_services/service_resolve_ur.h"
#include "user_memory.h"
@@ -46,6 +47,10 @@ static lv_obj_t *g_signSlider = NULL;
static lv_obj_t *g_parseErrorHintBox = NULL;
static KeyboardWidget_t *g_keyboardWidget = NULL;
+typedef struct {
+ ZcashCheckedPczt *checkedBatch;
+} ZcashCheckedBatchCleanup_t;
+
static void *GuiParseZcashBatchData(void);
static void CheckSliderProcessHandler(lv_event_t *e);
static void GuiRenderCurrentTransaction(bool showSignSlider);
@@ -67,6 +72,32 @@ static void FreeCheckedBatch(void)
}
}
+static int32_t FreeCheckedBatchAsync(const void *data, uint32_t dataLen)
+{
+ if (data == NULL || dataLen != sizeof(ZcashCheckedBatchCleanup_t)) {
+ return ERR_GENERAL_FAIL;
+ }
+
+ const ZcashCheckedBatchCleanup_t *cleanup = data;
+ free_zcash_checked_pczt(cleanup->checkedBatch);
+ return SUCCESS_CODE;
+}
+
+static void DeferFreeCheckedBatch(void)
+{
+ if (g_checkedBatch == NULL) {
+ return;
+ }
+
+ ZcashCheckedBatchCleanup_t cleanup = {
+ .checkedBatch = g_checkedBatch,
+ };
+ g_checkedBatch = NULL;
+
+ // Signing uses this pointer on the same FIFO task, so its destructor cannot overtake it.
+ AsyncExecute(FreeCheckedBatchAsync, &cleanup, sizeof(cleanup));
+}
+
static void ClearPageData(void)
{
g_currentTxIndex = 0;
@@ -74,7 +105,7 @@ static void ClearPageData(void)
g_currentTransaction = NULL;
g_displayZcashBatch = NULL;
- FreeCheckedBatch();
+ DeferFreeCheckedBatch();
if (g_parseResult != NULL) {
free_TransactionParseResult_DisplayZcashBatch(g_parseResult);
Why this scored 42/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.