`pihole-FTL --config <key> <value>` puts the value in argv, which is
visible via /proc/<pid>/cmdline and `ps -eo args=`. For secret values
(notably the web password) this leaks to other unprivileged users for
the lifetime of the child process.
Allow reading the value from stdin by passing `-` in place of the
value: `echo secret | pihole-FTL --config webserver.api.password -`.
fgets reads up to 4095 bytes into a stack buffer, trailing CR/LF is
stripped, and the existing set_config_from_CLI validation pipeline is
reused unchanged.
Buffer is bounded at 4096 bytes; values that fill the buffer without
a terminating newline are rejected with a clear error rather than
silently truncated.
Signed-off-by: bcambl <5565939+bcambl@users.noreply.github.com>
Expose a cJSON-backed JSON decoder to Lua scripts via the pihole
library. Uses the thread-safe cJSON_ParseWithLengthOpts variant with a
stack-local error pointer, consistent with other FTL cJSON consumers
(teleporter.c, cli.c, http-common.c).
Includes BATS tests for round-trip, nested objects, empty containers,
and invalid-JSON error reporting.
Signed-off-by: bcambl <5565939+bcambl@users.noreply.github.com>
The log_debug() between waitpid() and the ECHILD check can clobber errno, so the check must use the cached err. With the live errno the goto was skipped and the loop spun forever (seen only with DEBUG_CONFIG enabled).
Signed-off-by: DL6ER <dl6er@dl6er.de>
The API test suite asserts exact DNS query counters. Several of them
(`TOTAL`, `DNSKEY`, `DS`, `TOP_DOMAIN`) depend on how many DNSKEY/DS
lookups dnsmasq issues while validating DNSSEC. Until now those lookups
recursed to the live ICANN root, because FTL configures the real root
trust anchors whenever `dns.dnssec` is enabled and the local PowerDNS
recursor had no root zone of its own. The number of root DNSKEY queries
therefore tracked ICANN's published root key set, so an ongoing
key-signing-key rollover silently shifted the counters (9 -> 7 DNSKEY)
and broke the suite even on unrelated PRs.
We make the whole suite hermetic:
1. Serve a locally-signed root zone from PowerDNS, forward `.` to it and
trust its key, so root DNSKEY validation resolves inside the test
environment instead of reaching the internet.
2. Mark the locally-served *unsigned* zones (`icloud.com`,
`apple-dns.net`, `in-addr.arpa`, `ip6.arpa`) as local domains, so
dnsmasq no longer proves them unsigned by walking up to the real root.
3. Give the `bogus` zone a deliberately mismatched local trust anchor so
it fails validation locally rather than by failing to find a secure
delegation at the root.
4. Drop the root-key pre-warm `dig`, an internet round-trip that no
longer serves any purpose.
With no query leaving for the real root the counters are stable and
independent of ICANN key rollovers, so they are recalibrated
accordingly. Marking the extra zones as local emits the same "negative
DS reply without NS record" warning already whitelisted for `ftl`, so
the `test_final` whitelist is broadened to match it for any zone.
Signed-off-by: DL6ER <dl6er@dl6er.de>
test_dnsmasq_config() read dnsmasq's piped stderr one chunk at a time and kept only the last read. On slower architectures (e.g. RISC-V) the output splits across reads and the final read can be a lone newline, which the newline handling collapsed to an empty string - so an invalid config was reported as "not valid (), config remains unchanged" with the error text lost. Accumulate the full output, then keep the last non-empty line, realizing the intended "last pipe line" behavior independent of read chunking.
Signed-off-by: DL6ER <dl6er@dl6er.de>
readEnvValue() committed the parsed value into the config item before running the validator, so a rejected value still reached the generated dnsmasq config. Take a deep snapshot of the prior value and type before the commit and restore it (freeing the rejected value) when validation fails, freeing the snapshot on success.
Signed-off-by: DL6ER <dl6er@dl6er.de>
validate_dns_hosts() stopped scanning at a # comment token and never rejected embedded newlines, unlike the sibling validators, so a crafted entry could inject extra lines into the generated hosts file. Scan the whole value for CR/LF and reject it, matching validate_dns_cnames().
Signed-off-by: DL6ER <dl6er@dl6er.de>
Cover three fixes from the security pass: session cookies carry the Secure attribute only over TLS (and never over plain HTTP), an accepted TOTP code cannot be replayed, and /api/queries clamps an unbounded length parameter. The cookie and TOTP tests live in a self-contained module so they do not disturb the order-dependent workflow in test_z_auth.py; the TOTP test clears the shared secret via the session created by the accepted login so cleanup neither trips the replay guard nor emits a 2FA warning. Account for the module's four extra pihole.toml writes in test_final.bats.
Signed-off-by: DL6ER <dl6er@dl6er.de>
The length parameter was unbounded, so a single /api/queries request could materialize the entire query history as one in-memory JSON document. Cap the number of rows materialized at a hard maximum regardless of the requested length (including the documented all case) and regardless of the disk source.
Signed-off-by: DL6ER <dl6er@dl6er.de>
Per-element string-type validation was skipped for GRAVITY_GROUPS, so a payload with a non-string array element reached the SQL bind and error logging with a NULL value. Add a pre-pass that rejects any non-string array element with 400 for every list type.
Signed-off-by: DL6ER <dl6er@dl6er.de>
The carray bind cache keyed only on the intarray offset. Since the group_ids pointer is stored with SQLITE_STATIC, a shared-memory remap (mremap MREMAP_MAYMOVE) could move the backing array while the offset stayed the same, leaving SQLite with a dangling pointer. Also compare the pointer and rebind when it changes; reset the new pointer caches in both cache-reset paths.
Signed-off-by: DL6ER <dl6er@dl6er.de>
add_FTL_clients_to_network_table() re-fetched the client pointer after releasing the SHM lock and used it without a NULL check; a recycled slot could yield NULL (crash) or a different client (cross-contamination). Snapshot the needed fields under the initial lock and read from the snapshot; re-fetch only to write back numQueriesARP, guarded by a NULL check. Also release the lock on the DB_FAILED break.
Signed-off-by: DL6ER <dl6er@dl6er.de>
When the shared string pool was full, avail_mem - 1 underflowed and the pagesize clamp could bypass the available-memory clamp, letting strncpy write past the pool; truncated strings were also left unterminated. Bail out early when the pool is full, apply both length clamps independently, and force NUL termination after strncpy.
Signed-off-by: DL6ER <dl6er@dl6er.de>
nameFromDNS() and the answer loop advanced through a PTR reply with no buffer-end bound, allowing a stack over-read on a malformed reply. Pass an explicit end pointer, bound the label loop and the compression-pointer dereference against it, verify the resource-record header fits before casting, and NULL-check the parsed names in the caller.
Signed-off-by: DL6ER <dl6er@dl6er.de>
The EDNS0 Client Subnet branch read four fixed bytes and computed addrlen = optlen - 4 without checking optlen >= 4, risking an over-read and an addrlen underflow on a crafted option. Gate the branch on optlen >= 4 and, in the unknown-family path, advance by optlen - 4 so option iteration stays in sync.
Signed-off-by: DL6ER <dl6er@dl6er.de>
Several allocations were dereferenced without a NULL check, turning an allocation failure into a crash: the API payload buffer (also guard read_and_parse_payload), the API-hint buffer in the Lua handler, the redirect target in the .lp handler, and the ZIP read buffer in the teleporter (also reject a zero-size archive). Fail the request with 500/false instead.
Signed-off-by: DL6ER <dl6er@dl6er.de>
Refuse to step the system clock by more than a panic threshold (1000s, exempting the first sync after boot for RTC-less devices) so a consistent spoofed server cannot move the clock arbitrarily; use an unpredictable random 64-bit nonce as the on-wire origin timestamp (keeping the real send time separately for the T1 offset/delay math) so off-path reply forgery is infeasible; and commit ntp_stratum only after the sync round is validated, using the most conservative surviving stratum.
Signed-off-by: DL6ER <dl6er@dl6er.de>
verifyTOTP() only remembered the last accepted 6-digit code, so a captured code stayed replayable across its -1..+1 acceptance window. Track the last accepted RFC 6238 time-step counter instead and reject any matched step <= the stored counter, deferring the reuse verdict until the whole window is scanned so a genuinely fresh code is still accepted.
Signed-off-by: DL6ER <dl6er@dl6er.de>
Add the Secure attribute to session cookies when the session is established over TLS (so a TLS session's SID is not exposed over plaintext HTTP), stop logging plaintext passwords on failed logins (log only the length, and memset the buffer), compare SID/CSRF tokens in constant time via nettle memeql_sec behind a length gate, and make generateSID propagate RNG failure so a login can no longer create a session with an empty SID/CSRF; reject an empty presented SID in check_client_auth as defense in depth.
Signed-off-by: DL6ER <dl6er@dl6er.de>
CivetWeb URL-decodes local_uri_raw in place, so an encoded CR/LF (%0d%0a) in the path arrived as a literal newline. redirect_lp_handler reflected that path into the Location header of a 301 redirect verbatim, before any authentication, allowing unauthenticated HTTP response-header injection / response splitting (e.g. Set-Cookie fixation). Reject any request whose decoded URI contains control characters from a begin_request hook, which runs before auth and every handler, closing the whole class of URI-into-header injection.
The rejection is logged only at debug level and never echoes the URI: begin_request runs for every unauthenticated request, so logging each one at warning level (or logging the URI verbatim) would itself be a log-flooding / log-injection vector.
Add raw-socket API tests (TestURIControlCharRejection) verifying an encoded CR/LF path is rejected with 400 and not reflected into response headers, a bare control character is rejected, and a clean .lp request is still served.
Signed-off-by: DL6ER <dl6er@dl6er.de>
format_ntp_message() interpolated level/who/message into the HTML message body without escape_html(), unlike every sibling renderer. The server hostname is validated to [a-zA-Z0-9._-] today so no markup reaches it, but escaping keeps the renderer consistent and defends against any future, less-restricted input routed through it.
Signed-off-by: DL6ER <dl6er@dl6er.de>
request_handler() made its web-home and authentication decisions on local_uri_raw, which is URL-decoded but not dot-segment-normalized, while CivetWeb serves files from the normalized local_uri. A path such as /x/../admin/... appeared outside the web home, so the handler fell through without authentication yet CivetWeb served the protected page. Decide on local_uri instead.
Signed-off-by: DL6ER <dl6er@dl6er.de>
compile_filter_regex() set N_regex to the full array length while skipping empty/invalid entries without advancing the write index, leaving trailing zero-initialized regex_t slots. api_queries() then ran regexec() over those slots, dereferencing a NULL program pointer and crashing FTL (and with it DNS). Report the count actually compiled, free partial/unused allocations, and treat an all-empty list as "no filter".
Signed-off-by: DL6ER <dl6er@dl6er.de>
cJSON_Print() returns a heap string that was passed straight to log_err() and never freed, leaking on every non-string dns.revServers entry across config writes/reloads.
Signed-off-by: DL6ER <dl6er@dl6er.de>
HTTP Host is case-insensitive (RFC 7230 s5.4), so Host: PI.HOLE should
match domain pi.hole. Replace strncmp with strncasecmp (already available
via FTL.h) so the redirect fires regardless of Host header casing.
Signed-off-by: slmingol <slmingol@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The host-vs-domain comparison used strncmp(host, domain, host_len), which
only compares host_len bytes of domain. A host value shorter than domain
(e.g. "pi" vs "pi.hole") would incorrectly match. Add an exact-length
equality check before strncmp to prevent false positives.
When the host does not match webserver.domain and the handler falls through
(returning 0), CivetWeb's disabled-directory-listing path produces a 403
that uses the admin error403.html, making the response look like an
intentional block. Replace the silent "Not redirecting" log with a message
that names the mismatched host and domain so users can diagnose reverse
proxy Host header misconfigurations without needing to read source code.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: slmingol <slmingol@users.noreply.github.com>
cJSON_CreateArray() was never released, leaking the array and its duplicated children on every invalid enum value. Also fall back to "(null)" if cJSON_PrintUnformatted()/escape_json() return NULL so the error message never formats a NULL pointer.
Signed-off-by: DL6ER <dl6er@dl6er.de>
cJSON is a linked list, so cJSON_GetArrayItem(arr, i) is O(i) and calling
cJSON_GetArraySize() in a loop condition re-walks the whole list on every
iteration. A number of config, validation and serialization loops walked
their arrays by index, making them O(n^2); the header de-duplication in
cJSON_unique_array() was O(n^3). Walk the list directly instead
(cJSON_ArrayForEach / ->next, and cJSON_DetachItemViaPointer for the
in-place delete), keeping the running index only where it is still used
(error messages, in-place deletion). No behavioural change.
bcambl
yubiuserandGitHub
dependabot[bot]andGitHub
DominikandGitHub
DL6ER
Henry SowellandGitHub
rdevshp
Adam WarnerandGitHub
darkexplosiveqwx
Adam WarnerandGitHub
slmingolandClaude Sonnet 4.6