Repository navigation
Security review follow-ups (v1.1.1) - #3
Merged
Merged
Conversation
NanoCLI concatenated method names, option names and values straight into the exec() command line, so any caller forwarding user input (e.g. account_key(['account' => $_GET['a']])) allowed command execution on the node host. - Method and option names must match /^[a-z][a-z0-9_]*$/ (NanoCLIException otherwise); nano_node's own options all fit. - Every value is passed as one escapeshellarg()'d argument. The argv the binary receives is unchanged for benign input. - Documented that arguments land in argv (process list), so secrets must not be passed through this class. Tests (verify.php): injection payloads (&, ;, |, &&, $(), backticks) no longer execute; invalid names throw. They fail on the old code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A seed or key read from a file/env var with a trailing newline ended up in
full in the exception message (and so in logs, error trackers and HTML
error pages); var_dump/print_r of a wallet or block printed the key.
- Exception messages no longer echo seeds, private keys, master seeds,
BIP39 entropy or mnemonic words; they say what was expected instead
("Invalid seed (expected 64 hexadecimal characters)", "Invalid mnemonic
word at position 6"). public2account omits its value too, in case a
private key is passed by mistake.
- #[\SensitiveParameter] on every secret parameter (seed, private_key,
mseed, words, passphrase, hex entropy, API keys) and on the Ed25519 /
BLAKE2b internals that receive secret material, so stack traces show
"Object(SensitiveParameterValue)" instead of the first bytes.
- NanoWallet and NanoBlock get __debugInfo() without the private key.
- NanoBlock validates the key with ctype_xdigit instead of hex2bin (same
acceptance, no warning).
Valid-input behaviour and outputs are unchanged. Tests (verify.php) fail
on the old code.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With follow_location on, PHP's http wrapper re-sent the POST body and the Authorization header to whatever host a redirect named, including plain http hosts. Nano RPC never redirects. - NanoRPC follow_location now defaults to false; an unfollowed 3xx is reported as HTTP_<code> like any other non-200 status. - Opting in (follow_location => true) follows redirects manually, up to max_redirects: https -> http downgrades and non-http(s) targets are refused, the Authorization header is dropped when the origin (scheme, host, port) changes, and 301/302/303 continue as body-less GET while 307/308 repeat the POST, as PHP's wrapper does. - nanophp warns on stderr when Basic auth is sent over plain http to a non-loopback host. - The four PUBLIC_NODES answer block_count with 200 directly (checked 2026-09-28), so the CLI is unaffected. Tests (wallet.php, mock node gains /redirect, block_count and echo_auth): default no-follow, opt-in same-origin follow keeps auth, cross-origin drops it, max_redirects, downgrade/resolution helpers. The mock node's request log now goes to the null device: the unread pipe filled up and stalled the server once the suite made enough requests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without HOME/USERPROFILE (cron, some service units, env -i) nanophp read and wrote .nanophp-node in the shared temp directory, so another local user could plant a node endpoint that every later command trusted. - homeDir() returns null instead of falling back to sys_get_temp_dir(); then the saved-node file is neither read nor written and the CLI uses NANOPHP_NODE or http://localhost:7076 (`nanophp node` says it could not save). - On POSIX the saved file is ignored, with a stderr warning and the chmod fix, unless it is owned by the effective user (posix_geteuid, or the home directory's owner when the posix extension is absent) and not group/world-writable. `nanophp node` removes group/other write bits after saving so a umask of 002 doesn't trip the check. Tests (wallet.php) drive the CLI in child processes with a controlled environment: a planted temp-dir file is ignored without HOME (fails on the old code), NANOPHP_NODE still works, the home file is used, and on POSIX group/world-writable files are ignored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… (L5)
den2raw's /^...$/ also matched "1\n" (PCRE "$" matches before a final
newline), producing "1\n000..." which bcmath then rejected with a
ValueError. nanophp only caught Exception, so that became a PHP fatal
error with a stack trace (arguments included). No wrong amount was ever
signed, but the failure mode was ugly and leaky.
- den2raw anchors with \z, so "1\n" is a clean NanoToolException
("Invalid amount").
- account2public and string2burn anchor with the D modifier for the same
reason (a trailing newline now yields false / an exception up front
instead of a base32 decode error).
- nanophp catches \Throwable (main dispatch and the node probe) and prints
only the message.
Tests: verify.php (den2raw/account2public/string2burn with newlines) and a
CLI child-process check that `send "1\n"` fails with one clean line.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ed25519Blake2b::verify accepted public keys and R values of small order. With A = R = identity and S = 0, a "signature" verified for any message, so NanoTool::validSign "proved" ownership of nano_11a11111111111111111111111111111111111111111111111116iq5p4i8 (and other torsion-point accounts) for arbitrary data. Nobody holds these keys, so wallets are unaffected, but "sign in with Nano"-style checks were not. verify() now rejects A or R when 8*P is the identity (three doublings, negligible cost). Genuine keys are never of small order and R = r*B with r != 0 mod L, so valid signatures are unaffected. Tests (verify.php): the review's identity-key forgery and all 64 A/R combinations of the 8 small-order encodings with S = 0 (several verified before), plus a genuine-signature control. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
numToBits dropped leading zeros, so the double-and-add loop ran once per bit of the secret scalar and the addition only on 1-bits: key derivation and signing time depended on the nonce's bit length (~30 us/bit measured) and Hamming weight, the kind of leak lattice attacks (Minerva/TPM-Fail) exploit. docs/CRYPTO.md also called non-constant-time signing "acceptable for server-side wallets", which is backwards. - Secret scalars (private scalar a, nonce r) use a fixed ladder: 256 iterations from a fixed-work bit decomposition (numToBytes, 32 rounds), the addition computed every step and the result selected by index, and the starting identity given a random projective Z so leading zero bits don't run on cheap small numbers. Measured scalarMultBase time is now flat within noise for 8-, 128- and 252-bit scalars (was 3 vs 11 ms for 128 vs 252 bits). - Verification (public scalars only) keeps the fast variable loop. - Outputs are byte-identical: 400 random key/message pairs generated with the old code before the change give identical public keys, signatures and verify results; verify.php gains a 32-vector deterministic digest produced by the old implementation. - Cost: key derivation ~15 %, signing ~30 % slower; verify unchanged. - docs/CRYPTO.md corrected (not fine for server-side signing services), README "Security model" gains a timing bullet, AGENT.md updated. bcmath timing is still value-dependent, so this remains not constant-time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A failed send (node error -> false, or a missing/empty block hash) was
reported as a success entry with block null; only the all-zero hash
counted as an error. Every failure is now {'error' => 'Bad send',
'amount', 'reason' => node error}.
- wallet_send reported 'amount' from the stale loop variable of the
selection loop (another account's whole balance); entries now carry the
amount that account was selected to send.
- When the destination is one of the wallet's own accounts its share is
skipped, so less than 'amount' went out silently. The response now adds
'sent' and 'shortfall' (raw) so callers can see it; wallet_sweep adds
'sent'. Existing keys and entry shapes are unchanged.
- The send idempotency id is bin2hex(random_bytes(16)) instead of the
time-based uniqid().
- A failed wallet_balances call returns false with an error instead of
iterating over nothing.
Tests (wallet.php; the mock node gains wallet_info/wallet_balances/send
for a node wallet with a failing account and requires 128-bit hex ids):
fail on the old code.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A malicious node or peer could exhaust memory or hang the client: - NanoRPC: response bodies are capped by the new max_response_size option (default 64 MiB, null = unlimited). file_get_contents reads at most one byte past the cap, so an endless body is never buffered in full. - WebSocketClient: handshake response headers are limited to 16 KiB and to the connect timeout; once a frame has started, the rest must arrive within frame_timeout (new option, default max(timeout, 30) s), otherwise the connection is closed with an error instead of waiting forever (readStrict used to loop indefinitely on a stalled peer). - NanoIPC: the 32-bit frame length is checked against max_response_size (new option, default 64 MiB) before anything is allocated (a ~4 GiB announcement used to be a fatal allocation), and the body is read in a loop (a single fread returned short on sockets, silently truncating large responses). Oversized/truncated frames close the desynchronized connection and set error. Normal responses are unaffected. Tests: websocket.php (the echo server gains a header-flood path and a stalled-frame message; in-process IPC TCP peer for framed, oversized, capped and truncated responses) and wallet.php (RPC cap). They fail (or crash) on the old code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- I1: strict comparisons where loose ones were left: the "Account not found" error check and the 'raw' denomination switch use ===, the published block hash is compared with hash_equals, in_array for the work source is strict. - I2: work returned by work_generate is checked with NanoTool::validWork against the requested difficulty (and for 16-hex shape) before it is put into a block, with a clear error instead of a rejected publish. node_fallback falls back to local work in that case. - New options work_difficulty_send / work_difficulty_receive (defaults WORK_SEND / WORK_RECEIVE, so mainnet behaviour and requests are unchanged) for networks with other thresholds; the test suite uses them so the mock node can return genuine work. Test infrastructure: the mock node now computes real work at the cheap test difficulty (bogus work otherwise) and caches its signed ledger per test run (MOCK_LEDGER_CACHE), since the built-in server re-ran the key derivation and signing on every request; wallet.php drops from ~38 s to ~7 s. The new tests (bogus node work refused, invalid difficulty option) fail on the old code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RPC error texts and the probed block count come from the node and were printed to the terminal verbatim, so a node could embed ANSI escape sequences (clear screen, recolour, fake lines). nanophp now passes such text through printable(), which removes C0 controls, DEL and UTF-8 encoded C1 controls. Output for valid use is unchanged. Test (wallet.php, mock node gains an /evil path whose errors carry escape sequences): fails on the old code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The examples wrote seeds with `> seed.txt` / `tee seed.txt`, which under the common umask 022 produces a world-readable 0644 file. README, DOCKER.md and the nanophp header now show `(umask 077; ... > seed.txt)` and `chmod 600` for existing files. CLI output is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mnem2mseed accepts any sequence of word-list words (PBKDF2 doesn't need a valid checksum), so a mistyped-but-valid word silently derives a different wallet. A new optional third argument, $verify_checksum (default false), runs the mnem2hex validation first (word count, words, checksum). The default stays off on purpose: existing callers may rely on deriving from non-standard mnemonics, and turning it on would make previously working calls throw. Outputs are unchanged either way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
httpPost/httpRequest mutated responseRaw/status as a side effect, which
PHPStan (level 5, pinned 2.2.16) can't track across the call ("json_decode
expects string, null given"; "null != 200 always true"). They now return
[body|false, status] and [body|false, headers], and __call assigns the
properties. No behaviour change.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
1 participant
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.
Remaining fixes from the security review. The critical wallet-verification fix already shipped in v1.1.0. Outputs for valid input are unchanged: keys, addresses, hashes and signatures were checked against 400 random vectors from the previous code.
NanoCLIshell injection: method and option names are validated, and each argument isescapeshellarg'd.#[\SensitiveParameter], and__debugInfo()hides private keys.Authorizationis dropped when the host changes. The CLI warns when Basic auth goes over plain http.\Throwable.verify().wallet_send/wallet_sweepaccounting: failed sends are reported as errors, per-account amounts are correct, ids are random, and newsent/shortfallfields are added.mnem2mseedchecksum.Version 1.1.1, with notes in
docs/MODERNIZATION.md.Verification:
verify.php(97 checks),wallet.php(58) andwebsocket.php(18) pass on PHP 8.5 locally and on PHP 8.2 Alpine in Docker, as root and as a normal user.Behaviour notes:
chmod go-wor by re-runningnanophp node.🤖 Generated with Claude Code