Validate mining.notify fields before use

[Raoulito]

I run a Supra 401 and went digging through the stratum parser this weekend. Turns out a single malformed mining.notify reboots the miner. No big payload needed, no timing trick.

parse_mining_notify() checks params[0] properly but then just uses the rest :

cJSON *job_id_item = cJSON_GetArrayItem(params, 0);
if (!job_id_item || !cJSON_IsString(job_id_item)) { ... return false; }   // checked

new_work->prev_block_hash = strdup(cJSON_GetArrayItem(params, 1)->valuestring);
new_work->coinbase_1      = strdup(cJSON_GetArrayItem(params, 2)->valuestring);
new_work->coinbase_2      = strdup(cJSON_GetArrayItem(params, 3)->valuestring);
...
hex2bin(cJSON_GetArrayItem(merkle_branch, i)->valuestring, ...);
new_work->version = strtoul(cJSON_GetArrayItem(params, 5)->valuestring, NULL, 16);
new_work->target  = strtoul(cJSON_GetArrayItem(params, 6)->valuestring, NULL, 16);
new_work->ntime   = strtoul(cJSON_GetArrayItem(params, 7)->valuestring, NULL, 16);

cJSON sets valuestring to NULL for anything that is not a string, and strdup, strtoul and hex2bin all dereference straight away. The params_count < 8 check only tells you the items are there, not that they are strings. So a notify with 8 params where one of those is a number or a bool or null goes and reads address 0.

I compiled a small thing against managed_components/espressif__cjson to be sure I was not imagining it :

params 1-3 = numbers      params_count=8  (passes the <8 guard)
   param[1]: item=non-NULL IsString=0  valuestring=*** NULL ***
   param[2]: item=non-NULL IsString=0  valuestring=*** NULL ***
   param[3]: item=non-NULL IsString=0  valuestring=*** NULL ***

Same thing with null, true, {}.

The test cases I added crash current master. Built test-ci and ran it in QEMU :

Running Parse stratum mining.notify rejects non-string params...
Guru Meditation Error: Core  0 panic'ed (LoadProhibited). Exception was unhandled.
Backtrace: 0x400556d2:0x3fc96fe0 0x4201166f:0x3fc96ff0 0x4200cce2:0x3fc97020 ...
Rebooting...

LoadProhibited is the NULL read. If you want to see it yourself :

cd test-ci && idf.py build
cd build && esptool --chip esp32s3 merge-bin --pad-to-size 16MB -o flash_image.bin @flash_args
qemu-system-xtensa -machine esp32s3 -monitor none -nographic -no-reboot \
  -watchdog-action shutdown -drive file=flash_image.bin,if=mtd,format=raw -m 4 -serial stdio

While I was in there I noticed calculate_coinbase_tx_hash() builds a VLA out of the same unchecked strings :

size_t coinbase_tx_bin_len = (len1 + len2 + len3 + len4) / 2;
uint8_t coinbase_tx_bin[coinbase_tx_bin_len];

Nothing bounds the coinbase anywhere. realloc_json_buffer() keeps growing until malloc fails, and with CONFIG_SPIRAM_USE_MALLOC=y and ALWAYSINTERNAL=16384 the big allocations go to PSRAM, so those strings can end up much bigger than the 8 KB stack of create_jobs_task. The canary is only checked at a context switch and WATCHPOINT_END_OF_STACK is off, so you would corrupt DRAM before anything complains. This one needs a hostile or really broken pool, so it is less urgent than the NULL read, but it sits in the same function. calculate_coinbase_tx_hash_bin() a few lines below already does it the right way with malloc and a NULL check, so I just made the V1 path match.

What I changed :

• a small require_string_param() helper, used for all seven string fields, same as params[0] was already doing
• type check on the merkle branch entries before hex2bin
• cap each coinbase half at MAX_COINBASE_HEX_LEN (8192 hex chars). solo.ckpool.org sends me 116 and 290, so 406 combined, which leaves plenty of room
• coinbase buffer to the heap with a NULL check
• validation now happens before any allocation, which let me drop the four repeated free() blocks and their one shared exit path

No change to rounding, byte order or hashing. Only validation.

Tests in test_stratum_json.c cover a non-string at each position (1, 2, 3, 5, 6, 7), a non-string merkle entry, and an oversized coinbase. All of them panic before the patch and pass after. 74 tests, 0 failures, and the 27 existing parser tests plus the test_mining ones still pass so moving to the heap did not change any results.

Builds clean on ESP-IDF v6.0.2 for esp32s3, no new warnings, and it has been running on my 401 at 500 MHz against solo.ckpool with no rejects.

Two things I did not do : I never set up a hostile pool to actually trigger the VLA on hardware, and I only own the one board so I have not tried this on a Hex or a Gamma. Also MAX_COINBASE_HEX_LEN is a guess on my side, if someone knows a pool with a bigger coinbase that number should change.

[mutatrum]

We're handing all stratum protocol issues in PR #1897. Can you verify this against that PR, please?

[Raoulito]

#1897 fetches all seven items, then type-checks every one with cJSON_IsString in a single block before any use.
You can close that one @mutatrum