FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

2.0.12: Fuzzing, TSAN & hardening by tdewey-rpi · Pull Request #1738 · raspberrypi/rpi-imager · GitHub

2.0.12: Fuzzing, TSAN & hardening - #1738

Merged
tdewey-rpi merged 77 commits into
mainfrom
dev/tdewey/hardening-2026-09
Sep 23, 2026
Merged

tdewey-rpi merged 77 commits into
mainfrom
dev/tdewey/hardening-2026-09

Conversation

Copy link
Copy Markdown
Collaborator

No description provided.

tdewey-rpi force-pushed the dev/tdewey/hardening-2026-09 branch 2 times, most recently from 4c229b7 to 8b2cfe1 Compare September 14, 2026 15:52
tdewey-rpi marked this pull request as ready for review September 14, 2026 16:22

Copy link
Copy Markdown
Collaborator Author

Tested on macOS, including secure boot re-provisioning.

A sweep over values read from a card, a manifest or a download and then
used unchecked: FAT filenames that escaped their directory, partition
offsets that wrapped a 32-bit multiply, write sizes and alignments taken
from the device, OS list icons resolved through nested URLs against the
selected language, and objects outliving the thread that owned them.

Each is now refused where it is read rather than where it is used, so a
malformed image fails with a message instead of seeking somewhere
arbitrary on the card.
The scrollbar was laid out over the content rather than beside it, so
the last column of the OS list and of the advanced options sat beneath
it and could not be read or clicked. A write error was replaced by the
next progress update before anyone could read it, and is now held until
it is acknowledged.
Several harnesses ran the code under test and asserted nothing, so they
could only fail by crashing; each now states what must hold, and the
assertions that could not fail are replaced. The QML storm generator
drove a list it had not waited for, and the cases that use it take a
fixture instead. Sanitiser builds gain leak detection and the libstdc++
assertions.
…y id

A long filename read off a card reached the OS list, a map key and the
terminal with whatever bytes it held. FAT forbids control characters, so
they are dropped as the name is assembled; NUL survives, because it
terminates the name.

The icon fetcher keyed its replies on the requester's address, which a
cancelled fetch frees, so a later fetch landing on the same address was
handed the wrong icon. Replies are matched by id instead.
The gadget served was the one for the family first seen rather than the
one now connected, and the drive list dropped its USB boot and chip
annotations the moment the bootstrap gadget took over, so a board
part-way through the handover appeared as an unrelated device.

The suite around it is steadied at the same time: Catch2 v3 is required,
the suspend inhibitor links into the probes, and the screenshot, memory
and write-pacing cases stop racing what they measure.
…verbs

The hdiutil verbs used to convert and verify the image are deprecated
and were already warning on every release; the supported spellings are
used instead, and the image is compressed with LZMA. A Gatekeeper
assessment that cannot be made now fails the release rather than passing
it in silence, and a build tree whose macOS SDK has moved is refused
rather than quietly linked against whichever one is left.
bootfiles.bin was rewritten on its way out and then served from the copy
first downloaded rather than the current one, the reboot-order directive
was written under a name recovery.bin does not read, and the recovery
config was edited inside upstream's file instead of written as our own.

The file a device asks for is now logged by name, and the disconnect --
not a timer -- is taken as the sign that it rebooted.
Paths and file URLs were converted in a dozen places that disagreed
about anything outside ASCII, so an archive under a path in another
script could not be opened. The conversion now lives in one place and
the archive is read through QFile.

Alongside: a readability check that guessed from permissions now asks
the platform, a thread already cleared by terminate() is no longer
waited on, the telemetry endpoint accepts "off", a redirected repository
URL is adopted only where redirects exist, and the password salt comes
from the system CSPRNG.
The test binaries started without the paths they depend on, the QML
cases read fonts and icons from the installed module rather than staging
their own, and one case assumed a path would be unwritable instead of
asking for one that is. The Windows platform layer -- diskpart, file
operations, WinFile, the secure boot crypto and the privilege probes --
gains a first set of cases. BUILDING.md was hidden by an ignore rule,
and now says what the Windows suite wants installed.
Cases were skipped wholesale on Windows rather than by what they
actually need, and those that did run guessed at interpreter and
archiver paths instead of finding them, then ran a tool the gate had not
checked for.

Running them turned up a disk number parsed by throwing, a delayed-load
stub given a relative output path, an rpiboot request matched as a
string prefix rather than by path component, and an EEPROM version
sidecar written as text, which on Windows means CRLF.
The callback relay, the font engine, the WLAN profile reader, the
renderer choice, the drive list and the WinFile accessors had no cover
on Windows at all.

Writing it turned up a CNG hash freeing its buffers twice, a WinFile
position failure handed back as a position, and a boot image mounted on
whatever letter answered rather than one known to be free -- which on a
busy machine is somebody else's volume. The write buffer is now sized to
what the device will take, and cases that start binaries count them
rather than race.
A boot sector describing more than the volume holds, a root directory
cluster below the first, a cluster past the end of the partition, a FAT
chain pointing back at itself, and an FSinfo sector whose signature does
not match are each now refused rather than followed.

The rpiboot cases gain the two ways a board stops answering, a fastboot
transfer that fails part-way, and an EEPROM section pointing outside the
image it belongs to.
secureSettingsFile judged permissions by a POSIX mode Windows does not
keep, so it warned on every start; it now asks through
QNtfsPermissionCheckGuard and decides on the Everyone entry, and
verifies the restriction it applies rather than assuming it took.

Two advanced options shared a bit, so setting one set the other. The
keyboard fallback did not say which default it owned. A cancelled
synchronous write is reported as cancelled, not as failed. The revision
code table and the proxy URL are split from what reads them, so both can
be covered without a Pi or a proxy.
…ilters

ssh-keygen was looked for in System32, which for a 32-bit process is the
32-bit tree, so key generation never worked on 64-bit Windows; the
lookup tries SysNative as well, behind the platform layer.

The native open dialog held pointers into a vector that reallocated, so
its filters pointed at freed memory. A failed synchronous write recorded
no error code. The openssl stand-in four secure boot cases relied on was
an extensionless shell script, which Windows will not run -- so those
cases passed through the "not installed" path and proved nothing.
Key generation and the OTP key hash shelled out to openssl, which
Windows does not ship, so secure boot provisioning could not work there
at all. Both now go through CNG and a shared SPKI parser, and the UI no
longer offers to run a key generator that was never there.

Counterfeit card detection was compiled out on Windows for no stated
reason and is back. The WLAN passphrase wipe cleared a detached copy,
leaving the original in memory. Controlled Folder Access in the mode
aimed at disk writes was read as a hardware fault.
…part

Creating a boot image asked diskpart to attach a VHD and then wrote to
whatever drive letter it reported. On a machine with anything else
mounted that was another volume entirely, so the image went over
somebody's drive Z -- and it needed elevation to do it.

The image is now formatted in process by DiskFormatter and filled
through the FAT driver: no elevation, no letter, no diskpart. boot.sig
and pieeprom.sig are written as bytes rather than as text, and the FAT
walk can begin somewhere other than the root.
writeFile() refused any path holding a directory: getDirEntry() seeks to the
root first, so the entry would land at the top level. readFile() and
deleteFile() each carried a private walk around that, one level deep;
fileExists() and fileSize() did not split the path at all.

One walk now serves them all, creating directories as needed. It exposed two
faults: a name of exactly 13 characters carries no NUL and truncated to
nothing, and the chain was followed by FAT type, not by directory.
There were three implementations. Windows drove diskpart, which cannot attach
a raw file as a VHD, so it never worked. Linux ran mkfs.vfat and mtools, a
package the Debian dependencies never listed. macOS mounted the image and
carried on past a file it could not write, returning an incomplete one
reported as good.

DiskFormatter and DeviceWrapperFatPartition do both jobs in process, so one
implementation is under test rather than one per platform, and no external
formatter is needed. dosfstools and fdisk leave the packaging with it.
…can run

The driver cannot mark its own work: a case that writes and reads back with
it agrees with itself however wrong the result is. mtools builds the
reference, and the two check each other both ways round.

Three sets of cases stopped skipping. The boot image ones carrying a
directory no longer need mtools on Windows. The exFAT refusal writes its own
volume boot record rather than wanting mkfs.exfat. The legacy driver cases
build a partitioned scratch image rather than wanting a real card.
The three files were written this year and carried 2025. The comments beside
them narrated what the code used to do, which the history already records;
they now give the reason a reader needs instead.
connectTokenCleared meant two things: a successful write consuming the
single-use token, and an OS or storage change dropping a key minted for
something no longer selected. The wizard reset piConnectEnabled for both, so
the sidebar un-bolded Raspberry Pi Connect as the write finished, beside a
completion summary that still listed it.

The signal now says which. What the generator reads is cleared either way, so
Connect setup cannot reach the next card with no token behind it.
The flag behind it was covered and the label it feeds was not, which is the
half a wrong answer hides in. The case asserts on the Text that is drawn
rather than the MarqueeText around it: that wrapper exposes font as an alias,
so a case reading it cannot tell whether the font reached the glyphs.
CTest runs each case as its own invocation, several at once, so the fixture
image was formatted by one process while others read it. That either skipped
the case for want of the file or handed the driver a half-written FAT, which
it answers by extending the directory a cluster at a time until the image is
full -- thirty-three minutes of spinning before the run was stopped.
A virtual disk is not removable media, so IOCTL_STORAGE_EJECT_MEDIA has
nothing to act on -- but the volumes were dismounted before it was tried. The
disk stayed attached with nothing mounted: still listed as a drive, and the
file behind it could not be attached again because it never was detached,
which reads as an image that has been corrupted.

The backing file comes from the storage dependency query, which answers only
for a disk that has one, and detaching it is what ejecting one means.
The drain loop had never run with anything on it: one small write completes
before the wait is reached, so the completion port wait, the context lookup
and the callbacks were cold. Filling the queue first reaches them, and the
case asserts writes were in flight rather than passing on an empty queue.
Cancelling mid-flight is covered the same way.

The open backs off when another process holds the drive, and gives up early
when cancelled. Both need a virtual disk, so both skip without one.
DrainAndSwitchToSync was only ever asked of an empty queue, which returns
before its polling loop. The replay behind the five-minute emergency timeout
had no cover at all: nothing can wait that long, and nothing can stop a
scratch file completing, so it is reached through the test API instead. What
it checks is the part that matters -- every outstanding buffer written at the
offset it was given, not wherever the file pointer had reached.
tdewey-rpi and others added 28 commits September 23, 2026 13:39
The check ran on every Unix, but macOS reports elevated by design:
authopen elevates each device open. Pin that answer there instead.
gcovr rejects a hit count large enough to look like gcov's overflow bug.
allocateCluster scans the whole FAT, and the suite now formats enough
filesystems to reach seven billion iterations of its inner loop, so the
report failed after the instrumented suite had already run -- forty minutes
spent for nothing. The count is real, so the line is warned about rather than
refused, and objects whose working directory cannot be inferred no longer
forfeit the other ninety-odd sources either.
A table whose entries climb steadily never repeats, so the circular guard
never fired and the walk followed it to the end of the table -- checking the
repeat by scanning everything collected so far, which is quadratic in the
length. On a card-sized partition that is minutes of a busy processor, which
a user reads as the imager having hung.

A set answers the repeat in constant time, and a chain cannot hold more
clusters than the partition has.
The eject case attached its disk the way every other case does, tied to its
own handle -- which only that handle can detach, so DetachVirtualDisk from
the code under test answered ERROR_NOT_READY. It now attaches the way
Explorer and Mount-VHD do, which is the shape the product meets.

The retry case asked whether a failed open was worth repeating without
failing one first, so it read the error code of an open that never happened
and correctly got no. It now holds the drive, fails the open, and asks.
… applies

A token dropped because the OS or storage changed was covered only on the
Pi Connect step, which may not be loaded when it goes. The container must
unset the step then, since nothing was written with that token, and clear
what the generator reads.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The walk had a case; the return that stops a file being walked did not, nor
did the one-argument form the application actually calls -- which asks the
platform who invoked an elevated run rather than being told.

Four lines remain uncovered here and genuinely cannot run on Windows: they
need a chown to succeed, and giveTo answers false on a platform with no
POSIX ownership to hand back.
A size of nought or less means the caller's arithmetic over the files went
wrong, and an empty set means there is nothing to pack: both are refused, and
neither leaves a part-made image behind to be signed and served.

A path from the caller may arrive with a leading separator, which names the
root of the image rather than of the host and is stripped. One that is
nothing but separators names no file and is skipped, so a stray entry cannot
fail the whole image.
The arguments decide what a Pi accepts as its own bootcode, and none of the
refusals had a case. A key number or a version outside what the header holds
would otherwise be truncated into it: a signature over one set of fields
describing another.

Also that nothing partial comes back when there is no key to sign with, and
that a refused boot signature leaves no file behind -- a truncated one would
be served as though it were a signature.
A firmware archive is not all regular files, and it is re-packed after a
counter-signed bootcode is spliced into it before being served to a device.
The write side had no case covering a symlink, a recorded owner or a
directory, so an entry kind dropped on the way out would have produced a
different archive from the one that came in, with nothing to say so.
The cluster count decides the FAT type: below 65,525 a volume is a FAT16 to
every driver that opens it, whatever the boot sector claims. Driving the
arithmetic through a whole format only asks it the sizes a format is given,
so the degenerate ones had nothing holding them -- a partition no larger
than its reserved sectors, and one whose FATs fill what is left.

Both now answer no clusters rather than a count the boot sector could not
honour, and more space never yields fewer.
The existing case packs its files into four megabytes, which is below what
FAT32 can hold at all -- so the format is refused and nothing is ever
written. The fill failing was never reached, and its comment still described
mcopy, which that path no longer uses.

A legal FAT32 size with more files than it holds reaches it: the writer
throws partway, and what must not survive is a correctly sized, correctly
formatted image missing some of its contents, ready to be signed and served.
Whether a failed open is worth repeating is decided by the error it failed
with, but only for a path shaped like a physical drive -- a file is refused
on the shape alone, which is all the unelevated cases reached.

A drive number nothing is using gets past the shape check and is judged on
the error, which says "not there". That will not become true by waiting,
unlike the sharing violation a real drive gives while Windows re-enumerates.
Running out of input reaches libarchive as "No progress is possible", and the
two sources need different answers. For a download that has just finished it
is the tail of a race between the producer signalling completion and the last
bytes being consumed, so it is taken as the end of the stream -- without that
every download would fail at 100%.

A file on disk has no such race: it ran out because it is truncated. The
download half was covered; the local half was not.
…s told

Windows will not refuse SetupDiGetClassDevsW on demand, so the branch behind
it had no way to be reached: when enumeration fails the list returns a
sentinel, because an empty one is what a machine with nothing plugged in
returns and would tell the user to plug something in.

The refusal is forced by a linker flag on this target alone. A call into a
DLL goes through __imp_<name>, a data symbol holding the address, so what is
redirected is a pointer. The drive list is compiled unchanged.
Every buffer the formatter takes is checked and each check refuses the whole
format, but none had been reached: the allocations are small and succeed on
any machine that can run the suite. A card half-formatted because memory ran
out mid-write was nobody's tested path.

Six allocations happen in a format, and each is failed in turn rather than
this case having to know which refusal is which. A second case pins the count
so the walk cannot shrink to one refusal and still pass.
Neither outcome can be arranged on a scratch file: FSCTL_LOCK_VOLUME is
refused on anything that is not a volume, and a flush of a small file does
not fail. So a lock being granted, the state it records, the unlock that
depends on it, and the refusal when a sync cannot finish had all never run.

The write path syncs before calling a card written. A sync reported as
successful when the flush failed is the difference between telling somebody
their card is ready and telling them it is not.
No USB support, or no permission to use it. The scan runs while the drive
list is built, so an exception escaping it takes the list with it: the user
gets no drives at all, on a machine whose drives are fine, because a library
they are not using did not load.

libusb is linked statically, so this wraps the plain symbol rather than the
import pointer a DLL needs. A second case checks the scan completes when
libusb does start, so the first cannot pass by always returning nothing.
The allocation walk opens with a clean format behind a bare REQUIRE, so a
refusal for a reason other than the injector -- the disk, the path, a
scanner holding the file open -- came back with nothing to go on. One run
in several thousand under parallel load did exactly that.

Names the FormatError, and reports how many allocations each armed run
reached, so a format that never met the armed allocation is not read as a
refusal that failed to happen.
An extraction that fails part way through deletes what it unpacked, but
the libarchive write handle was not closed until after that loop, so it
still held the entry it was filling. Windows will not delete an open
file, QFile::remove()'s result was discarded, and a corrupt download left
part of an OS on a card the user was told had failed.

Closes the handles first. The working directory stays on the target while
the rollback runs, because the entry names are relative to it, and files
that still survive removal are now named in the log.
…g it

QueryPlatformDeviceIOLimits sizes the write buffer and the async queue
depth before the device is opened, and none of it ran: the only case
entering it is tagged [.diagnostic], which Catch2 hides and CTest
therefore never schedules.

Pins the guard that keeps a property IOCTL off a plain file, the
case-insensitive match on a path Drivelist spells as it likes, a drive
that is not there, and the values the system drive reports. No elevation:
the query opens with zero access. It skips if that open is refused, so it
cannot pass on defaults it never asked for.
It was the one case in the suite asserting nothing at all, which Catch2
counts as having proved nothing: a monitor that started and stopped
without crashing passed, and so would one whose stop threw, reported
against whichever case ran next.
The minted secret is written into the image inside a cloud-init runcmd
heredoc. One carrying a newline closes the heredoc early and what follows
runs as root on first boot, so ImageWriter checks the shape before
keeping it -- and that refusal had never run. verifyAuthKey was tested;
its use was not.

A well-formed secret alongside it, so the refusal is about the shape
rather than a mint that never got that far. The registrar's localhost API
stand-in moves to a header, both tests now sharing it.
formatSize() assembles the fractional part by hand, so only the whole
part goes through QLocale::toString() and its digit mapping. A reader of
Arabic or Bengali would otherwise be shown a figure in two numbering
systems, one either side of the decimal point.

The mapping had no test. Skips where Qt renders the locale in Latin
digits, so it cannot pass by having nothing to map.
The shipping manifest asks for requireAdministrator, so CreateProcess
refuses the binary from an unelevated process and QProcess reports a
start that never happened. Every case here skipped, leaving cli.cpp the
least covered file in the tree -- a shipping entry point, untested.

cli_harness is the same Cli from the same library with no manifest.
run() asks about privileges before it reads an argument, and that ends in
CheckTokenMembership, so a linker wrap answers it and the product carries
no test seam. Eighteen cases now run; the refusal itself is one of them.
archive_error_string() answers null where libarchive set no message, and
Bootfiles appended it straight to a std::string in seven places. That is
undefined, and it crashed: the three refusals in writeToFile() all segfault
the moment they fire. The other callers in the tree already guard this.

Found by making libarchive refuse the write, which nothing could do before
-- the archive is small and the destination a temporary directory, so a
short write, a refused header and a failed close were all unreachable. A
bootfiles.bin written short is a board that will not boot.
With nothing listening, the relay starts the Imager beside it and hands
over the callback URL. That is Pi Connect sign-in for anybody whose Imager
is not open, and none of it ran: reaching it means launching the real
binary, which asks for administrator and would put a UAC prompt in front
of the suite.

The relay is copied somewhere of its own with a stand-in beside it that
records what it was started with. It resolves the Imager against its own
module path, so the copy can reach nothing else.
A scratch file completes before the second write polls, so the refusal
reached the third write, not the fourth this case required, and it failed
on every run here. A slow card collects it in the wait for a slot instead.
Both are correct; the case now holds only what is guaranteed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
platformhelper.cpp is in the shared source list, so the CLI build
compiles it, and ddaeeb9 gave it an accessibility observer built on
QAccessible, which is Qt GUI. The CLI links only Core and Network, so
its release build stopped with "QAccessible: No such file or
directory". The observer is now left out of CLI builds, where
assistiveTechnologyActive is always false: with no window there is
nothing for an assistive technology to attach to.
tdewey-rpi merged commit d0ad657 into main Sep 23, 2026
tdewey-rpi deleted the dev/tdewey/hardening-2026-09 branch September 23, 2026 15:05
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant


Back | FazBrowse Home | New Git URL