Harden Stratum V1 network input validation - #1844
Draft
skot wants to merge 4 commits into
Draft
Conversation
Test Results 2 files ±0 2 suites ±0 1s ⏱️ ±0s For more details on these failures, see this check. Results for commit 01d7a0a. ± Comparison against base commit d554968. |
Collaborator
Author
|
shout out to @johnny9 for help with this. |
johnny9
reviewed
Aug 3, 2026
johnny9
left a comment
johnny9
left a comment
Collaborator
There was a problem hiding this comment.
I reviewed the three commits at 01d7a0ae in an isolated worktree.
The explicit-length receive buffer is a good direction, but I do not think this is ready to merge yet. Pool-controlled input can still reach an out-of-bounds coinbase varint read and can nearly exhaust the complete 8 KiB mining-task stack through values assembled from two separately capped JSON lines. The new receive tests also do not currently run: upstream QEMU CI fails all three during SPIRAM-only initialization.
I left six inline findings, ordered by priority, covering those issues plus exact hex validation, connection-boundary buffer reset, and allocation-failure publication. The firmware build and frontend tests are green; the backend Unit Test job is red with the three new receive tests failing.
| if (!cJSON_IsString(job_id_item) || | ||
| !cJSON_IsString(prev_block_hash_item) || | ||
| !cJSON_IsString(coinbase_1_item) || | ||
| !cJSON_IsString(coinbase_2_item) || |
Collaborator
There was a problem hiding this comment.
[P1] Please bound-check the serialized coinbase before it reaches the decoder. This guard accepts any string for coinbase_2, so a valid-looking but truncated value such as a four-byte sequence followed by a terminal fd varint marker reaches coinbase_decode_varint(). That function then reads two bytes beyond the allocation because it receives no buffer length. I reproduced the exact decoder logic under AddressSanitizer as a heap-buffer-overflow. Please make varint decoding length-aware and fail the notification when either the output-count or script-length varint is truncated.
Collaborator
There was a problem hiding this comment.
Proposed patch (plus updating the existing varint tests to pass sizeof(data) and adding truncated FD/FE/FF cases):
diff --git a/components/stratum/include/coinbase_decoder.h b/components/stratum/include/coinbase_decoder.h
@@
-uint64_t coinbase_decode_varint(const uint8_t *data, int *offset);
+bool coinbase_decode_varint(const uint8_t *data, size_t data_len,
+ int *offset, uint64_t *value);
diff --git a/components/stratum/coinbase_decoder.c b/components/stratum/coinbase_decoder.c
@@
-uint64_t coinbase_decode_varint(const uint8_t *data, int *offset) {
- uint8_t first_byte = data[*offset];
- (*offset)++;
- /* decode without a length */
-}
+bool coinbase_decode_varint(const uint8_t *data, size_t data_len,
+ int *offset, uint64_t *value)
+{
+ if (!data || !offset || !value || *offset < 0 ||
+ (size_t)*offset >= data_len) {
+ return false;
+ }
+
+ uint8_t first = data[(*offset)++];
+ if (first < 0xFD) {
+ *value = first;
+ return true;
+ }
+
+ size_t payload_len = first == 0xFD ? 2 : first == 0xFE ? 4 : 8;
+ if (payload_len > data_len - (size_t)*offset) {
+ return false;
+ }
+
+ uint64_t decoded = 0;
+ for (size_t i = 0; i < payload_len; ++i) {
+ decoded |= (uint64_t)data[*offset + (int)i] << (i * 8);
+ }
+ *offset += (int)payload_len;
+ *value = decoded;
+ return true;
+}
@@
- uint64_t num_outputs = coinbase_decode_varint(coinbase_2_bin, &offset);
+ uint64_t num_outputs;
+ if (!coinbase_decode_varint(coinbase_2_bin, coinbase_2_len,
+ &offset, &num_outputs)) {
+ free(coinbase_2_bin);
+ return ESP_ERR_INVALID_ARG;
+ }
@@
- uint64_t script_len = coinbase_decode_varint(coinbase_2_bin, &offset);
- if (offset + script_len > coinbase_2_len) break;
+ uint64_t script_len;
+ if (!coinbase_decode_varint(coinbase_2_bin, coinbase_2_len,
+ &offset, &script_len) ||
+ script_len > (uint64_t)(coinbase_2_len - offset)) {
+ free(coinbase_2_bin);
+ return ESP_ERR_INVALID_ARG;
+ }| return false; | ||
| } | ||
|
|
||
| int extranonce_2_len = extranonce2_len->valueint; |
Collaborator
There was a problem hiding this comment.
[P1] This constrains extranonce2_size, but the separately supplied extranonce1 remains unbounded and is later combined with both coinbase strings in a VLA. Two individually legal 8 KiB JSON lines can carry about 8,150 extranonce characters plus 8,026 coinbase characters; with a 32-byte extranonce2 that produces an approximately 8,120-byte coinbase_tx_bin inside the 8,192-byte create_jobs_task stack, before any call frames or SHA-256 state. Please move the assembled transaction off the stack, propagate allocation/size failure, and retain an explicit upper bound.
Collaborator
There was a problem hiding this comment.
Proposed patch: keep the existing wire-size compatibility, but bound the assembled binary and allocate it on the heap so it cannot consume the task stack. Existing callers in tests may ignore the new Boolean result; the production caller must check it.
diff --git a/components/stratum/include/mining.h b/components/stratum/include/mining.h
@@
-#include <stdint.h>
+#include <stdbool.h>
+#include <stdint.h>
@@
-void calculate_coinbase_tx_hash(const char *coinbase_1, const char *coinbase_2,
- const char *extranonce, const char *extranonce_2, uint8_t dest[32]);
+bool calculate_coinbase_tx_hash(const char *coinbase_1, const char *coinbase_2,
+ const char *extranonce, const char *extranonce_2,
+ uint8_t dest[32]);
diff --git a/components/stratum/mining.c b/components/stratum/mining.c
@@
-void calculate_coinbase_tx_hash(const char *coinbase_1, const char *coinbase_2, const char *extranonce, const char *extranonce_2, uint8_t dest[32])
+#define MAX_COINBASE_TX_BYTES 8192
+
+bool calculate_coinbase_tx_hash(const char *coinbase_1, const char *coinbase_2,
+ const char *extranonce, const char *extranonce_2,
+ uint8_t dest[32])
{
size_t len1 = strlen(coinbase_1);
size_t len2 = strlen(extranonce);
size_t len3 = strlen(extranonce_2);
size_t len4 = strlen(coinbase_2);
- size_t coinbase_tx_bin_len = (len1 + len2 + len3 + len4) / 2;
- uint8_t coinbase_tx_bin[coinbase_tx_bin_len];
+ if ((len1 | len2 | len3 | len4) & 1U ||
+ len1 > SIZE_MAX - len2 || len1 + len2 > SIZE_MAX - len3 ||
+ len1 + len2 + len3 > SIZE_MAX - len4) {
+ return false;
+ }
+ size_t coinbase_tx_bin_len = (len1 + len2 + len3 + len4) / 2;
+ if (coinbase_tx_bin_len > MAX_COINBASE_TX_BYTES) return false;
+
+ uint8_t *coinbase_tx_bin = malloc(coinbase_tx_bin_len);
+ if (!coinbase_tx_bin) return false;
@@
- double_sha256_bin(coinbase_tx_bin, coinbase_tx_bin_len, dest);
+ bool complete = bin_offset == coinbase_tx_bin_len;
+ if (complete) double_sha256_bin(coinbase_tx_bin, coinbase_tx_bin_len, dest);
+ free(coinbase_tx_bin);
+ return complete;
}
diff --git a/main/tasks/create_jobs_task.c b/main/tasks/create_jobs_task.c
@@
- calculate_coinbase_tx_hash(notification->coinbase_1, notification->coinbase_2, GLOBAL_STATE->extranonce_str, extranonce_2_str, coinbase_tx_hash);
+ if (!calculate_coinbase_tx_hash(notification->coinbase_1, notification->coinbase_2,
+ GLOBAL_STATE->extranonce_str, extranonce_2_str,
+ coinbase_tx_hash)) {
+ ESP_LOGE(TAG, "Invalid or oversized coinbase transaction, skipping job");
+ return;
+ }| exit(1); | ||
| ESP_LOGE(TAG, "Failed to allocate Stratum request timings"); | ||
| STRATUM_V1_cleanup_buffer(); | ||
| return false; |
Collaborator
There was a problem hiding this comment.
[P1] This makes all three new receive-buffer tests fail in the repository's QEMU CI before they exercise the buffer. The test firmware has no usable MALLOC_CAP_SPIRAM, so STRATUM_V1_initialize_buffer() returns false at every new test's setup assertion; the current Unit Test run reports exactly those three failures. Please separate receive-buffer initialization from the optional request-timing allocation (or otherwise provide a test-compatible allocation path) so the new regressions actually run.
Collaborator
There was a problem hiding this comment.
Proposed patch: make the buffer initializer responsible only for buffer state, and add a session initializer for the production task's request-timing state. The receive tests then exercise the real buffer without requiring PSRAM.
diff --git a/components/stratum/include/stratum_api.h b/components/stratum/include/stratum_api.h
@@
+bool STRATUM_V1_initialize(void);
bool STRATUM_V1_initialize_buffer(void);
diff --git a/components/stratum/stratum_api.c b/components/stratum/stratum_api.c
@@
bool STRATUM_V1_initialize_buffer(void)
{
STRATUM_V1_cleanup_buffer();
json_rpc_buffer = malloc(BUFFER_SIZE);
@@
json_rpc_buffer_used = 0;
json_rpc_buffer[0] = '\0';
+ return true;
+}
+bool STRATUM_V1_initialize(void)
+{
+ if (!STRATUM_V1_initialize_buffer()) return false;
+
if (request_timings == NULL) {
request_timings = heap_caps_malloc(sizeof(RequestTiming) * MAX_REQUEST_IDS,
MALLOC_CAP_SPIRAM);
@@
}
return true;
}
diff --git a/main/tasks/stratum_v1_task.c b/main/tasks/stratum_v1_task.c
@@
- if (!STRATUM_V1_initialize_buffer()) {
+ if (!STRATUM_V1_initialize()) {
ESP_LOGE(TAG, "Failed to initialize Stratum V1 receive state, notifying coordinator");STRATUM_V1_receive_jsonrpc_line() can continue lazily calling the buffer-only initializer. get_request_timing() already safely handles a null timing table.
|
|
||
| for (size_t i = 0; i < new_work->n_merkle_branches; i++) { | ||
| cJSON *branch = cJSON_GetArrayItem(merkle_branch, i); | ||
| hex2bin(branch->valuestring, |
Collaborator
There was a problem hiding this comment.
[P2] Please validate encoding and exact widths before calling hex2bin()/strtoul(). A Merkle branch of "00" is accepted here and initializes only one byte of its 32-byte allocation; hashing consumes the other 31 uninitialized bytes. A short previous hash has the same problem on the stack, invalid characters silently decode as zero, and the 32-bit fields accept empty or partial prefixes. Require 64 hex characters for hashes and Merkle elements, even valid hex for both coinbase parts, and exactly eight hex characters for version/target/ntime.
Collaborator
There was a problem hiding this comment.
Proposed parser patch, followed by table-driven regressions for short/non-hex hashes and Merkle elements, odd/non-hex coinbase parts, and short/non-hex/long 32-bit fields:
diff --git a/components/stratum/stratum_api.c b/components/stratum/stratum_api.c
@@
+static int hex_digit_value(char c)
+{
+ if (c >= '0' && c <= '9') return c - '0';
+ if (c >= 'a' && c <= 'f') return c - 'a' + 10;
+ if (c >= 'A' && c <= 'F') return c - 'A' + 10;
+ return -1;
+}
+
+static bool is_hex_string(const cJSON *item, size_t expected_len)
+{
+ if (!cJSON_IsString(item) || !item->valuestring ||
+ strlen(item->valuestring) != expected_len) return false;
+ for (size_t i = 0; i < expected_len; ++i) {
+ if (hex_digit_value(item->valuestring[i]) < 0) return false;
+ }
+ return true;
+}
+
+static bool is_even_hex_string(const cJSON *item)
+{
+ if (!cJSON_IsString(item) || !item->valuestring) return false;
+ size_t len = strlen(item->valuestring);
+ if (len & 1U) return false;
+ for (size_t i = 0; i < len; ++i) {
+ if (hex_digit_value(item->valuestring[i]) < 0) return false;
+ }
+ return true;
+}
+
+static bool parse_hex_u32(const cJSON *item, uint32_t *value)
+{
+ if (!value || !is_hex_string(item, 8)) return false;
+ uint32_t parsed = 0;
+ for (size_t i = 0; i < 8; ++i) {
+ parsed = (parsed << 4) | (uint32_t)hex_digit_value(item->valuestring[i]);
+ }
+ *value = parsed;
+ return true;
+}
@@
- if (!cJSON_IsString(job_id_item) ||
- !cJSON_IsString(prev_block_hash_item) ||
- !cJSON_IsString(coinbase_1_item) ||
- !cJSON_IsString(coinbase_2_item) ||
+ if (!cJSON_IsString(job_id_item) || !job_id_item->valuestring ||
+ !is_hex_string(prev_block_hash_item, HASH_SIZE * 2) ||
+ !is_even_hex_string(coinbase_1_item) ||
+ !is_even_hex_string(coinbase_2_item) ||
!cJSON_IsArray(merkle_branch) ||
- !cJSON_IsString(version_item) ||
- !cJSON_IsString(target_item) ||
- !cJSON_IsString(ntime_item) ||
!cJSON_IsBool(clean_jobs_item)) {
- ESP_LOGE(TAG, "Invalid field type in mining.notify");
+ ESP_LOGE(TAG, "Invalid field type or encoding in mining.notify");
return false;
}
+
+ uint32_t version, target, ntime;
+ if (!parse_hex_u32(version_item, &version) ||
+ !parse_hex_u32(target_item, &target) ||
+ !parse_hex_u32(ntime_item, &ntime)) return false;
@@
- if (!cJSON_IsString(cJSON_GetArrayItem(merkle_branch, i))) {
+ if (!is_hex_string(cJSON_GetArrayItem(merkle_branch, i), HASH_SIZE * 2)) {
ESP_LOGE(TAG, "Invalid Merkle branch at index %d", i);
return false;
}
@@
- new_work->version = strtoul(version_item->valuestring, NULL, 16);
- new_work->target = strtoul(target_item->valuestring, NULL, 16);
- new_work->ntime = strtoul(ntime_item->valuestring, NULL, 16);
+ new_work->version = version;
+ new_work->target = target;
+ new_work->ntime = ntime;| if (remaining_len > 0) { | ||
| memmove(json_rpc_buffer, newline_pos + 1, remaining_len); | ||
| } | ||
| json_rpc_buffer_used = remaining_len; |
Collaborator
There was a problem hiding this comment.
[P2] The retained tail needs to be cleared at every transport/session boundary. If one read contains client.reconnect\n followed by another complete message, this code preserves the second line; stratum_v1_close_connection() closes the socket but does not reset this buffer, so the next connection processes the old pool's buffered line before reading the new transport. Please expose/call a buffer reset when closing a connection and add a regression that switches mock transports after the first line.
Collaborator
There was a problem hiding this comment.
Proposed patch, including a regression where the first transport contains first\nstale\n, the buffer is reset, and the second transport must return fresh rather than stale:
diff --git a/components/stratum/include/stratum_api.h b/components/stratum/include/stratum_api.h
@@
void STRATUM_V1_cleanup_buffer(void);
+void STRATUM_V1_reset_buffer(void);
diff --git a/components/stratum/stratum_api.c b/components/stratum/stratum_api.c
@@
-static void reset_json_buffer(void)
+void STRATUM_V1_reset_buffer(void)
{
json_rpc_buffer_used = 0;
@@
- reset_json_buffer();
+ STRATUM_V1_reset_buffer();
@@
- reset_json_buffer();
+ STRATUM_V1_reset_buffer();
@@
- reset_json_buffer();
+ STRATUM_V1_reset_buffer();
diff --git a/main/tasks/stratum_v1_task.c b/main/tasks/stratum_v1_task.c
@@
if (transport != NULL) {
esp_transport_close(transport);
}
+ STRATUM_V1_reset_buffer();
SYSTEM_clean_jobs_queue(GLOBAL_STATE);The test should call STRATUM_V1_reset_buffer() between the two mock transports without reinitializing the allocation, proving that session data is discarded while the buffer remains reusable.
| return false; | ||
| } | ||
|
|
||
| int extranonce_2_len = extranonce2_size->valueint; |
Collaborator
There was a problem hiding this comment.
[P2] Please check the later strdup() before reporting a valid message, in both mining.set_extranonce and the subscribe-result path. On allocation failure the parser currently returns true, the task replaces and frees the previous global extranonce with NULL, and job generation later calls strlen(NULL). Allocate into a temporary pointer, return false on failure, and only publish/replace state after the copy succeeds.
Collaborator
There was a problem hiding this comment.
Proposed minimal patch; allocation happens before the old value is released, so failure leaves the message/session state untouched:
diff --git a/components/stratum/stratum_api.c b/components/stratum/stratum_api.c
@@ static bool parse_set_extranonce(cJSON *json, StratumApiV1Message *message)
- if (message->extranonce_str) free(message->extranonce_str);
- message->extranonce_str = strdup(extranonce1->valuestring);
+ char *new_extranonce = strdup(extranonce1->valuestring);
+ if (!new_extranonce) {
+ ESP_LOGE(TAG, "Memory allocation failed for extranonce1");
+ return false;
+ }
+ free(message->extranonce_str);
+ message->extranonce_str = new_extranonce;
@@ static bool parse_subscribe_result(cJSON *json, StratumApiV1Message *message)
- if (message->extranonce_str) free(message->extranonce_str);
- message->extranonce_str = strdup(extranonce->valuestring);
+ char *new_extranonce = strdup(extranonce->valuestring);
+ if (!new_extranonce) {
+ ESP_LOGE(TAG, "Memory allocation failed for subscribe extranonce1");
+ return false;
+ }
+ free(message->extranonce_str);
+ message->extranonce_str = new_extranonce;If the unit harness supports allocator fault injection, add one case for each parser and assert STRATUM_V1_parse() is false with message.extranonce_str == NULL.
Collaborator
|
I converted the six review findings into a single candidate patch against the current PR head, It adds the implementation fixes and targeted regressions for truncated varints, bounded heap-based coinbase assembly, testable receive-buffer initialization, strict Stratum hex validation, session-tail reset, and extranonce allocation failures. Local verification:
GitHub cannot represent a 12-file change as one single-click inline suggestion, so the complete suggested patch is included below. Copy the diff into a file and run Suggested patch (12 files, including regression tests)diff --git a/components/stratum/coinbase_decoder.c b/components/stratum/coinbase_decoder.c
index e62bbc7..d0bedf7 100644
--- a/components/stratum/coinbase_decoder.c
+++ b/components/stratum/coinbase_decoder.c
@@ -24,29 +24,33 @@ static void ensure_base58_init(void) {
}
}
-uint64_t coinbase_decode_varint(const uint8_t *data, int *offset) {
- uint8_t first_byte = data[*offset];
- (*offset)++;
-
+bool coinbase_decode_varint(const uint8_t *data, size_t data_len, size_t *offset, uint64_t *value)
+{
+ if (data == NULL || offset == NULL || value == NULL || *offset >= data_len) {
+ return false;
+ }
+
+ size_t next = *offset;
+ uint8_t first_byte = data[next++];
if (first_byte < 0xFD) {
- return first_byte;
- } else if (first_byte == 0xFD) {
- uint64_t value = data[*offset] | (data[*offset + 1] << 8);
- *offset += 2;
- return value;
- } else if (first_byte == 0xFE) {
- uint64_t value = data[*offset] | (data[*offset + 1] << 8) |
- (data[*offset + 2] << 16) | (data[*offset + 3] << 24);
- *offset += 4;
- return value;
- } else { // 0xFF
- uint64_t value = 0;
- for (int i = 0; i < 8; i++) {
- value |= ((uint64_t)data[*offset + i]) << (i * 8);
- }
- *offset += 8;
- return value;
+ *offset = next;
+ *value = first_byte;
+ return true;
+ }
+
+ size_t payload_len = first_byte == 0xFD ? 2 : first_byte == 0xFE ? 4 : 8;
+ if (payload_len > data_len - next) {
+ return false;
+ }
+
+ uint64_t decoded = 0;
+ for (size_t i = 0; i < payload_len; i++) {
+ decoded |= ((uint64_t)data[next + i]) << (i * 8);
}
+
+ *offset = next + payload_len;
+ *value = decoded;
+ return true;
}
void coinbase_decode_address_from_scriptpubkey(const uint8_t *script, size_t script_len,
@@ -268,7 +272,7 @@ esp_err_t coinbase_process_notification(const mining_notify *notification,
hex2bin(notification->coinbase_2, coinbase_2_bin, coinbase_2_len);
- int offset = coinbase_2_offset;
+ size_t offset = (size_t)coinbase_2_offset;
// Read sequence (4 bytes) for BIP-54 detection
if (offset + 4 > coinbase_2_len) {
@@ -287,13 +291,20 @@ esp_err_t coinbase_process_notification(const mining_notify *notification,
return ESP_ERR_INVALID_ARG;
}
- uint64_t num_outputs = coinbase_decode_varint(coinbase_2_bin, &offset);
+ uint64_t num_outputs;
+ if (!coinbase_decode_varint(coinbase_2_bin, (size_t)coinbase_2_len, &offset, &num_outputs)) {
+ free(coinbase_2_bin);
+ return ESP_ERR_INVALID_ARG;
+ }
result->output_count = 0;
// Parse each output
for (uint64_t i = 0; i < num_outputs && offset < coinbase_2_len; i++) {
// Read value (8 bytes, little-endian)
- if (offset + 8 > coinbase_2_len) break;
+ if ((size_t)coinbase_2_len - offset < 8) {
+ free(coinbase_2_bin);
+ return ESP_ERR_INVALID_ARG;
+ }
uint64_t value_satoshis = 0;
for (int i = 0; i < 8; i++) {
@@ -305,10 +316,12 @@ esp_err_t coinbase_process_notification(const mining_notify *notification,
result->total_value_satoshis += value_satoshis;
// Read scriptPubKey length
- if (offset >= coinbase_2_len) break;
- uint64_t script_len = coinbase_decode_varint(coinbase_2_bin, &offset);
-
- if (offset + script_len > coinbase_2_len) break;
+ uint64_t script_len;
+ if (!coinbase_decode_varint(coinbase_2_bin, (size_t)coinbase_2_len, &offset, &script_len) ||
+ script_len > (uint64_t)((size_t)coinbase_2_len - offset)) {
+ free(coinbase_2_bin);
+ return ESP_ERR_INVALID_ARG;
+ }
if (decode_coinbase_tx) {
if (value_satoshis > 0) {
diff --git a/components/stratum/include/coinbase_decoder.h b/components/stratum/include/coinbase_decoder.h
index b84dfa0..1d76b53 100644
--- a/components/stratum/include/coinbase_decoder.h
+++ b/components/stratum/include/coinbase_decoder.h
@@ -27,10 +27,12 @@ typedef struct mining_notify mining_notify;
* @brief Decode Bitcoin varint from binary data
*
* @param data Binary data containing the varint
- * @param offset Pointer to current offset, will be updated after reading
- * @return Decoded varint value
+ * @param data_len Length of the binary data
+ * @param offset Pointer to current offset, updated after a successful read
+ * @param value Pointer to the decoded varint value
+ * @return true when a complete varint was decoded
*/
-uint64_t coinbase_decode_varint(const uint8_t *data, int *offset);
+bool coinbase_decode_varint(const uint8_t *data, size_t data_len, size_t *offset, uint64_t *value);
/**
* @brief Decode Bitcoin address from scriptPubKey
diff --git a/components/stratum/include/mining.h b/components/stratum/include/mining.h
index 56355b3..6ca25fc 100644
--- a/components/stratum/include/mining.h
+++ b/components/stratum/include/mining.h
@@ -1,8 +1,12 @@
#ifndef MINING_H_
#define MINING_H_
+#include <stdbool.h>
+#include <stddef.h>
#include <stdint.h>
+#define MAX_COINBASE_TX_BYTES 8192
+
typedef struct mining_notify mining_notify;
typedef struct bm_job
@@ -27,7 +31,7 @@ typedef struct bm_job
void free_bm_job(bm_job *job);
-void calculate_coinbase_tx_hash(const char *coinbase_1, const char *coinbase_2,
+bool calculate_coinbase_tx_hash(const char *coinbase_1, const char *coinbase_2,
const char *extranonce, const char *extranonce_2, uint8_t dest[32]);
void calculate_coinbase_tx_hash_bin(const uint8_t *prefix, size_t prefix_len,
@@ -51,4 +55,4 @@ void extranonce_2_generate(uint64_t extranonce_2, uint32_t length, char dest[sta
uint32_t increment_bitmask(const uint32_t value, const uint32_t mask);
-#endif /* MINING_H_ */
\ No newline at end of file
+#endif /* MINING_H_ */
diff --git a/components/stratum/include/stratum_api.h b/components/stratum/include/stratum_api.h
index a364a6e..2632124 100644
--- a/components/stratum/include/stratum_api.h
+++ b/components/stratum/include/stratum_api.h
@@ -82,10 +82,14 @@ typedef struct RequestTiming
esp_transport_handle_t STRATUM_V1_transport_init(tls_mode tls, char * cert);
+bool STRATUM_V1_initialize(void);
+
bool STRATUM_V1_initialize_buffer(void);
void STRATUM_V1_cleanup_buffer(void);
+void STRATUM_V1_reset_buffer(void);
+
char *STRATUM_V1_receive_jsonrpc_line(esp_transport_handle_t transport);
int STRATUM_V1_subscribe(esp_transport_handle_t transport, int send_uid, const char * model);
diff --git a/components/stratum/mining.c b/components/stratum/mining.c
index 8e441df..9aa6b5b 100644
--- a/components/stratum/mining.c
+++ b/components/stratum/mining.c
@@ -1,6 +1,7 @@
#include <string.h>
#include <stdio.h>
#include <limits.h>
+#include <stdlib.h>
#include "mining.h"
#include "stratum_api.h"
#include "utils.h"
@@ -12,16 +13,57 @@ void free_bm_job(bm_job *job)
free(job);
}
-void calculate_coinbase_tx_hash(const char *coinbase_1, const char *coinbase_2, const char *extranonce, const char *extranonce_2, uint8_t dest[32])
+__attribute__((weak)) void *stratum_mining_malloc(size_t size)
{
+ return malloc(size);
+}
+
+static bool is_even_hex(const char *value)
+{
+ if (value == NULL) return false;
+
+ size_t length = strlen(value);
+ if ((length & 1U) != 0) return false;
+
+ for (size_t i = 0; i < length; i++) {
+ char digit = value[i];
+ if (!((digit >= '0' && digit <= '9') ||
+ (digit >= 'a' && digit <= 'f') ||
+ (digit >= 'A' && digit <= 'F'))) {
+ return false;
+ }
+ }
+ return true;
+}
+
+bool calculate_coinbase_tx_hash(const char *coinbase_1, const char *coinbase_2,
+ const char *extranonce, const char *extranonce_2,
+ uint8_t dest[32])
+{
+ if (dest == NULL || !is_even_hex(coinbase_1) || !is_even_hex(coinbase_2) ||
+ !is_even_hex(extranonce) || !is_even_hex(extranonce_2)) {
+ return false;
+ }
+
size_t len1 = strlen(coinbase_1);
size_t len2 = strlen(extranonce);
size_t len3 = strlen(extranonce_2);
size_t len4 = strlen(coinbase_2);
+ if (len1 > SIZE_MAX - len2 || len1 + len2 > SIZE_MAX - len3 ||
+ len1 + len2 + len3 > SIZE_MAX - len4) {
+ return false;
+ }
+
size_t coinbase_tx_bin_len = (len1 + len2 + len3 + len4) / 2;
+ if (coinbase_tx_bin_len == 0 || coinbase_tx_bin_len > MAX_COINBASE_TX_BYTES) {
+ return false;
+ }
- uint8_t coinbase_tx_bin[coinbase_tx_bin_len];
+ uint8_t *coinbase_tx_bin = stratum_mining_malloc(coinbase_tx_bin_len);
+ if (coinbase_tx_bin == NULL) {
+ return false;
+ }
size_t bin_offset = 0;
bin_offset += hex2bin(coinbase_1, coinbase_tx_bin + bin_offset, coinbase_tx_bin_len - bin_offset);
@@ -29,7 +71,12 @@ void calculate_coinbase_tx_hash(const char *coinbase_1, const char *coinbase_2,
bin_offset += hex2bin(extranonce_2, coinbase_tx_bin + bin_offset, coinbase_tx_bin_len - bin_offset);
bin_offset += hex2bin(coinbase_2, coinbase_tx_bin + bin_offset, coinbase_tx_bin_len - bin_offset);
- double_sha256_bin(coinbase_tx_bin, coinbase_tx_bin_len, dest);
+ bool complete = bin_offset == coinbase_tx_bin_len;
+ if (complete) {
+ double_sha256_bin(coinbase_tx_bin, coinbase_tx_bin_len, dest);
+ }
+ free(coinbase_tx_bin);
+ return complete;
}
void calculate_coinbase_tx_hash_bin(const uint8_t *prefix, size_t prefix_len,
diff --git a/components/stratum/stratum_api.c b/components/stratum/stratum_api.c
index 9fe31b7..158727b 100644
--- a/components/stratum/stratum_api.c
+++ b/components/stratum/stratum_api.c
@@ -32,6 +32,11 @@ static size_t json_rpc_buffer_used = 0;
static RequestTiming *request_timings = NULL;
+__attribute__((weak)) char *stratum_api_strdup(const char *source)
+{
+ return strdup(source);
+}
+
static RequestTiming* get_request_timing(int request_id) {
if (request_id < 0 || request_timings == NULL) return NULL;
int index = request_id % MAX_REQUEST_IDS;
@@ -102,7 +107,6 @@ void STRATUM_V1_cleanup_buffer(void)
bool STRATUM_V1_initialize_buffer(void)
{
- // Release state left by a previous V1 task before starting a new session.
STRATUM_V1_cleanup_buffer();
json_rpc_buffer = malloc(BUFFER_SIZE);
@@ -114,6 +118,16 @@ bool STRATUM_V1_initialize_buffer(void)
json_rpc_buffer_used = 0;
json_rpc_buffer[0] = '\0';
+ return true;
+}
+
+bool STRATUM_V1_initialize(void)
+{
+ // Release state left by a previous V1 task before starting a new session.
+ if (!STRATUM_V1_initialize_buffer()) {
+ return false;
+ }
+
if (request_timings == NULL) {
request_timings = heap_caps_malloc(sizeof(RequestTiming) * MAX_REQUEST_IDS, MALLOC_CAP_SPIRAM);
if (request_timings == NULL) {
@@ -131,7 +145,7 @@ bool STRATUM_V1_initialize_buffer(void)
return true;
}
-static void reset_json_buffer(void)
+void STRATUM_V1_reset_buffer(void)
{
json_rpc_buffer_used = 0;
if (json_rpc_buffer != NULL) {
@@ -196,7 +210,7 @@ char *STRATUM_V1_receive_jsonrpc_line(esp_transport_handle_t transport)
break;
}
ESP_LOGE(TAG, "Error: transport read failed: %s (code: %d)", err_str, nbytes);
- reset_json_buffer();
+ STRATUM_V1_reset_buffer();
return NULL;
}
@@ -209,14 +223,14 @@ char *STRATUM_V1_receive_jsonrpc_line(esp_transport_handle_t transport)
if (json_rpc_buffer_used > STRATUM_V1_MAX_JSON_LINE_SIZE ||
bytes_before_newline > STRATUM_V1_MAX_JSON_LINE_SIZE - json_rpc_buffer_used) {
ESP_LOGE(TAG, "JSON-RPC line exceeds %d-byte limit", STRATUM_V1_MAX_JSON_LINE_SIZE);
- reset_json_buffer();
+ STRATUM_V1_reset_buffer();
return NULL;
}
size_t append_offset = json_rpc_buffer_used;
size_t required_size = append_offset + (size_t)nbytes + 1;
if (!ensure_json_buffer_capacity(required_size)) {
- reset_json_buffer();
+ STRATUM_V1_reset_buffer();
return NULL;
}
@@ -233,7 +247,7 @@ char *STRATUM_V1_receive_jsonrpc_line(esp_transport_handle_t transport)
char *line = malloc(line_len + 1);
if (line == NULL) {
ESP_LOGE(TAG, "Failed to allocate %zu-byte JSON-RPC line", line_len + 1);
- reset_json_buffer();
+ STRATUM_V1_reset_buffer();
return NULL;
}
@@ -303,6 +317,62 @@ static stratum_method parse_method(const cJSON *method_json)
return METHOD_UNKNOWN;
}
+static int hex_digit_value(char digit)
+{
+ if (digit >= '0' && digit <= '9') return digit - '0';
+ if (digit >= 'a' && digit <= 'f') return digit - 'a' + 10;
+ if (digit >= 'A' && digit <= 'F') return digit - 'A' + 10;
+ return -1;
+}
+
+static bool is_hex_string(const cJSON *item, size_t expected_length)
+{
+ if (!item || !cJSON_IsString(item) || item->valuestring == NULL ||
+ strlen(item->valuestring) != expected_length) {
+ return false;
+ }
+
+ for (size_t i = 0; i < expected_length; i++) {
+ if (hex_digit_value(item->valuestring[i]) < 0) {
+ return false;
+ }
+ }
+ return true;
+}
+
+static bool is_even_length_hex_string(const cJSON *item)
+{
+ if (!item || !cJSON_IsString(item) || item->valuestring == NULL) {
+ return false;
+ }
+
+ size_t length = strlen(item->valuestring);
+ if ((length & 1U) != 0) {
+ return false;
+ }
+
+ for (size_t i = 0; i < length; i++) {
+ if (hex_digit_value(item->valuestring[i]) < 0) {
+ return false;
+ }
+ }
+ return true;
+}
+
+static bool parse_hex_u32(const cJSON *item, uint32_t *result)
+{
+ if (result == NULL || !is_hex_string(item, 8)) {
+ return false;
+ }
+
+ uint32_t value = 0;
+ for (size_t i = 0; i < 8; i++) {
+ value = (value << 4) | (uint32_t)hex_digit_value(item->valuestring[i]);
+ }
+ *result = value;
+ return true;
+}
+
static bool parse_mining_notify(cJSON *json, StratumApiV1Message *message)
{
cJSON *params = cJSON_GetObjectItem(json, "params");
@@ -327,16 +397,23 @@ static bool parse_mining_notify(cJSON *json, StratumApiV1Message *message)
cJSON *ntime_item = cJSON_GetArrayItem(params, 7);
cJSON *clean_jobs_item = cJSON_GetArrayItem(params, params_count - 1);
- if (!cJSON_IsString(job_id_item) ||
- !cJSON_IsString(prev_block_hash_item) ||
- !cJSON_IsString(coinbase_1_item) ||
- !cJSON_IsString(coinbase_2_item) ||
- !cJSON_IsArray(merkle_branch) ||
- !cJSON_IsString(version_item) ||
- !cJSON_IsString(target_item) ||
- !cJSON_IsString(ntime_item) ||
- !cJSON_IsBool(clean_jobs_item)) {
- ESP_LOGE(TAG, "Invalid field type in mining.notify");
+ if (!job_id_item || !cJSON_IsString(job_id_item) || job_id_item->valuestring == NULL ||
+ !is_hex_string(prev_block_hash_item, HASH_SIZE * 2) ||
+ !is_even_length_hex_string(coinbase_1_item) ||
+ !is_even_length_hex_string(coinbase_2_item) ||
+ !merkle_branch || !cJSON_IsArray(merkle_branch) ||
+ !clean_jobs_item || !cJSON_IsBool(clean_jobs_item)) {
+ ESP_LOGE(TAG, "Invalid field type or encoding in mining.notify");
+ return false;
+ }
+
+ uint32_t version;
+ uint32_t target;
+ uint32_t ntime;
+ if (!parse_hex_u32(version_item, &version) ||
+ !parse_hex_u32(target_item, &target) ||
+ !parse_hex_u32(ntime_item, &ntime)) {
+ ESP_LOGE(TAG, "Invalid version, target, or ntime in mining.notify");
return false;
}
@@ -347,7 +424,7 @@ static bool parse_mining_notify(cJSON *json, StratumApiV1Message *message)
}
for (int i = 0; i < merkle_branch_count; i++) {
- if (!cJSON_IsString(cJSON_GetArrayItem(merkle_branch, i))) {
+ if (!is_hex_string(cJSON_GetArrayItem(merkle_branch, i), HASH_SIZE * 2)) {
ESP_LOGE(TAG, "Invalid Merkle branch at index %d", i);
return false;
}
@@ -381,15 +458,20 @@ static bool parse_mining_notify(cJSON *json, StratumApiV1Message *message)
for (size_t i = 0; i < new_work->n_merkle_branches; i++) {
cJSON *branch = cJSON_GetArrayItem(merkle_branch, i);
- hex2bin(branch->valuestring,
- new_work->merkle_branches + HASH_SIZE * i,
- HASH_SIZE);
+ size_t decoded = hex2bin(branch->valuestring,
+ new_work->merkle_branches + HASH_SIZE * i,
+ HASH_SIZE);
+ if (decoded != HASH_SIZE) {
+ ESP_LOGE(TAG, "Failed to decode Merkle branch at index %zu", i);
+ STRATUM_V1_free_mining_notify(new_work);
+ return false;
+ }
}
}
- new_work->version = strtoul(version_item->valuestring, NULL, 16);
- new_work->target = strtoul(target_item->valuestring, NULL, 16);
- new_work->ntime = strtoul(ntime_item->valuestring, NULL, 16);
+ new_work->version = version;
+ new_work->target = target;
+ new_work->ntime = ntime;
// Some pools append extension fields; clean_jobs remains the final parameter.
new_work->clean_jobs = cJSON_IsTrue(clean_jobs_item);
@@ -453,8 +535,13 @@ static bool parse_set_extranonce(cJSON *json, StratumApiV1Message *message)
return false;
}
- if (message->extranonce_str) free(message->extranonce_str);
- message->extranonce_str = strdup(extranonce1->valuestring);
+ char *new_extranonce = stratum_api_strdup(extranonce1->valuestring);
+ if (new_extranonce == NULL) {
+ ESP_LOGE(TAG, "Memory allocation failed for extranonce1");
+ return false;
+ }
+ free(message->extranonce_str);
+ message->extranonce_str = new_extranonce;
if (extranonce_2_len > MAX_EXTRANONCE_2_LEN) {
ESP_LOGW(TAG, "Extranonce_2_len %d exceeds maximum %d, clamping to maximum",
@@ -517,8 +604,13 @@ static bool parse_subscribe_result(cJSON *json, StratumApiV1Message *message)
return false;
}
- if (message->extranonce_str) free(message->extranonce_str);
- message->extranonce_str = strdup(extranonce->valuestring);
+ char *new_extranonce = stratum_api_strdup(extranonce->valuestring);
+ if (new_extranonce == NULL) {
+ ESP_LOGE(TAG, "Memory allocation failed for subscribe extranonce1");
+ return false;
+ }
+ free(message->extranonce_str);
+ message->extranonce_str = new_extranonce;
if (extranonce_2_len > MAX_EXTRANONCE_2_LEN) {
ESP_LOGW(TAG, "Extranonce_2_len %d exceeds maximum %d, clamping to maximum",
diff --git a/components/stratum/test/test_coinbase_decoder.c b/components/stratum/test/test_coinbase_decoder.c
index 2a028d4..3ecc633 100644
--- a/components/stratum/test/test_coinbase_decoder.c
+++ b/components/stratum/test/test_coinbase_decoder.c
@@ -7,37 +7,88 @@
TEST_CASE("Varint decode single byte", "[coinbase_decoder]")
{
uint8_t data[] = {0x42};
- int offset = 0;
- uint64_t result = coinbase_decode_varint(data, &offset);
+ size_t offset = 0;
+ uint64_t result = 0;
+ TEST_ASSERT_TRUE(coinbase_decode_varint(data, sizeof(data), &offset, &result));
TEST_ASSERT_TRUE(0x42 == result);
- TEST_ASSERT_EQUAL_INT(1, offset);
+ TEST_ASSERT_EQUAL_UINT32(1, offset);
}
TEST_CASE("Varint decode FD format", "[coinbase_decoder]")
{
uint8_t data[] = {0xFD, 0x34, 0x12}; // 0x1234 in little-endian
- int offset = 0;
- uint64_t result = coinbase_decode_varint(data, &offset);
+ size_t offset = 0;
+ uint64_t result = 0;
+ TEST_ASSERT_TRUE(coinbase_decode_varint(data, sizeof(data), &offset, &result));
TEST_ASSERT_TRUE(0x1234 == result);
- TEST_ASSERT_EQUAL_INT(3, offset);
+ TEST_ASSERT_EQUAL_UINT32(3, offset);
}
TEST_CASE("Varint decode FE format", "[coinbase_decoder]")
{
uint8_t data[] = {0xFE, 0x78, 0x56, 0x34, 0x12}; // 0x12345678 in little-endian
- int offset = 0;
- uint64_t result = coinbase_decode_varint(data, &offset);
+ size_t offset = 0;
+ uint64_t result = 0;
+ TEST_ASSERT_TRUE(coinbase_decode_varint(data, sizeof(data), &offset, &result));
TEST_ASSERT_TRUE(0x12345678 == result);
- TEST_ASSERT_EQUAL_INT(5, offset);
+ TEST_ASSERT_EQUAL_UINT32(5, offset);
}
TEST_CASE("Varint decode FF format", "[coinbase_decoder]")
{
uint8_t data[] = {0xFF, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
- int offset = 0;
- uint64_t result = coinbase_decode_varint(data, &offset);
+ size_t offset = 0;
+ uint64_t result = 0;
+ TEST_ASSERT_TRUE(coinbase_decode_varint(data, sizeof(data), &offset, &result));
TEST_ASSERT_TRUE(0x0807060504030201ULL == result);
- TEST_ASSERT_EQUAL_INT(9, offset);
+ TEST_ASSERT_EQUAL_UINT32(9, offset);
+}
+
+TEST_CASE("Varint decoder rejects every truncated extended encoding", "[coinbase_decoder]")
+{
+ static const struct {
+ uint8_t marker;
+ size_t complete_size;
+ } cases[] = {
+ {0xFD, 3},
+ {0xFE, 5},
+ {0xFF, 9},
+ };
+
+ for (size_t case_index = 0; case_index < sizeof(cases) / sizeof(cases[0]); case_index++) {
+ uint8_t data[9] = {cases[case_index].marker};
+ for (size_t truncated_size = 1; truncated_size < cases[case_index].complete_size; truncated_size++) {
+ size_t offset = 0;
+ uint64_t result = UINT64_MAX;
+ TEST_ASSERT_FALSE(coinbase_decode_varint(data, truncated_size, &offset, &result));
+ TEST_ASSERT_EQUAL_UINT32(0, offset);
+ TEST_ASSERT_TRUE(UINT64_MAX == result);
+ }
+ }
+}
+
+TEST_CASE("Coinbase notification rejects truncated output varints", "[coinbase_decoder]")
+{
+ static const char minimal_coinbase_1[] =
+ "00000000" "01"
+ "0000000000000000000000000000000000000000000000000000000000000000"
+ "ffffffff" "020100";
+ static const char *truncated_coinbase_2[] = {
+ "fffffffffd",
+ "ffffffff010000000000000000fd00",
+ };
+
+ for (size_t i = 0; i < sizeof(truncated_coinbase_2) / sizeof(truncated_coinbase_2[0]); i++) {
+ mining_notify notification = {
+ .coinbase_1 = (char *)minimal_coinbase_1,
+ .coinbase_2 = (char *)truncated_coinbase_2[i],
+ .target = 0x1d00ffff,
+ };
+ mining_notification_result_t result = {};
+
+ TEST_ASSERT_EQUAL(ESP_ERR_INVALID_ARG,
+ coinbase_process_notification(¬ification, "", 0, NULL, false, &result));
+ }
}
TEST_CASE("Decode P2PKH address", "[coinbase_decoder]")
@@ -214,7 +265,7 @@ TEST_CASE("BIP-110 signaling not detected", "[coinbase_decoder]")
mining_notify notify = { 0 };
notify.version = 0x20000000; // No BIP-110 signaling
notify.job_id = "test_job";
- notify.coinbase_1 = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff4b03a5020cfabe6d6d379ae882651f6469f2ed6b8b40a4f9a4b41fd838a3ad6de8cba775f4e8f1d3080100000000000000";
+ notify.coinbase_1 = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff4803a5020cfabe6d6d379ae882651f6469f2ed6b8b40a4f9a4b41fd838a3ad6de8cba775f4e8f1d3080100000000000000";
notify.coinbase_2 = "41903d4c1b2f736c7573682f0000000003ca890d27000000001976a9147c154ed1dc59609e3d26abb2df2ea3d587cd8c4188ac00000000000000002c6a4c2952534b424c4f434b3a4cb4cb2ddfc37c41baf5ef6b6b4899e3253a8f1dfc7e5dd68a5b5b27005014ef0000000000000000266a24aa21a9ed5caa249f1af9fbf71c986fea8e076ca34ae3514fb2f86400561b28c7b15949bf00000000";
mining_notification_result_t result = { 0 };
@@ -231,7 +282,7 @@ TEST_CASE("BIP-110 signaling detected", "[coinbase_decoder]")
mining_notify notify = { 0 };
notify.version = 0x20000010; // Version with BIP-110 signaling
notify.job_id = "test_job";
- notify.coinbase_1 = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff4b03a5020cfabe6d6d379ae882651f6469f2ed6b8b40a4f9a4b41fd838a3ad6de8cba775f4e8f1d3080100000000000000";
+ notify.coinbase_1 = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff4803a5020cfabe6d6d379ae882651f6469f2ed6b8b40a4f9a4b41fd838a3ad6de8cba775f4e8f1d3080100000000000000";
notify.coinbase_2 = "41903d4c1b2f736c7573682f0000000003ca890d27000000001976a9147c154ed1dc59609e3d26abb2df2ea3d587cd8c4188ac00000000000000002c6a4c2952534b424c4f434b3a4cb4cb2ddfc37c41baf5ef6b6b4899e3253a8f1dfc7e5dd68a5b5b27005014ef0000000000000000266a24aa21a9ed5caa249f1af9fbf71c986fea8e076ca34ae3514fb2f86400561b28c7b15949bf00000000";
mining_notification_result_t result = { 0 };
@@ -248,7 +299,7 @@ TEST_CASE("BIP-110 signaling last block", "[coinbase_decoder]")
mining_notify notify = { 0 };
notify.version = 0x20000010; // Version with BIP-110 signaling
notify.job_id = "test_job";
- notify.coinbase_1 = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff4b031fbc0efabe6d6d379ae882651f6469f2ed6b8b40a4f9a4b41fd838a3ad6de8cba775f4e8f1d3080100000000000000";
+ notify.coinbase_1 = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff48031fbc0efabe6d6d379ae882651f6469f2ed6b8b40a4f9a4b41fd838a3ad6de8cba775f4e8f1d3080100000000000000";
notify.coinbase_2 = "41903d4c1b2f736c7573682f0000000003ca890d27000000001976a9147c154ed1dc59609e3d26abb2df2ea3d587cd8c4188ac00000000000000002c6a4c2952534b424c4f434b3a4cb4cb2ddfc37c41baf5ef6b6b4899e3253a8f1dfc7e5dd68a5b5b27005014ef0000000000000000266a24aa21a9ed5caa249f1af9fbf71c986fea8e076ca34ae3514fb2f86400561b28c7b15949bf00000000";
mining_notification_result_t result = { 0 };
@@ -266,7 +317,7 @@ TEST_CASE("BIP-110 signaling expired", "[coinbase_decoder]")
mining_notify notify = { 0 };
notify.version = 0x20000010; // Version with BIP-110 signaling
notify.job_id = "test_job";
- notify.coinbase_1 = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff4b0320bc0efabe6d6d379ae882651f6469f2ed6b8b40a4f9a4b41fd838a3ad6de8cba775f4e8f1d3080100000000000000";
+ notify.coinbase_1 = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff480320bc0efabe6d6d379ae882651f6469f2ed6b8b40a4f9a4b41fd838a3ad6de8cba775f4e8f1d3080100000000000000";
notify.coinbase_2 = "41903d4c1b2f736c7573682f0000000003ca890d27000000001976a9147c154ed1dc59609e3d26abb2df2ea3d587cd8c4188ac00000000000000002c6a4c2952534b424c4f434b3a4cb4cb2ddfc37c41baf5ef6b6b4899e3253a8f1dfc7e5dd68a5b5b27005014ef0000000000000000266a24aa21a9ed5caa249f1af9fbf71c986fea8e076ca34ae3514fb2f86400561b28c7b15949bf00000000";
mining_notification_result_t result = { 0 };
@@ -276,4 +327,4 @@ TEST_CASE("BIP-110 signaling expired", "[coinbase_decoder]")
TEST_ASSERT_EQUAL(ESP_OK, err);
TEST_ASSERT_EQUAL(965664, result.block_height);
TEST_ASSERT_FALSE(result.bip110_signaling);
-}
\ No newline at end of file
+}
diff --git a/components/stratum/test/test_mining.c b/components/stratum/test/test_mining.c
index 06cb91d..850cb09 100644
--- a/components/stratum/test/test_mining.c
+++ b/components/stratum/test/test_mining.c
@@ -4,8 +4,20 @@
#include "utils.h"
#include <limits.h>
+#include <stdlib.h>
#include <string.h>
+static bool fail_next_mining_allocation;
+
+void *stratum_mining_malloc(size_t size)
+{
+ if (fail_next_mining_allocation) {
+ fail_next_mining_allocation = false;
+ return NULL;
+ }
+ return malloc(size);
+}
+
TEST_CASE("Check coinbase tx construction", "[mining]")
{
const char *coinbase_1 = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff20020862062f503253482f04b8864e5008";
@@ -13,7 +25,7 @@ TEST_CASE("Check coinbase tx construction", "[mining]")
const char *extranonce = "e9695791";
const char *extranonce_2 = "99999999";
uint8_t coinbase_tx_hash[32];
- calculate_coinbase_tx_hash(coinbase_1, coinbase_2, extranonce, extranonce_2, coinbase_tx_hash);
+ TEST_ASSERT_TRUE(calculate_coinbase_tx_hash(coinbase_1, coinbase_2, extranonce, extranonce_2, coinbase_tx_hash));
char expected_coinbase_tx[] = "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff20020862062f503253482f04b8864e5008e969579199999999072f736c7573682f000000000100f2052a010000001976a914d23fcdf86f7e756a64a7a9688ef9903327048ed988ac00000000";
size_t expected_coinbase_tx_len = strlen(expected_coinbase_tx) / 2;
@@ -26,6 +38,68 @@ TEST_CASE("Check coinbase tx construction", "[mining]")
TEST_ASSERT_EQUAL_UINT8_ARRAY(expected_coinbase_tx_hash, coinbase_tx_hash, 32);
}
+TEST_CASE("Coinbase hash rejects malformed hex", "[mining]")
+{
+ static const struct {
+ const char *coinbase_1;
+ const char *coinbase_2;
+ const char *extranonce;
+ const char *extranonce_2;
+ } cases[] = {
+ {"0", "00", "00", "00"},
+ {"00", "zz", "00", "00"},
+ {"00", "00", "0g", "00"},
+ {"00", "00", "00", "000"},
+ };
+
+ for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) {
+ uint8_t hash[32];
+ memset(hash, 0xA5, sizeof(hash));
+ TEST_ASSERT_FALSE(calculate_coinbase_tx_hash(cases[i].coinbase_1, cases[i].coinbase_2,
+ cases[i].extranonce, cases[i].extranonce_2,
+ hash));
+ for (size_t byte = 0; byte < sizeof(hash); byte++) {
+ TEST_ASSERT_EQUAL_HEX8(0xA5, hash[byte]);
+ }
+ }
+}
+
+TEST_CASE("Coinbase hash enforces the assembled transaction limit", "[mining]")
+{
+ size_t max_hex_len = MAX_COINBASE_TX_BYTES * 2;
+ char *coinbase_1 = malloc(max_hex_len + 3);
+ TEST_ASSERT_NOT_NULL(coinbase_1);
+ memset(coinbase_1, '0', max_hex_len + 2);
+ coinbase_1[max_hex_len] = '\0';
+
+ uint8_t hash[32] = {};
+ TEST_ASSERT_TRUE(calculate_coinbase_tx_hash(coinbase_1, "", "", "", hash));
+
+ coinbase_1[max_hex_len] = '0';
+ coinbase_1[max_hex_len + 1] = '0';
+ coinbase_1[max_hex_len + 2] = '\0';
+ memset(hash, 0xA5, sizeof(hash));
+ TEST_ASSERT_FALSE(calculate_coinbase_tx_hash(coinbase_1, "", "", "", hash));
+ for (size_t byte = 0; byte < sizeof(hash); byte++) {
+ TEST_ASSERT_EQUAL_HEX8(0xA5, hash[byte]);
+ }
+
+ free(coinbase_1);
+}
+
+TEST_CASE("Coinbase hash reports allocation failure", "[mining]")
+{
+ uint8_t hash[32];
+ memset(hash, 0xA5, sizeof(hash));
+ fail_next_mining_allocation = true;
+
+ TEST_ASSERT_FALSE(calculate_coinbase_tx_hash("00", "", "", "", hash));
+ TEST_ASSERT_FALSE(fail_next_mining_allocation);
+ for (size_t byte = 0; byte < sizeof(hash); byte++) {
+ TEST_ASSERT_EQUAL_HEX8(0xA5, hash[byte]);
+ }
+}
+
// Values calculated from esp-miner/components/stratum/test/verifiers/merklecalc.py
TEST_CASE("Validate merkle root calculation", "[mining]")
{
@@ -34,7 +108,7 @@ TEST_CASE("Validate merkle root calculation", "[mining]")
const char *extranonce = "00f2052a";
const char *extranonce_2 = "01000000";
uint8_t coinbase_tx_hash[32];
- calculate_coinbase_tx_hash(coinbase_1, coinbase_2, extranonce, extranonce_2, coinbase_tx_hash);
+ TEST_ASSERT_TRUE(calculate_coinbase_tx_hash(coinbase_1, coinbase_2, extranonce, extranonce_2, coinbase_tx_hash));
uint8_t merkles[12][32];
int num_merkles = 12;
@@ -66,7 +140,7 @@ TEST_CASE("Validate another merkle root calculation", "[mining]")
const char *extranonce = "603f352a";
const char *extranonce_2 = "01000000";
uint8_t coinbase_tx_hash[32];
- calculate_coinbase_tx_hash(coinbase_1, coinbase_2, extranonce, extranonce_2, coinbase_tx_hash);
+ TEST_ASSERT_TRUE(calculate_coinbase_tx_hash(coinbase_1, coinbase_2, extranonce, extranonce_2, coinbase_tx_hash));
uint8_t merkles[5][32];
int num_merkles = 5;
@@ -199,12 +273,12 @@ TEST_CASE("Test nonce diff checking 2", "[mining test_nonce][not-on-qemu]")
notify_message.ntime = 0x647025b5;
uint8_t coinbase_tx_hash[32];
- calculate_coinbase_tx_hash(
+ TEST_ASSERT_TRUE(calculate_coinbase_tx_hash(
"01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff4b0389130cfabe6d6d5cbab26a2599e92916edec5657a94a0708ddb970f5c45b5d12905085617eff8e",
"31650707758de07b010000000000001cfd7038212f736c7573682f000000000379ad0c2a000000001976a9147c154ed1dc59609e3d26abb2df2ea3d587cd8c4188ac00000000000000002c6a4c2952534b424c4f434b3ae725d3994b811572c1f345deb98b56b465ef8e153ecbbd27fa37bf1b005161380000000000000000266a24aa21a9ed63b06a7946b190a3fda1d76165b25c9b883bcc6621b040773050ee2a1bb18f1800000000",
"01000000",
"00000000",
- coinbase_tx_hash);
+ coinbase_tx_hash));
uint8_t merkles[13][32];
int num_merkles = 13;
diff --git a/components/stratum/test/test_stratum_json.c b/components/stratum/test/test_stratum_json.c
index b53a0fb..809b56b 100644
--- a/components/stratum/test/test_stratum_json.c
+++ b/components/stratum/test/test_stratum_json.c
@@ -1,6 +1,27 @@
#include "unity.h"
#include "stratum_api.h"
+#include <stdlib.h>
+#include <string.h>
+
+#define TEST_ZERO_HASH "0000000000000000000000000000000000000000000000000000000000000000"
+
+typedef struct {
+ const char *description;
+ const char *json;
+} invalid_notify_case_t;
+
+static bool fail_next_api_strdup;
+
+char *stratum_api_strdup(const char *source)
+{
+ if (fail_next_api_strdup) {
+ fail_next_api_strdup = false;
+ return NULL;
+ }
+ return strdup(source);
+}
+
TEST_CASE("Parse stratum method", "[stratum]")
{
StratumApiV1Message stratum_api_v1_message = {};
@@ -124,6 +145,18 @@ TEST_CASE("Reject negative mining.subscribe extranonce2 length", "[mining.subscr
TEST_ASSERT_NULL(stratum_api_v1_message.extranonce_str);
}
+TEST_CASE("Reject mining.subscribe when extranonce allocation fails", "[mining.subscribe]")
+{
+ StratumApiV1Message message = {};
+ const char *json = "{\"result\":[[[\"mining.notify\",\"695482c0\"]],\"4de05269\",8],\"id\":2,\"error\":null}";
+ fail_next_api_strdup = true;
+
+ TEST_ASSERT_FALSE(STRATUM_V1_parse(&message, json));
+ TEST_ASSERT_FALSE(fail_next_api_strdup);
+ TEST_ASSERT_NULL(message.extranonce_str);
+ TEST_ASSERT_FALSE(message.response_success);
+}
+
TEST_CASE("Parse stratum mining.set_version_mask params", "[stratum]")
{
StratumApiV1Message stratum_api_v1_message = {};
@@ -258,28 +291,142 @@ TEST_CASE("Parse stratum invalid json or malformed parameters", "[stratum]")
TEST_CASE("Reject malformed mining.notify fields", "[mining.notify]")
{
- const char *invalid_notifications[] = {
- // clean_jobs is missing
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",\"prev\",\"cb1\",\"cb2\",[],\"version\",\"nbits\",\"ntime\"]}",
- // Each fixed string field must actually be a string
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[null,\"prev\",\"cb1\",\"cb2\",[],\"version\",\"nbits\",\"ntime\",false]}",
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",null,\"cb1\",\"cb2\",[],\"version\",\"nbits\",\"ntime\",false]}",
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",\"prev\",1,\"cb2\",[],\"version\",\"nbits\",\"ntime\",false]}",
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",\"prev\",\"cb1\",null,[],\"version\",\"nbits\",\"ntime\",false]}",
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",\"prev\",\"cb1\",\"cb2\",[],1,\"nbits\",\"ntime\",false]}",
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",\"prev\",\"cb1\",\"cb2\",[],\"version\",null,\"ntime\",false]}",
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",\"prev\",\"cb1\",\"cb2\",[],\"version\",\"nbits\",false,false]}",
- // Merkle path must be an array containing only strings
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",\"prev\",\"cb1\",\"cb2\",null,\"version\",\"nbits\",\"ntime\",false]}",
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",\"prev\",\"cb1\",\"cb2\",[null],\"version\",\"nbits\",\"ntime\",false]}",
- // clean_jobs must be a boolean
- "{\"id\":null,\"method\":\"mining.notify\",\"params\":[\"job\",\"prev\",\"cb1\",\"cb2\",[],\"version\",\"nbits\",\"ntime\",\"false\"]}",
+ static const invalid_notify_case_t cases[] = {
+ {
+ "missing clean_jobs",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[],"
+ "\"20000000\",\"1d00ffff\",\"65000000\"]}",
+ },
+ {
+ "null previous block hash",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",null,\"00\",\"00\",[],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "numeric coinbase prefix",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",0,\"00\",[],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "null coinbase suffix",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",null,[],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "non-array Merkle branches",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",null,"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "non-string Merkle branch",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[null],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "numeric version",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[],"
+ "1,\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "null target",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[],"
+ "\"20000000\",null,\"65000000\",true]}",
+ },
+ {
+ "boolean ntime",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[],"
+ "\"20000000\",\"1d00ffff\",false,true]}",
+ },
+ {
+ "string clean_jobs",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",\"true\"]}",
+ },
};
- for (size_t i = 0; i < sizeof(invalid_notifications) / sizeof(invalid_notifications[0]); i++) {
+ for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) {
StratumApiV1Message message = {};
- TEST_ASSERT_FALSE(STRATUM_V1_parse(&message, invalid_notifications[i]));
- TEST_ASSERT_NULL(message.mining_notification);
+ TEST_ASSERT_FALSE_MESSAGE(STRATUM_V1_parse(&message, cases[i].json), cases[i].description);
+ TEST_ASSERT_NULL_MESSAGE(message.mining_notification, cases[i].description);
+ STRATUM_V1_reset_message(&message);
+ }
+}
+
+TEST_CASE("Reject mining.notify fields with malformed hex", "[mining.notify]")
+{
+ static const invalid_notify_case_t cases[] = {
+ {
+ "short previous block hash",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"00\",\"00\",\"00\",[],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "non-hex previous block hash",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"g000000000000000000000000000000000000000000000000000000000000000\","
+ "\"00\",\"00\",[],\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "odd-length coinbase prefix",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"0\",\"00\",[],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "non-hex coinbase suffix",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"zz\",[],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "short Merkle branch",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[\"00\"],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "non-hex Merkle branch",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\","
+ "[\"g111111111111111111111111111111111111111111111111111111111111111\"],"
+ "\"20000000\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "short version",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[],"
+ "\"20\",\"1d00ffff\",\"65000000\",true]}",
+ },
+ {
+ "non-hex target",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[],"
+ "\"20000000\",\"1d00fffz\",\"65000000\",true]}",
+ },
+ {
+ "long ntime",
+ "{\"id\":null,\"method\":\"mining.notify\",\"params\":["
+ "\"job\",\"" TEST_ZERO_HASH "\",\"00\",\"00\",[],"
+ "\"20000000\",\"1d00ffff\",\"650000000\",true]}",
+ },
+ };
+
+ for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) {
+ StratumApiV1Message message = {};
+ TEST_ASSERT_FALSE_MESSAGE(STRATUM_V1_parse(&message, cases[i].json), cases[i].description);
+ TEST_ASSERT_NULL_MESSAGE(message.mining_notification, cases[i].description);
+ STRATUM_V1_reset_message(&message);
}
}
@@ -309,6 +456,17 @@ TEST_CASE("Reject negative mining.set_extranonce length", "[stratum]")
TEST_ASSERT_NULL(stratum_api_v1_message.extranonce_str);
}
+TEST_CASE("Reject mining.set_extranonce when allocation fails", "[mining.set_extranonce]")
+{
+ StratumApiV1Message message = {};
+ const char *json = "{\"id\":1,\"method\":\"mining.set_extranonce\",\"params\":[\"deadbeef\",8]}";
+ fail_next_api_strdup = true;
+
+ TEST_ASSERT_FALSE(STRATUM_V1_parse(&message, json));
+ TEST_ASSERT_FALSE(fail_next_api_strdup);
+ TEST_ASSERT_NULL(message.extranonce_str);
+}
+
TEST_CASE("Parse stratum client.show_message", "[stratum]")
{
StratumApiV1Message stratum_api_v1_message = {};
diff --git a/components/stratum/test/test_stratum_receive.c b/components/stratum/test/test_stratum_receive.c
index a726a9b..272a09e 100644
--- a/components/stratum/test/test_stratum_receive.c
+++ b/components/stratum/test/test_stratum_receive.c
@@ -78,6 +78,40 @@ TEST_CASE("SV1 receive buffer handles fragmented and batched lines", "[stratum][
esp_transport_destroy(transport);
}
+TEST_CASE("SV1 receive buffer discards a previous session tail", "[stratum][receive]")
+{
+ const char *first_input = "first\nstale\n";
+ mock_transport_data_t first_mock = {
+ .data = first_input,
+ .length = strlen(first_input),
+ };
+ esp_transport_handle_t first_transport = create_mock_transport(&first_mock);
+ TEST_ASSERT_NOT_NULL(first_transport);
+ TEST_ASSERT_TRUE(STRATUM_V1_initialize_buffer());
+
+ char *line = STRATUM_V1_receive_jsonrpc_line(first_transport);
+ TEST_ASSERT_EQUAL_STRING("first", line);
+ free(line);
+
+ STRATUM_V1_reset_buffer();
+
+ const char *second_input = "fresh\n";
+ mock_transport_data_t second_mock = {
+ .data = second_input,
+ .length = strlen(second_input),
+ };
+ esp_transport_handle_t second_transport = create_mock_transport(&second_mock);
+ TEST_ASSERT_NOT_NULL(second_transport);
+
+ line = STRATUM_V1_receive_jsonrpc_line(second_transport);
+ TEST_ASSERT_EQUAL_STRING("fresh", line);
+ free(line);
+
+ STRATUM_V1_cleanup_buffer();
+ esp_transport_destroy(first_transport);
+ esp_transport_destroy(second_transport);
+}
+
TEST_CASE("SV1 receive buffer accepts the maximum line size", "[stratum][receive]")
{
char * input = malloc(STRATUM_V1_MAX_JSON_LINE_SIZE + 2);
diff --git a/main/tasks/create_jobs_task.c b/main/tasks/create_jobs_task.c
index 99b4356..f935cc6 100644
--- a/main/tasks/create_jobs_task.c
+++ b/main/tasks/create_jobs_task.c
@@ -199,7 +199,12 @@ static void generate_work(GlobalState *GLOBAL_STATE, mining_notify *notification
extranonce_2_generate(extranonce_2, GLOBAL_STATE->extranonce_2_len, extranonce_2_str);
uint8_t coinbase_tx_hash[32];
- calculate_coinbase_tx_hash(notification->coinbase_1, notification->coinbase_2, GLOBAL_STATE->extranonce_str, extranonce_2_str, coinbase_tx_hash);
+ if (!calculate_coinbase_tx_hash(notification->coinbase_1, notification->coinbase_2,
+ GLOBAL_STATE->extranonce_str, extranonce_2_str,
+ coinbase_tx_hash)) {
+ ESP_LOGE(TAG, "Invalid or oversized coinbase transaction, skipping job");
+ return;
+ }
uint8_t merkle_root[32];
calculate_merkle_root_hash(coinbase_tx_hash, (uint8_t(*)[32])notification->merkle_branches, notification->n_merkle_branches, merkle_root);
diff --git a/main/tasks/stratum_v1_task.c b/main/tasks/stratum_v1_task.c
index fa31e69..0189820 100644
--- a/main/tasks/stratum_v1_task.c
+++ b/main/tasks/stratum_v1_task.c
@@ -74,6 +74,7 @@ void stratum_v1_close_connection(GlobalState *GLOBAL_STATE)
if (transport != NULL) {
esp_transport_close(transport);
}
+ STRATUM_V1_reset_buffer();
SYSTEM_clean_jobs_queue(GLOBAL_STATE);
vTaskDelay(1000 / portTICK_PERIOD_MS);
}
@@ -179,7 +180,7 @@ void stratum_v1_task(void *pvParameters)
// Set V1-specific free function for the work queue
GLOBAL_STATE->stratum_queue.free_fn = (void (*)(void *))STRATUM_V1_free_mining_notify;
- if (!STRATUM_V1_initialize_buffer()) {
+ if (!STRATUM_V1_initialize()) {
ESP_LOGE(TAG, "Failed to initialize Stratum V1 receive state, notifying coordinator");
protocol_coordinator_notify_failure();
vTaskDelete(NULL); |
johnny9
reviewed
Aug 7, 2026
johnny9
left a comment
johnny9
left a comment
Collaborator
There was a problem hiding this comment.
Follow-up review of exact head a9b2db0. The six findings from the earlier revision are fixed. I found three remaining input-validation defects and one boundary-coverage gap. I reproduced the affected cases, prepared independently applicable patches, and validated the combined result under ESP32-S3 QEMU: 94 tests, 0 failures.
| for (uint64_t i = 0; i < num_outputs; i++) { | ||
| // Read value (8 bytes, little-endian) | ||
| if (offset + 8 > coinbase_2_len) break; | ||
| if ((size_t)coinbase_2_len - offset < 8) { |
Collaborator
There was a problem hiding this comment.
High: fixed-size coinbase fields are still read without first proving that the decoded bytes exist. The new output/varint checks fail closed, but a truncated coinbase_1 can make the ScriptSig/block-height length reads reach past the decoded buffer, and a coinbase_2 with no final locktime still returns ESP_OK. I reproduced both cases. The proposal in the reply adds explicit bounds, requires the four-byte locktime, and adds regressions.
Collaborator
There was a problem hiding this comment.
Proposed patch. It applies to the exact reviewed head and was validated as part of the combined 94/94 ESP32-S3 QEMU run:
diff --git a/components/stratum/coinbase_decoder.c b/components/stratum/coinbase_decoder.c
index 73a5cc9..866de31 100644
--- a/components/stratum/coinbase_decoder.c
+++ b/components/stratum/coinbase_decoder.c
@@ -198,10 +198,17 @@ esp_err_t coinbase_process_notification(const mining_notify *notification,
hex2bin(notification->coinbase_1 + (coinbase_1_offset * 2), &block_height_len, 1);
coinbase_1_offset++;
- if (coinbase_1_len < coinbase_1_offset || block_height_len == 0 || block_height_len > 4) return ESP_ERR_INVALID_ARG;
+ if (coinbase_1_offset > coinbase_1_len || block_height_len == 0 || block_height_len > 4 ||
+ block_height_len > coinbase_1_len - coinbase_1_offset ||
+ scriptsig_len < 1 + block_height_len) {
+ return ESP_ERR_INVALID_ARG;
+ }
result->block_height = 0;
- hex2bin(notification->coinbase_1 + (coinbase_1_offset * 2), (uint8_t *)&result->block_height, block_height_len);
+ if (hex2bin(notification->coinbase_1 + (coinbase_1_offset * 2),
+ (uint8_t *)&result->block_height, block_height_len) != block_height_len) {
+ return ESP_ERR_INVALID_ARG;
+ }
coinbase_1_offset += block_height_len;
// Detect BIP-110 signaling: check if bit 4 (0x00000010) is set in version
@@ -350,12 +357,17 @@ esp_err_t coinbase_process_notification(const mining_notify *notification,
offset += script_len;
}
- // Read nLockTime (4 bytes at the end of the transaction) for BIP-54 detection
+ // A serialized transaction always ends in a four-byte nLockTime.
+ if ((size_t)coinbase_2_len - offset < 4) {
+ free(coinbase_2_bin);
+ free(result->scriptsig);
+ result->scriptsig = NULL;
+ return ESP_ERR_INVALID_ARG;
+ }
+
uint32_t nLockTime = 0;
- if (offset + 4 <= coinbase_2_len) {
- for (int i = 0; i < 4; i++) {
- nLockTime |= ((uint32_t)coinbase_2_bin[offset + i]) << (i * 8);
- }
+ for (int i = 0; i < 4; i++) {
+ nLockTime |= ((uint32_t)coinbase_2_bin[offset + i]) << (i * 8);
}
// Detect BIP-54 signaling: nLockTime = block_height - 1 AND nSequence != 0xffffffff
diff --git a/components/stratum/test/test_coinbase_decoder.c b/components/stratum/test/test_coinbase_decoder.c
index 5ea85b1..dbd0ef2 100644
--- a/components/stratum/test/test_coinbase_decoder.c
+++ b/components/stratum/test/test_coinbase_decoder.c
@@ -92,6 +92,41 @@ TEST_CASE("Coinbase notification rejects truncated output data and varints", "[c
}
}
+TEST_CASE("Coinbase notification rejects truncated fixed fields", "[coinbase_decoder]")
+{
+ static const struct {
+ const char *description;
+ const char *coinbase_1;
+ const char *coinbase_2;
+ } cases[] = {
+ {
+ "truncated block height",
+ "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff0504",
+ "ffffffff0000000000",
+ },
+ {
+ "missing locktime",
+ "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff0403010203",
+ "ffffffff00",
+ },
+ };
+
+ for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) {
+ mining_notify notify = {
+ .coinbase_1 = (char *)cases[i].coinbase_1,
+ .coinbase_2 = (char *)cases[i].coinbase_2,
+ .target = 0x1d00ffff,
+ };
+ mining_notification_result_t result = {};
+
+ TEST_ASSERT_EQUAL_MESSAGE(
+ ESP_ERR_INVALID_ARG,
+ coinbase_process_notification(¬ify, "", 0, "", true, &result),
+ cases[i].description);
+ free(result.scriptsig);
+ }
+}
+
TEST_CASE("Decode P2PKH address", "[coinbase_decoder]")
{
// P2PKH: OP_DUP OP_HASH160 <20 bytes> OP_EQUALVERIFY OP_CHECKSIG
| mining_notify *new_work = calloc(1, sizeof(mining_notify)); | ||
| if (!new_work) { | ||
| ESP_LOGE(TAG, "Memory allocation failed for mining_notify"); | ||
| cJSON *job_id_item = cJSON_GetArrayItem(params, 0); |
Collaborator
There was a problem hiding this comment.
Medium: the field-level checks validate only the first JSON value because cJSON_Parse accepts a valid document followed by arbitrary non-whitespace data. A line such as a valid result plus trailing is therefore accepted. The proposal reply switches the network parser to require complete input and adds a regression.
Collaborator
There was a problem hiding this comment.
Proposed patch. It applies to the exact reviewed head and was validated as part of the combined 94/94 ESP32-S3 QEMU run:
diff --git a/components/stratum/stratum_api.c b/components/stratum/stratum_api.c
index 158727b..c9e0ef0 100644
--- a/components/stratum/stratum_api.c
+++ b/components/stratum/stratum_api.c
@@ -722,7 +722,7 @@ bool STRATUM_V1_parse(StratumApiV1Message *message, const char *stratum_json)
ESP_LOGI(TAG, "rx: %s", stratum_json); // debug incoming stratum messages
- cJSON *json = cJSON_Parse(stratum_json);
+ cJSON *json = cJSON_ParseWithOpts(stratum_json, NULL, true);
if (!json) {
ESP_LOGE(TAG, "JSON parse failed: %s", stratum_json);
message->method = METHOD_UNKNOWN;
diff --git a/components/stratum/test/test_stratum_json.c b/components/stratum/test/test_stratum_json.c
index 809b56b..36e702f 100644
--- a/components/stratum/test/test_stratum_json.c
+++ b/components/stratum/test/test_stratum_json.c
@@ -289,6 +289,16 @@ TEST_CASE("Parse stratum invalid json or malformed parameters", "[stratum]")
TEST_ASSERT_FALSE(STRATUM_V1_parse(&stratum_api_v1_message2, json_string2));
}
+TEST_CASE("Reject trailing data after a JSON-RPC document", "[stratum][security]")
+{
+ StratumApiV1Message message = {};
+ const char *json = "{\"id\":1,\"result\":true,\"error\":null} trailing";
+
+ TEST_ASSERT_FALSE(STRATUM_V1_parse(&message, json));
+ TEST_ASSERT_FALSE(message.response_success);
+ STRATUM_V1_reset_message(&message);
+}
+
TEST_CASE("Reject malformed mining.notify fields", "[mining.notify]")
{
static const invalid_notify_case_t cases[] = {
|
|
||
| char * line = STRATUM_V1_receive_jsonrpc_line(transport); | ||
| TEST_ASSERT_NOT_NULL(line); | ||
| TEST_ASSERT_EQUAL_UINT32(STRATUM_V1_MAX_JSON_LINE_SIZE, strlen(line)); |
Collaborator
There was a problem hiding this comment.
Coverage: the maximum-line test ends immediately after that line, so it does not exercise the capacity edge where the same transport read also contains the next line. That is the branch which must retain the batched tail. The proposal reply adds the missing max-line-plus-next-line regression.
Collaborator
There was a problem hiding this comment.
Proposed patch. It applies to the exact reviewed head and was validated as part of the combined 94/94 ESP32-S3 QEMU run:
diff --git a/components/stratum/test/test_stratum_receive.c b/components/stratum/test/test_stratum_receive.c
index 272a09e..763cc18 100644
--- a/components/stratum/test/test_stratum_receive.c
+++ b/components/stratum/test/test_stratum_receive.c
@@ -139,6 +139,39 @@ TEST_CASE("SV1 receive buffer accepts the maximum line size", "[stratum][receive
esp_transport_destroy(transport);
}
+TEST_CASE("SV1 receive buffer preserves a line after a maximum-size line", "[stratum][receive]")
+{
+ static const char next_line[] = "next\n";
+ size_t input_len = STRATUM_V1_MAX_JSON_LINE_SIZE + 1 + sizeof(next_line) - 1;
+ char *input = malloc(input_len);
+ TEST_ASSERT_NOT_NULL(input);
+ memset(input, 'a', STRATUM_V1_MAX_JSON_LINE_SIZE);
+ input[STRATUM_V1_MAX_JSON_LINE_SIZE] = '\n';
+ memcpy(input + STRATUM_V1_MAX_JSON_LINE_SIZE + 1,
+ next_line, sizeof(next_line) - 1);
+
+ mock_transport_data_t mock = {
+ .data = input,
+ .length = input_len,
+ };
+ esp_transport_handle_t transport = create_mock_transport(&mock);
+ TEST_ASSERT_NOT_NULL(transport);
+ TEST_ASSERT_TRUE(STRATUM_V1_initialize_buffer());
+
+ char *line = STRATUM_V1_receive_jsonrpc_line(transport);
+ TEST_ASSERT_NOT_NULL(line);
+ TEST_ASSERT_EQUAL_UINT32(STRATUM_V1_MAX_JSON_LINE_SIZE, strlen(line));
+ free(line);
+
+ line = STRATUM_V1_receive_jsonrpc_line(transport);
+ TEST_ASSERT_EQUAL_STRING("next", line);
+ free(line);
+
+ STRATUM_V1_cleanup_buffer();
+ esp_transport_destroy(transport);
+ free(input);
+}
+
TEST_CASE("SV1 receive buffer rejects oversized lines and recovers", "[stratum][receive]")
{
char * oversized = malloc(STRATUM_V1_MAX_JSON_LINE_SIZE + 3);
|
|
||
|
|
||
| int extranonce_2_len = extranonce2_size->valueint; | ||
| if (extranonce_2_len < 0) { |
Collaborator
There was a problem hiding this comment.
High: cJSON valueint silently converts numeric JSON before these checks. Fractional sizes are truncated and oversized sizes are clamped, so values such as 1.5 or 33 are accepted with different semantics instead of rejected. Downstream extranonce buffer sizing relies on this being an exact bounded integer. The proposal reply enforces a finite integer in 0..32 for both set_extranonce and subscribe results, with regressions.
Collaborator
There was a problem hiding this comment.
Proposed patch. It applies to the exact reviewed head and was validated as part of the combined 94/94 ESP32-S3 QEMU run:
diff --git a/components/stratum/stratum_api.c b/components/stratum/stratum_api.c
index 158727b..da9c60c 100644
--- a/components/stratum/stratum_api.c
+++ b/components/stratum/stratum_api.c
@@ -16,6 +16,7 @@
#include "esp_timer.h"
#include "esp_heap_caps.h"
#include <inttypes.h>
+#include <math.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
@@ -515,6 +516,23 @@ static bool parse_set_version_mask(cJSON *json, StratumApiV1Message *message)
return true;
}
+static bool parse_extranonce2_size(const cJSON *item, int *value)
+{
+ if (item == NULL || value == NULL || !cJSON_IsNumber(item) ||
+ !isfinite(item->valuedouble) || item->valuedouble < 0 ||
+ item->valuedouble > MAX_EXTRANONCE_2_LEN) {
+ return false;
+ }
+
+ int parsed = (int)item->valuedouble;
+ if ((double)parsed != item->valuedouble) {
+ return false;
+ }
+
+ *value = parsed;
+ return true;
+}
+
static bool parse_set_extranonce(cJSON *json, StratumApiV1Message *message)
{
cJSON *params = cJSON_GetObjectItem(json, "params");
@@ -524,17 +542,13 @@ static bool parse_set_extranonce(cJSON *json, StratumApiV1Message *message)
}
cJSON *extranonce1 = cJSON_GetArrayItem(params, 0);
cJSON *extranonce2_size = cJSON_GetArrayItem(params, 1);
- if (!extranonce1 || !extranonce2_size || !cJSON_IsString(extranonce1) || !cJSON_IsNumber(extranonce2_size)) {
+ int extranonce_2_len;
+ if (!extranonce1 || !cJSON_IsString(extranonce1) ||
+ !parse_extranonce2_size(extranonce2_size, &extranonce_2_len)) {
ESP_LOGE(TAG, "Invalid extranonce data in set_extranonce");
return false;
}
- int extranonce_2_len = extranonce2_size->valueint;
- if (extranonce_2_len < 0) {
- ESP_LOGE(TAG, "Invalid negative extranonce_2_len: %d", extranonce_2_len);
- return false;
- }
-
char *new_extranonce = stratum_api_strdup(extranonce1->valuestring);
if (new_extranonce == NULL) {
ESP_LOGE(TAG, "Memory allocation failed for extranonce1");
@@ -543,11 +557,6 @@ static bool parse_set_extranonce(cJSON *json, StratumApiV1Message *message)
free(message->extranonce_str);
message->extranonce_str = new_extranonce;
- if (extranonce_2_len > MAX_EXTRANONCE_2_LEN) {
- ESP_LOGW(TAG, "Extranonce_2_len %d exceeds maximum %d, clamping to maximum",
- extranonce_2_len, MAX_EXTRANONCE_2_LEN);
- extranonce_2_len = MAX_EXTRANONCE_2_LEN;
- }
message->extranonce_2_len = extranonce_2_len;
ESP_LOGI(TAG, "Set extranonce: %s, size: %d", message->extranonce_str, message->extranonce_2_len);
return true;
@@ -593,17 +602,13 @@ static bool parse_subscribe_result(cJSON *json, StratumApiV1Message *message)
cJSON *result = cJSON_GetObjectItem(json, "result");
cJSON *extranonce = cJSON_GetArrayItem(result, 1);
cJSON *extranonce2_len = cJSON_GetArrayItem(result, 2);
- if (!extranonce || !extranonce2_len || !cJSON_IsString(extranonce) || !cJSON_IsNumber(extranonce2_len)) {
+ int extranonce_2_len;
+ if (!extranonce || !cJSON_IsString(extranonce) ||
+ !parse_extranonce2_size(extranonce2_len, &extranonce_2_len)) {
ESP_LOGE(TAG, "Invalid extranonce data in subscribe result");
return false;
}
- int extranonce_2_len = extranonce2_len->valueint;
- if (extranonce_2_len < 0) {
- ESP_LOGE(TAG, "Invalid negative extranonce_2_len: %d", extranonce_2_len);
- return false;
- }
-
char *new_extranonce = stratum_api_strdup(extranonce->valuestring);
if (new_extranonce == NULL) {
ESP_LOGE(TAG, "Memory allocation failed for subscribe extranonce1");
@@ -612,11 +617,6 @@ static bool parse_subscribe_result(cJSON *json, StratumApiV1Message *message)
free(message->extranonce_str);
message->extranonce_str = new_extranonce;
- if (extranonce_2_len > MAX_EXTRANONCE_2_LEN) {
- ESP_LOGW(TAG, "Extranonce_2_len %d exceeds maximum %d, clamping to maximum",
- extranonce_2_len, MAX_EXTRANONCE_2_LEN);
- extranonce_2_len = MAX_EXTRANONCE_2_LEN;
- }
message->extranonce_2_len = extranonce_2_len;
message->response_success = true;
ESP_LOGI(TAG, "Subscribe result: extranonce=%s, extranonce2_len=%d",
diff --git a/components/stratum/test/test_stratum_json.c b/components/stratum/test/test_stratum_json.c
index 809b56b..de59c49 100644
--- a/components/stratum/test/test_stratum_json.c
+++ b/components/stratum/test/test_stratum_json.c
@@ -145,6 +145,22 @@ TEST_CASE("Reject negative mining.subscribe extranonce2 length", "[mining.subscr
TEST_ASSERT_NULL(stratum_api_v1_message.extranonce_str);
}
+TEST_CASE("Reject fractional mining.subscribe extranonce2 length", "[mining.subscribe]")
+{
+ StratumApiV1Message message = {};
+ const char *json = "{\"result\":[[[\"mining.notify\",\"695482c0\"]],\"4de05269\",1.5],\"id\":2,\"error\":null}";
+ TEST_ASSERT_FALSE(STRATUM_V1_parse(&message, json));
+ TEST_ASSERT_NULL(message.extranonce_str);
+}
+
+TEST_CASE("Reject oversized mining.subscribe extranonce2 length", "[mining.subscribe]")
+{
+ StratumApiV1Message message = {};
+ const char *json = "{\"result\":[[[\"mining.notify\",\"695482c0\"]],\"4de05269\",33],\"id\":2,\"error\":null}";
+ TEST_ASSERT_FALSE(STRATUM_V1_parse(&message, json));
+ TEST_ASSERT_NULL(message.extranonce_str);
+}
+
TEST_CASE("Reject mining.subscribe when extranonce allocation fails", "[mining.subscribe]")
{
StratumApiV1Message message = {};
@@ -456,6 +472,22 @@ TEST_CASE("Reject negative mining.set_extranonce length", "[stratum]")
TEST_ASSERT_NULL(stratum_api_v1_message.extranonce_str);
}
+TEST_CASE("Reject fractional mining.set_extranonce length", "[stratum]")
+{
+ StratumApiV1Message message = {};
+ const char *json = "{\"id\":1,\"method\":\"mining.set_extranonce\",\"params\":[\"deadbeef\",1.5]}";
+ TEST_ASSERT_FALSE(STRATUM_V1_parse(&message, json));
+ TEST_ASSERT_NULL(message.extranonce_str);
+}
+
+TEST_CASE("Reject oversized mining.set_extranonce length", "[stratum]")
+{
+ StratumApiV1Message message = {};
+ const char *json = "{\"id\":1,\"method\":\"mining.set_extranonce\",\"params\":[\"deadbeef\",33]}";
+ TEST_ASSERT_FALSE(STRATUM_V1_parse(&message, json));
+ TEST_ASSERT_NULL(message.extranonce_str);
+}
+
TEST_CASE("Reject mining.set_extranonce when allocation fails", "[mining.set_extranonce]")
{
StratumApiV1Message message = {};
mutatrum
reviewed
Aug 7, 2026
| return malloc(size); | ||
| } | ||
|
|
||
| static bool is_even_hex(const char *value) |
Collaborator
There was a problem hiding this comment.
This is a weird name. Initially I though 'even' was referring to mod 2 length, but I think it's means to say "is it even a hex string"?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This hardens the Stratum V1 network receive and parsing paths against malformed or unbounded pool input.
extranonce2_sizevalues frommining.subscribeandmining.set_extranonce, and validate the range again before generating work.mining.notifyfields and Merkle branch elements before dereferencing them, and handle every allocation failure with complete cleanup.mining.notifyparameters while preserving compatibility with pools that insert extension fields before the finalclean_jobsBoolean.strlen/strncatreceive-buffer growth with explicit length tracking,memcpy, and linear scanning.Why
Stratum V1 messages are controlled by the connected pool and were previously trusted in several memory-sensitive paths. A negative extranonce length could be converted to a very large unsigned size during work generation, malformed
mining.notifyfields could reach string and allocation operations with invalid pointers, and a connection that never sent a newline could force unbounded heap growth. These checks make those inputs fail closed without corrupting memory or silently leaving the protocol coordinator in the wrong state.Compatibility
The parser accepts the standard Bitcoin Stratum V1
mining.notifylayout and retains the existing variable-length compatibility behavior used by ckpool-style messages, whereclean_jobsremains the final parameter.Testing
mining.notifyfield types.