Harden Stratum V1 input parsing and work generation - #1849
Conversation
Test Results 2 files 2 suites 0s ⏱️ Results for commit 19ac92e. ♻️ This comment has been updated with latest results. |
27d6388 to
0bcd791
Compare
johnny9
left a comment
johnny9
left a comment
There was a problem hiding this comment.
Reviewed exact head 0bcd791 with the PR-required ESP-IDF 6.0.2 toolchain. I found two work-validation defects and one boundary-coverage gap. The missing-locktime regression fails on the PR head (84 tests, 1 failure); the three sequential proposal patches pass 86/86 ESP32-S3 QEMU tests and a fresh full ESP-IDF 6.0.2 firmware build.
| if (offset + 4 <= coinbase_2_len) { | ||
| for (int i = 0; i < 4; i++) { | ||
| nLockTime |= ((uint32_t)coinbase_2_bin[offset + i]) << (i * 8); | ||
| if (offset <= coinbase_2_len && coinbase_2_len - offset >= 4U) { |
There was a problem hiding this comment.
High: nLockTime is treated as optional and trailing transaction bytes are ignored. A coinbase_2 with no locktime, or with bytes after the locktime, reaches ESP_OK, so the decoder accepts a serialization that is not exactly one complete transaction. I reproduced the missing-locktime case. The proposal reply requires exactly four bytes after the parsed outputs and adds missing/trailing regressions.
There was a problem hiding this comment.
Proposed patch. It applies sequentially to the exact reviewed head and was validated in the combined 86/86 ESP32-S3 QEMU run plus a fresh full ESP-IDF 6.0.2 firmware build:
diff --git a/components/stratum/coinbase_decoder.c b/components/stratum/coinbase_decoder.c
index 25b9b5a..60ffbc0 100644
--- a/components/stratum/coinbase_decoder.c
+++ b/components/stratum/coinbase_decoder.c
@@ -404,11 +404,13 @@ esp_err_t coinbase_process_notification(const mining_notify *notification,
}
// Read nLockTime (4 bytes at the end of the transaction) for BIP-54 detection
+ if (offset > coinbase_2_len || coinbase_2_len - offset != 4U) {
+ goto invalid_coinbase_2;
+ }
+
uint32_t nLockTime = 0;
- if (offset <= coinbase_2_len && coinbase_2_len - offset >= 4U) {
- for (size_t i = 0; i < 4; i++) {
- nLockTime |= ((uint32_t)coinbase_2_bin[offset + i]) << (i * 8U);
- }
+ for (size_t i = 0; i < 4; i++) {
+ nLockTime |= ((uint32_t)coinbase_2_bin[offset + i]) << (i * 8U);
}
// 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 56824d8..e09cb30 100644
--- a/components/stratum/test/test_coinbase_decoder.c
+++ b/components/stratum/test/test_coinbase_decoder.c
@@ -234,6 +234,48 @@ TEST_CASE("Decode regtest P2WPKH address", "[coinbase_decoder]")
// integration-level — the detection logic is tested implicitly through
// the address prefix matching in the full processing pipeline.
+TEST_CASE("Coinbase decoder requires exactly one locktime", "[coinbase_decoder][security]")
+{
+ const char *coinbase_1 =
+ "0100000001000000000000000000000000000000000000000000000000000000"
+ "0000000000ffffffff4b03a5020cfabe6d6d379ae882651f6469f2ed6b8b40a4"
+ "f9a4b41fd838a3ad6de8cba775f4e8f1d3080100000000000000";
+ const char *valid_coinbase_2 =
+ "41903d4c1b2f736c7573682f0000000003ca890d27000000001976a9147c154e"
+ "d1dc59609e3d26abb2df2ea3d587cd8c4188ac00000000000000002c6a4c2952"
+ "534b424c4f434b3a4cb4cb2ddfc37c41baf5ef6b6b4899e3253a8f1dfc7e5dd"
+ "68a5b5b27005014ef0000000000000000266a24aa21a9ed5caa249f1af9fbf71"
+ "c986fea8e076ca34ae3514fb2f86400561b28c7b15949bf00000000";
+ mining_notify notify = {
+ .version = 0x20000000,
+ .coinbase_1 = (char *)coinbase_1,
+ };
+ mining_notification_result_t result = { 0 };
+
+ char *missing_locktime = strdup(valid_coinbase_2);
+ TEST_ASSERT_NOT_NULL(missing_locktime);
+ missing_locktime[strlen(missing_locktime) - 8U] = '\0';
+ notify.coinbase_2 = missing_locktime;
+ TEST_ASSERT_EQUAL(ESP_ERR_INVALID_ARG,
+ coinbase_process_notification(
+ ¬ify, "01020304050607", 8, "", true, &result));
+ free(result.scriptsig);
+ free(missing_locktime);
+
+ size_t valid_length = strlen(valid_coinbase_2);
+ char *trailing_byte = malloc(valid_length + 3U);
+ TEST_ASSERT_NOT_NULL(trailing_byte);
+ memcpy(trailing_byte, valid_coinbase_2, valid_length);
+ memcpy(trailing_byte + valid_length, "00", 3U);
+ notify.coinbase_2 = trailing_byte;
+ memset(&result, 0, sizeof(result));
+ TEST_ASSERT_EQUAL(ESP_ERR_INVALID_ARG,
+ coinbase_process_notification(
+ ¬ify, "01020304050607", 8, "", true, &result));
+ free(result.scriptsig);
+ free(trailing_byte);
+}
+
TEST_CASE("BIP-110 signaling not detected", "[coinbase_decoder]")
{
// Create a mining_notify without BIP-110 bit set
|
Physical Bitaxe 602 smoke test passed on head The application-only OTA reported the expected firmware version, the device returned to healthy mining with three stable API samples, zero fault indication, and the configured pool intact. An independent authorized Stratum V1 probe received a fresh Full test result and artifacts: https://mining-qa-status.vercel.app/results/4e29d544-6585-43ad-8006-da95691ff1a5 Test harness: |
| #define MAX_REQUEST_IDS 1024 | ||
| #define MAX_EXTRANONCE_2_LEN 32 | ||
| #define MAX_POOL_MESSAGE_LEN 256 | ||
| #define STRATUM_V1_MAX_JSON_LINE_SIZE 16384U |
| return false; | ||
| } | ||
|
|
||
| for (size_t i = 0; i < length; i++) { |
There was a problem hiding this comment.
This now loops over the characters twice. One option is to change hex2bin where it accepts a pointer to the index (what the return value is now) and have an esp_return_t to do the check.
|
|
||
| uint8_t coinbase_tx_bin[coinbase_tx_bin_len]; | ||
| size_t coinbase_tx_hex_len = len1 + len2 + len3 + len4; | ||
| if (coinbase_tx_hex_len > STRATUM_V1_MAX_JSON_LINE_SIZE) { |
There was a problem hiding this comment.
Either the constant is wrongly named, or it tests the wrong thing.
| cJSON *difficulty = cJSON_GetArrayItem(params, 0); | ||
| if (!difficulty || !cJSON_IsNumber(difficulty)) { | ||
| if (!difficulty || !cJSON_IsNumber(difficulty) || | ||
| !isfinite(difficulty->valuedouble) || difficulty->valuedouble <= 0 || |
There was a problem hiding this comment.
Is should limit the pool difficulty to whatever we set as ASIC difficulty in device_config, most probably 256. Lower than that will not be reported by the ASIC.
But not sure what failure condition that is to be honest, the miner can still mine, but won't send enough shares to make sense for the pool statistics.
| } | ||
|
|
||
| uint8_t *coinbase_tx_bin = NULL; | ||
| #ifdef CONFIG_SPIRAM |
There was a problem hiding this comment.
We can't get here if PSRAM is not there.
|
Please review stratum |
3 participants
Summary
Treat Stratum V1 pool messages and derived coinbase/work data as untrusted input through receive, parse, and job generation.
Why
Pool-controlled data previously reached conversions, allocations, and transaction decoding with limited structural validation. This could cause invalid reads, unsafe conversions, null dereferences, or excessive allocation. The trusted-pool/network design is unchanged; no authentication is added.
Review follow-up
Incorporates @johnny9's findings: require exactly four lock-time bytes, gate every work side effect on decode success, and add the proposed maximum-line/successor, embedded-NUL, 32-byte extranonce, and NULL/empty-input regressions.
Related work
Related to #1844, which takes a smaller receive/parser-focused approach. This draft also hardens coinbase decoding, binary Merkle handling, work arithmetic, cleanup, and task failure propagation.
Validation
git diff --check