Audit, hardening and conventions pass: 28 bisectable commits - #55
Merged
Merged
Conversation
Two gaps in the same target, both of which the README already describes as requirements for an ASan build. LinkIncremental was unset, so every link emitted LNK4300: ignoring '/INCREMENTAL' because input module contains ASAN metadata. Seven of the eight ASan projects set it; this one did not, which is why that warning has been on every build of this tree. It also had no CopyAsanRuntime target, and that one is worse than a warning: the loader needs clang_rt.asan_dynamic beside the binary, and without it the exe exits 0xC0000135 before main, so the suite reports a bare non-zero exit and no output at all. It only ever ran by accident. A solution build puts every binary in one directory, so the sandbox projects deposited the DLL next to this exe. Invoke-BlorgChecks.ps1 builds project-by-project, where output is project-local -- and there the exe has no DLL beside it. Which copy the runner picks depends on a recursive glob taking the first match, so this target could pass or fail depending only on which build shape ran last. Deleting the project's output directory is what exposed it. README's sanitizer section now lists all three ASan requirements against the symptom each produces when missed, since "the exe does not start at all" gives a reader nothing to search for and 0xC0000135 does.
CodeQL has been failing every scheduled run on master, and the VM deploy pipeline fails the same way, with error C1041: cannot open program database ... if multiple CL.EXE write to the same .PDB file. Two independent causes, and fixing only the first leaves it red. Across projects: all seven sandbox targets defaulted IntDir to $(Platform)\$(Configuration)\ beside the project, so their object files and one shared vc*.pdb landed in the same folder. MSBuild has been saying so on every build as MSB8028, "can lead to incorrect clean and rebuild behavior". Each target now gets its own directory. Within a project: every sandbox target mixes C and C++ sources, and MSBuild compiles the two groups in separate CL.EXE invocations because their command lines differ. Both write the project PDB, so they race and one loses -- the same error, one level down. /FS is what the error message itself prescribes. What decides whether you see it is /m plus a cold tree. build.yml and Invoke-BlorgChecks.ps1 both build serially, so they never hit it; codeql.yml and deploy/Deploy-ToVM.ps1 both pass /m, and both fail. A warm incremental tree hides it even under /m, because the race needs two language groups compiling at once -- which is why it looked like a CI-only problem right up until a deploy was run from a clean build. Verified in a throwaway worktree, the condition that reproduces it: a full rebuild with nothing cached. Before, that build gave 68 C1041 errors. After, the Build tier, the exact CI command, and the deploy's own msbuild invocation all exit 0 with zero C1041 and zero MSB8028. Not a consequence of the query-pack bump. It predates it; that bump's pull request is simply the first thing that ever ran the workflow.
Deploying needed five settings passed by hand every time -- VM path, guest account and password, the .vmx config-encryption password, the snapshot to revert to -- so a session starting from nothing could not deploy or debug until someone supplied them. deploy/blorgfs.env holds them once and Deploy-ToVM.ps1 defaults from it, with explicit arguments still winning so nothing is locked in. The file is gitignored, and the ignore rule went in before the file was created. Every setting in it is a credential for the debug VM, and the KDNET key in particular lets anyone on the network attach a kernel debugger to that guest, which is the same reason dbgsettings_out.txt is already ignored. blorgfs.env.example is the committed template and carries no real values. Deploy-ToVM.ps1 also stops trusting PATH to find MSBuild. It threw outside a Developer PowerShell, and worse, PATH may hold the 32-bit MSBuild -- the WDK NuGet picks its PREfast and ApiValidator directory from the MSBuild process architecture, so that one silently produces a different build than the check runner does. Invoke-BlorgChecks.ps1 already knew how to find the right one, so that knowledge moves to tools\Get-BlorgMSBuild.ps1 and both callers share it rather than keeping two answers that had already drifted. It also honours MSBUILD_PATH, for an edition the list does not name. Verified by running the deploy with no arguments at all: snapshot revert, parallel build, copy, install, "Deployment succeeded". Fast tier unchanged.
MapUserBuffer had no callers anywhere in src/ or tests/, and it was the only thing in the file -- so removing it left a translation unit containing an #include and a comment. The driver project never compiled Util.c at all: BlorgFS.vcxproj lists only Util.h, which holds the helpers that are actually used (ReallocateBufferUninitialized and friends, all inline). DispatchSandbox was the single project building it, which means this code was never in the shipping driver -- it was dead in the only place it could have mattered and alive only in a test target. Util.h is untouched.
The README said anything exported across a translation unit takes `Blorg`, and 86 functions did not. Most of the offenders were fastfat names carried over from early development -- FsdPostRequest, PrePostIrp, OplockComplete, CompleteRequest, IsIrpTopLevel -- alongside the transport layer's own SendWskAsync/TlsSha256/DrainHttpClient families and the FspCsq callbacks. The rule now holds literally, with no carve-out for layer or ancestry. That is the point: `Blorg` marks external linkage and nothing else, so a bare module name at file scope is a promise the symbol is `static`, and the export list is a grep rather than a reading of four headers. The ported names are this driver's functions now; keeping fastfat's spelling was preserving a provenance that stopped being useful once the code diverged. Purely mechanical: 934 occurrences across 55 files, applied by whole-identifier match, including the sandbox shims and models that implement the same surface. DriverEntry and DriverUnload keep their names, which the OS requires. Statics are untouched and keep their bare module prefix. The full Fast tier passes unchanged, which for a rename is the whole verification: every sandbox target compiles the real sources, so a missed or over-eager substitution is a link error rather than a silent change. Re-running the export scan afterwards reports nothing unprefixed left. README's own example was wrong before this and is corrected -- it cited TlsSetPin as a file-static helper when TlsSetPin was exported.
54 of them across CacheManager.c/.h, FspWorkQueue.c/.h and Util.h -- the last fastfat-era annotations in the tree, everywhere else already declaring parameters plain. They are no-op macros: they document direction to a human and tell the analyser nothing, so a reader who trusts them gets a convention that is only followed in five files, and PREfast gets nothing either way. The one that did say something is upgraded rather than deleted: BlorgCompleteRequest's `PIRP Irp OPTIONAL` really does tolerate NULL and tests for it, so it becomes `_In_opt_` -- modern SAL the analyser reads, in the one place the annotation earns its keep. That matches how the rest of the tree uses SAL: sparingly, and only where it carries a contract (IRQL levels, allocator behaviour, _Outptr_result_nullonfailure_).
Four places where a parameter is read and never written, stated in the signature rather than left to the reader. BlorgGetHttpAddrInfo took PADDRINFOEXW Hints and did nothing but forward it to BlorgGetWskAddrInfo, which already declares it const -- so the wrapper was strictly weaker than the function it wraps, for no reason. BlorgSendWskAsync is the send-only half of BlorgSendRecvWskAsync, and send locks its MDL IoReadAccess. The buffer is an input, so it says const and casts once where the shared implementation still needs PVOID. That is the one place a caller could previously have been handed a mutable alias of a request it had already put on the wire. BlorgAcquireReusableWskSocketAsync copies RemoteAddress into the connect context precisely because the caller's copy may not outlive the call -- an input by construction. SockAddrEqual, which compares two of them, follows. Both need a cast only at WskSocketConnect, whose own signature predates const being applied to this sort of thing. Not done, deliberately: BlorgGetFileEntry/BlorgGetSubDirEntry return interior pointers into the listing they are handed, and Client.c's deserialiser writes through those, so const-ing the parameter would need a const and non-const pair -- more machinery than the guarantee is worth in C. Tls.h and Structs.h were already const-correct throughout. The change earned its keep immediately: SandboxSocket.c's stand-ins for BlorgSendWskAsync and BlorgAcquireReusableWskSocketAsync had drifted from the header they include, which C4028 surfaced only once the real signature moved.
A negative Parameters.Read.ByteOffset reached the prefetcher widened to ULONG64. BlorgTrimReadToFileSize does not catch it -- neither of its comparisons is true for a negative offset -- so nothing trimmed the read, and the ring's containment test, (Offset - RangeOffset) + Length > Hot.Length, wrapped to a small value and reported coverage. The hit path then copied Length bytes from buffer + (ULONG)(Offset - RangeOffset), a displacement near 4 GB: a kernel out-of-bounds read at a caller-influenced offset. The park path stored the same displacement in WaiterSlotOffsets and copied on completion. The I/O manager screens negative offsets out of NtReadFile, so usermode cannot reach this, but a kernel caller that builds its own IRP fills in ByteOffset with nobody validating it -- which is what a filter layered above a filesystem does. Fixed on both sides, because the copy's safety should not depend on a check somewhere else having run. BlorgVolumeRead refuses a negative offset before it reads anything else, and the containment test moves into PrefetchSlotCovers -- shared by the serve scan and the near-miss scan, which each carried their own copy -- ordered so the subtraction runs only after Offset is known to be the larger and comparing against the slot's remaining bytes instead of summing. No expression in it can wrap for any input. Both directions are pinned by tests that were confirmed to fail against the pre-fix code: ReadTest.NegativeByteOffsetIsRejectedBeforeAnyFetch and PrefetchKernelTest.WrappingOffsetIsNeverServedFromASlot, the latter driving wrapping offsets past every slot base the ring could hold, on both the in-flight and ready paths.
The counter was added to measure the cost of exact-offset slot matching: reads a containment test would have served and an exact one would not. Containment landed, and that case became unreachable -- the miss scan and the serve scan call the same PrefetchSlotCovers, so anything covered in one was covered in the other. It kept counting, though, and three comments kept describing the retired meaning. What it finds now is the other way a covered slot goes unserved: the slot is in flight and another reader is already parked on it, one waiter per slot. That makes it a contention counter rather than a coverage one -- two readers on the same file chasing the same chunk, the second paying a full round trip for bytes already on the wire, which is the video-plus-subtitle case worth watching. A small remainder is race: the serve loop drops the lock before the miss scan runs. Comment-only, in the three places that described it: Prefetch.c, Statistics.h and Prefetch.h. The name is left alone deliberately -- it is in the FSCTL surface and a stored baseline, and renaming it would break comparisons against numbers already collected. Each site now says the name is wrong and where to read what it means.
Overflowing the header array is a hard parse failure, not a truncation: picohttpparser answers -1, the same value it gives for a malformed status line, so a perfectly good response is rejected and looks like a broken backend. Nothing in the failure says "too many headers". 16 was reachable by ordinary servers. A plain nginx 206 already spends five or six entries on Date/Server/Content-Type/Content-Length/Connection/ Accept-Ranges, and anything behind a CDN or carrying the usual security and CORS headers passes 16 without trying. 64 entries costs 2 KB of the HTTP_CONTEXT per in-flight request and stops the array being something real responses collide with. It is not a security bound and was never doing that job -- a byte ceiling is what bounds an abusive peer, and it is independent of how many headers fit. Pinned by HttpClientTest.ResponseWithManyHeadersIsStillParsed, confirmed to fail at 16. It checks the delivered body, not just the status, since the point is that such a response is used rather than merely accepted.
The header phase completes on whatever arrives and re-posts for as long as picohttpparser answers "incomplete", growing Ctx->Buffer a page at a time as it goes. Its only ceiling was HttpGrowBufferIfNeeded's MAXULONG, so a peer that streams header bytes and never sends the terminating CRLFCRLF drove close to 4 GB of NonPagedPoolNx per in-flight request -- from a peer that has sent no valid response at all. Non-paged pool exhaustion takes the machine down, not just this driver, which makes it worse than the listing amplification already bounded, that one being PagedPool. HTTP_MAX_HEADER_BYTES is the same untrusted-peer policy as HTTP_MAX_CONTENT_LENGTH from the other side: Content-Length bounds what a peer can make this driver allocate once it has declared a size, this bounds what it can make it allocate by never declaring one. Enforced in HttpParseHeaders, which is the only place the distinction can be drawn -- STATUS_BUFFER_TOO_SMALL is precisely the signal that makes the caller post another receive and grow again, so past the ceiling it answers STATUS_INVALID_NETWORK_RESPONSE and the request fails. That covers the TLS path too: a record-framed response reaches the same parse after every application-data record while BodyOffset is still zero. 64 KB is several times what any real server sends (nginx defaults to 8 KB, IIS to 16 KB), so the ceiling is unreachable by accident. Pinned by HttpClientTest.UnterminatedHeadersAreRejectedRatherThanGrown- WithoutLimit, confirmed to fail without the cap. It floods one endless header line rather than many short ones on purpose: many short ones hit HTTP_MAX_HEADERS and fail as a parse error, which is a different bound that was already there.
GetContentLengthFromHeaders returned at the first match and never looked at the rest, so a response carrying two Content-Length headers was framed by whichever came first. That is the client-side half of response smuggling. This driver pools keep-alive connections, so the bytes it declines to consume do not go away -- they stay in the stream and become the head of the next response read on that socket. If a proxy or origin ahead of it frames the response by the other value, one request's body is served as another request's answer, from a cache that has no way to tell. A response that declares its own length twice is malformed by RFC 9110 whichever value is right, so the scan now covers the whole header set and fails on a second occurrence rather than choosing. Nothing legitimate is lost, and the ambiguity is gone rather than resolved by convention. Pinned by HttpClientTest.DuplicateContentLengthIsRejected, confirmed to fail against the first-match version. It runs both orderings: short-first leaves a tail on the wire, long-first over-reads into whatever follows.
MAXIMUM_ALLOWED was in the first access mask -- the set this volume understands at all -- and missing from the read-only one, so an open carrying it was rejected with STATUS_ACCESS_DENIED. Applications that open with MAXIMUM_ALLOWED as a matter of habit could not get a read handle at all. The bit is not a request for write access; it means "grant whatever I am entitled to", and the entitlement is settled before this check runs. Both devices are FILE_DEVICE_SECURE_OPEN, so the I/O manager resolves MAXIMUM_ALLOWED against the device security descriptor and sets the handle's granted access from that. This check is the second, independent gate, and refusing the bit here denied a handle the caller had already been granted. Pinned by CreateDirectoryTest.MaximumAllowedIsInsideTheReadOnlyMask, confirmed to fail without the change. It covers files and directories separately, since the two masks are maintained apart and an omission in one is invisible from the other -- which is how this one survived.
CheckFileAccess and CheckDirectoryAccess each opened with a verbatim copy of the same sixteen-flag mask -- the set of bits this volume understands at all -- before diverging. That check was unreachable in two senses at once. The IsReadOnly parameter guarding the second, narrower mask was TRUE at all four call sites, and the read-only set is a strict subset of the wider one, so no mask could be rejected by the first test that the second did not reject anyway. Two copies of a list whose only job was to be kept in step with a list that already decided every answer. What is left is the policy itself, stated once: BLORGFS_READ_ONLY_ACCESS, plus BLORGFS_DIRECTORY_CHILD_ACCESS for the three bits a directory adds -- which is the entire reason two predicates exist. The IsReadOnly parameter goes with the dead branch; it was not expressing a choice anything made. Verified as an equivalence, not just a build. CreateDirectoryTest.ReadOnlyAccessMaskIsDecidedBitByBit drives all 32 access bits through a real open against both node types. Both predicates reject on "any bit outside the permitted set", which is monotone in bits, so deciding every single bit correctly decides every combination. It was written and run against the two-mask version first and passed unchanged here; dropping one flag from the set fails it.
Volume teardown runs DestroyWorkQueue -- stop the workers, drain and cancel the queue -- and only then frees the node tree. Freeing a node calls FsRtlUninitializeOplock, which hands every IRP the oplock package still holds to OplockComplete, which queued the granted ones. By then there is no worker left to dispatch them and no drain left to cancel them, so the IRP is stranded and its caller waits on a volume that is gone. Not a race: the sequence is deterministic, and it is teardown itself that triggers the late releases. OplockComplete now reads the ThreadsActive gate that FsdPostRequest and FsdRequeueRequest already consult before queueing. Those two can return STATUS_DEVICE_REMOVED and leave the completion to their caller; a callback has no return path, so it completes the IRP itself with the status its siblings hand back for this condition. The failure arm is unchanged -- a failed break already completed the IRP, so it was the granted break, the normal outcome, that had the bug. Reversing the teardown order instead would run oplock-released IRPs through live workers against a half-torn-down volume, which is worse. Two tests, both in FspWorkQueueStressTest: the teardown case was confirmed to fail without the gate, and the live-queue case pins that this is a teardown check rather than a new policy for oplock breaks.
WSK takes a socket's address family from the LOCAL address handed to WskSocketConnect, not from the remote. This one was a fixed AF_INET wildcard, so an AF_INET6 backend could never be reached: DriverEntry resolves AF_UNSPEC, the pool's address comparison handles AF_INET6, the connect context sizes itself for either -- and then the connect asked for an IPv4 socket regardless. A zeroed SOCKADDR_STORAGE carrying only the family is the wildcard for both: INADDR_ANY and in6addr_any are all-zero, as are port, flowinfo and scope id. So this drops the per-family branch rather than adding one. WskModel now records the local and remote families of each connect, which is the observable that was missing -- the model discarded both parameters, so no test could see a socket opened in the wrong family. SocketKernelTest.LocalBindFamilyFollowsTheRemoteAddress drives both families (pinning only v6 would let a hardcoded AF_INET6 pass) and was confirmed to fail against the fixed-AF_INET version.
FileFsAttributeInformation set FILE_CASE_SENSITIVE_SEARCH while every name comparison in the driver is case-insensitive: RtlEqualUnicodeString with CaseInSensitive = TRUE in Create.c, PathCache.c and Structs.c, over hashes built from upcased characters. The volume also contradicted itself -- FileCaseSensitiveInformation already reports no flags at all, so the two query classes gave opposite answers about the same volume. The cost of the lie falls on whoever believes it. An application that trusts the flag and stops normalizing case gets aliasing it never asked for: two names it treats as distinct open the same file. Games assume the Windows default, which is the behaviour this volume actually has. FILE_CASE_PRESERVED_NAMES stays -- names are still returned with their original case, which is a separate and true claim. Pinned by FileInfoTest.FsAttributeInformationDoesNotClaimCaseSensitiveSearch, confirmed to fail with the flag restored.
A flush on this volume has nothing to write back -- it is read-only, so nothing is ever dirty -- and the handler answered STATUS_INVALID_DEVICE_REQUEST anyway. That reaches an application as "Incorrect function", the same misleading error that hid the statistics IOCTL routing bug, and some applications treat a failed flush as fatal rather than as "this volume needs no flushing". Most filesystems return success on a read-only volume for exactly that reason. Success here is the honest answer, not a stub standing in for work: there is no flushing a later change would have to remember to add. The three-armed switch went with it. Every arm was empty and fell through to the same status, so it described a distinction that did not exist. What survives is the one that does: a device this driver does not own is still refused, because a flush arriving there is a routing error and swallowing it would hide one. Pinned by DispatchSandbox.FlushBuffersSucceedsOnEveryDeviceThisDriverOwns, which drives all three device objects and keeps the foreign device as its control. Confirmed to fail against the old status.
FileNetworkOpenInformation filled both AllocationSize and EndOfFile from Header.AllocationSize, so it and FileStandardInformation could answer the same question about the same file differently. It happens to be invisible today only because BlorgCreateFCB sets FileSize and AllocationSize equal -- nothing enforces that, and the first time they diverge the wrong answer lands on FileNetworkOpenInformation, which is the fast path the loader and Explorer take. BlorgTrimReadToFileSize's two traces had the same slip: both print "file size = ..." from AllocationSize while the comparison beside them reads FileSize. A trace that names one field and prints another is worse than no trace when the two stop agreeing, which is exactly the case anyone would be reading it for. The existing size assertions could not have caught either, since both fields hold the same value in the fixture. FileInfoTest.EndOfFileComesFromFileSizeNotAllocationSize drives them apart first, then queries both classes against the same FCB and requires them to agree with each other and with FileSize -- while still reporting the allocation size as the allocation size. Confirmed to fail before the fix.
UrlEncodeUnicodeString built a UNICODE_STRING that HttpBuildRequest then handed to RtlStringCbPrintfA as %wZ, so every character was widened here and narrowed again there. The output is ASCII by construction -- an unreserved byte passes through, anything else becomes '%' plus two hex digits -- so the trip through UTF-16 bought nothing. Emitting ANSI directly halves the allocation, drops a conversion pass per request, and removes a %wZ from a driver that otherwise takes trouble to keep clear of that specifier. It also lets the send-buffer budget count the path's exact byte length rather than twice it. HttpBuildRequest stays PASSIVE_LEVEL only, but now for one reason instead of two: UrlEncodePathToAnsi's RtlUnicodeStringToUTF8String is paged code, which is the reason Prefetch.h's issuance rule already names. This is a refactor, so the test does the opposite of the usual job -- it pins behaviour that must NOT change. HttpClientTest.NonAsciiPathIsPercentEncodedFromItsUtf8Bytes asserts the exact bytes on the wire for a path of two-byte characters, where a per-byte encoder and a per-character one visibly disagree. It was run against the old encoder as well as the new one and passes on both, which is what says the output did not move; a swapped-nibble mutation fails it and RequestLineAndRangeAreWellFormed together, which is what says it is not vacuous.
WskRegistration and WskProviderNpi had external linkage while nothing outside Socket.c refers to them -- verified across src/ and tests/. The convention is that a file-scope name with external linkage is either prefixed and declared in a header, or lives in `global` (Driver.h); these were neither, so they published two symbols nobody was permitted to use. SocketTlsRecvCapacity, a few lines below, stays external on purpose: it is declared in Socket.h and read by Client.c, which is what the rule allows.
TlsHandshake.c held the only unbraced ifs in the tree -- 31 of them, all the same line: `if (NT_SUCCESS(status)) status = Something(...);`. Zero elsewhere in src/, so this was one file's local habit rather than drift. Bracing them would have satisfied the rule and quadrupled the line count of a section whose whole value is that the sequence reads as a sequence: derive, extract, expand, hash, import, each step consuming the last one's output. TLS_CHAIN_STEP names the idiom once and removes the branch rather than formatting it, so the chain reads as the list of derivations it is and the short-circuit lives in one place where it can be checked. Purely mechanical otherwise: the diff is the macro, its comment, and 31 one-for-one substitutions -- nothing else changed. Verified as an equivalence. The RFC 8448 vectors, the live openssl handshake, and TlsHandshakeKernelTest all pass unchanged, and neutering the macro so every step is skipped breaks the driver build and fails three kernel-handshake tests -- so the chain those tests exercise is the one this touched.
A malicious or compromised backend could make the driver attempt a multi-gigabyte PagedPool allocation from one 64 MB response. The counts come off the wire. flatcc's verifier bounds them to the buffer, so they cannot be nonsense -- but it does not stop them being amplified. A flatbuffers vector of tables is a length followed by 4-byte offsets, and nothing requires those offsets to be distinct, so 16.7M of them can point at one minimal table inside a 64 MB body. Each expands to a DIRECTORY_FILE_METADATA, which carries an inline WCHAR Name[260] and costs 560 bytes: 8.8 GB requested, a ~140x amplification. Even assuming every entry needs its own 16-byte table it is still ~2.2 GB. HTTP_MAX_CONTENT_LENGTH does not cover this. It bounds the input, not what the input is inflated into. The allocation now goes through HttpCheckedAddSizeT, which this file's own header comment already says every size computation combining a wire-parsed length must use, and which this one did not. That is defence in depth rather than the fix -- with a 64-bit SIZE_T the multiply does not actually wrap at these magnitudes; the count bound is what makes it safe. Also extends ClientFuzz to drive BlorgHttpGetDirectoryInfo, not only a ranged read. The listing is the one response body this driver PARSES rather than copies -- a flatbuffer decoded in kernel mode, carrying server-supplied names, sizes and counts -- and it had no malformed-input coverage at all, which is why none of the above had been noticed. 200k iterations clean. Worth knowing about that coverage. ClientFuzz carries its own embedded HTTP seeds, but not one of them holds a flatbuffer body, and mutating an HTTP response essentially never produces a buffer flatcc's verifier accepts. So the parser is now REACHED, which it was not before, but most iterations stop at the verifier and the post-verify logic -- entry counts, name conversion, the bound added here -- is only shallowly exercised. Adding a valid listing to the seed list would make this considerably sharper, and is the obvious next step.
All three said something the driver does not do, which is worse than silence -- a reader who trusts them "fixes" correct code. The %wZ rule banned the specifier "at <= DISPATCH_LEVEL", which literally includes PASSIVE and therefore forbade every use in the tree. The hazard is running above PASSIVE, and practice was already right: %wZ is used freely on PASSIVE-only dispatch paths and kept off completion chains. Reworded to say that. The ProbeForRead rule had no carve-out for the two places that deliberately do not probe -- the cached read path in Read.c and Security.c's SeQuerySecurityDescriptorInfo -- both of which follow fastfat and catch the fault rather than preventing it. Left unstated, both look like oversights waiting to be corrected. The surviving KdBreakPoint in BlorgVolumeCreate had its reasoning recorded in two places that a reader greping for the call would not find. It now lives in that function's own header comment, where the comment rule allows it and where landing on line 1125 puts it. deploy/DEBUGGING.md also named the wrong function for it (BlorgCreate).
BlorgFS.inf seeded Parameters\RemoteHost with "blorgfs.blorg.lan" while Driver.c's BLORGFS_DEFAULT_REMOTE_HOST is 10.0.50.17, and the INF's own comment asserted the two were the same value. They were not, and the comment is why nobody checked. That is not only documentation drift. The INF always writes the value, so the driver's fallback never applies on an INF install -- meaning an install without an explicit -RemoteHost pointed at a name nothing resolves, while a bare driver load reached the real backend. Two install paths, two different answers, one of them untested. Both now say 10.0.50.17, and each side carries a note that the other exists and must be kept in step, since nothing mechanical can enforce it across an INF and a C macro. Baking a lab address into the INF is not lovely, but the driver already compiles that address in as its default -- the choice was never "address or no address", only whether the two agreed.
The workflow is schedule-only, and scheduled workflows run only on the default branch. So the one change that decides what CodeQL reports -- a query-pack pin in .github/codeql/codeql-config.yml -- could only ever be merged unrun, with the first evidence that it builds, resolves and reports arriving the following Saturday, on master. Two triggers fix that. A pull request touching this workflow or .github/codeql/ runs the analysis it is changing, which is precisely when a run is worth paying for. workflow_dispatch covers re-scans on demand -- after a pack bump, or after a batch of fixes -- instead of a seven-day wait. Neither makes CodeQL a per-push gate. PREfast already runs on every build.yml push and PR, which is why this was weekly to begin with. README gains a CI section, since when each of the three workflows runs, and why they are split that way, was previously only discoverable by reading three YAML files.
Pinned at 1.1.0 and well behind. Pinning itself is right -- a pack release must not silently change what a scheduled scan reports -- but a pin still has to be reviewed rather than frozen, and this is the pack carrying the driver-specific IRQL and annotation queries, so a stale one is lost coverage on exactly the class of bug hardest to find any other way. Ordered after the trigger change on purpose: with it in place, the pull request carrying this bump runs the analysis under the new pack rather than merging it unverified. The previous triage (0 IRQL findings, 31 own-code hits all dismissed as false positives with reasons) will need redoing against the new queries rather than assuming those verdicts still hold.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // IO_CSQ insert callback: appends Irp to the tail of the pending-IRP queue. | ||
| VOID FspCsqInsertIrp(IO_CSQ* Csq, PIRP Irp) | ||
| VOID BlorgFspCsqInsertIrp(IO_CSQ* Csq, PIRP Irp) |
|
|
||
| _IRQL_raises_(DISPATCH_LEVEL) | ||
| VOID FspCsqAcquireLock(IO_CSQ* Csq, _At_(*Irql, _IRQL_saves_) PKIRQL Irql) | ||
| VOID BlorgFspCsqAcquireLock(IO_CSQ* Csq, _At_(*Irql, _IRQL_saves_) PKIRQL Irql) |
|
|
||
| _IRQL_requires_(DISPATCH_LEVEL) | ||
| VOID FspCsqReleaseLock(IO_CSQ* Csq, _IRQL_restores_ KIRQL Irql) | ||
| VOID BlorgFspCsqReleaseLock(IO_CSQ* Csq, _IRQL_restores_ KIRQL Irql) |
|
|
||
| NTSTATUS FsdRequeueRequest( | ||
| IN PIRP Irp | ||
| NTSTATUS BlorgFsdRequeueRequest( |
| // turn, so no two locks are ever held together. | ||
| // | ||
| VOID PathCacheInvalidatePrefix(const UNICODE_STRING* Dir) | ||
| VOID BlorgPathCacheInvalidatePrefix(const UNICODE_STRING* Dir) |
| // rather than the synchronous CloseWskSocket. | ||
| // | ||
| NTSTATUS ReleaseReusableWskSocket(PKSOCKET Socket) | ||
| NTSTATUS BlorgReleaseReusableWskSocket(PKSOCKET Socket) |
| // as are port, flowinfo and scope id -- so there is no per-family branch to | ||
| // keep in step. | ||
| // | ||
| NTSTATUS BlorgAcquireReusableWskSocketAsync( |
| // all-zero Value never matching a real SHA-256 digest. | ||
| // | ||
| BOOLEAN TlsCheckPin(const UCHAR* Spki, ULONG SpkiLen) | ||
| BOOLEAN BlorgTlsCheckPin(const UCHAR* Spki, ULONG SpkiLen) |
| // for the very first record. | ||
| // | ||
| VOID TlsStartHandshakeAsync( | ||
| VOID BlorgTlsStartHandshakeAsync( |
|
|
||
| if (NT_SUCCESS(status)) status = TlsDecodeP256SubjectPublicKeyInfo(spki, spkiLen, Ctx->ServerLongTermKey); | ||
| if (NT_SUCCESS(status)) status = TlsSha256(Ctx->Transcript, Ctx->TranscriptLen, Ctx->TranscriptHashThroughCert); | ||
| TLS_CHAIN_STEP(status, BlorgTlsDecodeP256SubjectPublicKeyInfo(spki, spkiLen, Ctx->ServerLongTermKey)); |
Chuccle
force-pushed
the
audit/hardening-pass
branch
from
August 23, 2026 04:49
6368b18 to
5ed017b
Compare
BlorgInitialiseHttpClient ran after CreateBlorgFileSystemDeviceObject,
which leaves a window in which SocketPool and WskProviderNpi are nothing
but their zero-initialised storage while the driver is already reachable.
The mechanism is IoRegisterFileSystem, not DO_DEVICE_INITIALIZING. An
ordinary device object receives nothing until the load completes, so
clearing that flag mid-DriverEntry opens nothing on its own. Registration
is different: it puts this FSD on the I/O manager's list there and then,
and every arriving volume is offered to every registered filesystem from
another thread, so a mount can land while DriverEntry is still running.
This driver already knew that -- BlorgMountVolume is written to survive it
and says so, naming "the window DriverEntry opens between
IoRegisterFileSystem and the DDO existing". A mount reaches
BlorgCreateVolumeDeviceObject and from there the read path.
Zeroed is not harmless. IsListEmpty compares Flink against the list head,
and a zeroed head has Flink NULL, so the pool reads as NOT empty and the
next acquire runs RemoveHeadList on a NULL Flink. WskProviderNpi.Dispatch
is NULL across the same window.
Every early return between the client coming up and DriverEntry succeeding
now tears it back down, which the old ordering did not have to think about.
Found by the windows-drivers pack at 1.10.0, which is the first thing the
bump has paid for. It reported ten uses of that rule; the other eight are
false positives with concrete reasons, recorded so the next triage does not
redo the work: BlorgPathCacheInit and BlorgTlsHandshakeGlobalInit already
run at the top of DriverEntry, and FspQueue cannot be reached through a CSQ
callback before BlorgCreateWorkQueue has run IoCsqInitialize.
Also settles the three workflow warnings CodeQL prints on every run:
- on.push for master. Code scanning only surfaces default-branch alerts
from a push analysis, so without it the weekly run uploaded results that
never appeared anywhere a person would look.
- ilammy/msvc-dev-cmd pinned to a SHA rather than the mutable v1 tag,
which is what actions/unpinned-tag was flagging and also answers the
Node 20 deprecation notice.
- cpp/commented-out-code, cpp/poorly-documented-function and
cpp/long-switch excluded. Between them they produced 57 permanent alerts
against code written the way this project intends -- the first reads the
prose comment above a function as dead code, and the second measures
comments inside a function body and wants 2% where the rule here is
zero. A list nobody can action is a list nobody reads. Nothing
security-tagged is excluded and nothing from the windows-drivers pack.
The remaining "cannot build an overlay database" notice is informational:
build-mode manual is correct for a driver that needs a real build.
Chuccle
force-pushed
the
audit/hardening-pass
branch
from
August 23, 2026 04:54
5ed017b to
ef4f301
Compare
Chuccle
changed the title
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
2 participants
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.
Two independent audits, the conventions pass, and the CI/tooling breakage
found while verifying them. 28 commits, each one building and passing the
full Fast tier on its own.
How this was verified
Every fix has a test that was confirmed to fail against a deliberately
reverted version before the fix went back in. A green test that was never
seen red proves nothing, and this branch has 20-odd of them.
For the three refactors (URL encoding, the TLS chain macro, the access-mask
collapse) the test does the opposite job: it was written and run against the
old code first and had to pass there too, which is what makes it an
equivalence check rather than a regression test.
Beyond per-commit:
-Tier Fastindividually, swept in anisolated worktree. The history is bisectable, not merely tidy.
size, hash, range, reread and EOF-straddling reads over files up to
19.6 GB, driver loaded in the VM.
git diffandgit diff -wproduce identical counts, andgit diff --checkis clean.Ordering
The rename touches all 55 files, so everything else sits on top of it. The
first three commits are CI and tooling fixes that the rest depend on for a
green run.
Security and correctness
buffer + 0xFFFFF000-- a kernel OOB read at a caller-influenced displacement. Fixed at the dispatch boundary and in the arithmetic, because the copy's safety should not depend on a check elsewhere.FsRtlUninitializeOplockhanded IRPs to a queue with no workers and no drain left. Deterministic, not a race.MAXIMUM_ALLOWEDopensFILE_CASE_SENSITIVE_SEARCHwhile comparing case-insensitively, and contradicted its ownFileCaseSensitiveInformation.IRP_MJ_FLUSH_BUFFERSFileNetworkOpenInformationreported allocation size as EOF, disagreeing withFileStandardInformation.CI and tooling, all found by running things rather than reading them
across seven projects, and mixed C/C++ sources compiled in separate
CL.EXEinvocations within each. Under/mthat isC1041. It had beenfailing every scheduled CodeQL run on master and every
Deploy-ToVM.ps1build from a cold tree.
merged unrun. It now runs on any PR touching its own config.
TlsFuzzTesthad no ASan-runtime copy target and only worked byborrowing another project's DLL out of the shared solution output. A
per-project build left it exiting
0xC0000135.Deploy-ToVM.ps1trusted PATH for MSBuild; PATH may hold the 32-bitone, which silently drops PREfast. Both callers now share one locator.
deploy/blorgfs.env(gitignored) lets the deploy pipeline configureitself, so a session starting cold can deploy with no arguments.
Conventions
Blorgnow means external linkage and nothing else -- 86 functions, 934occurrences -- so a bare module name at file scope is a promise the symbol
is
static. Plus: the deadUtil.cdeleted,IN/OUTSAL remnantsdropped, input parameters const-qualified (which immediately caught two
sandbox stand-ins that had drifted from the header they include), the 31
unbraced
ifs inTlsHandshake.ccollapsed onto a named idiom, and theduplicated access-mask predicate reduced to one permitted set.
Three convention statements that contradicted the code are corrected, since
a reader who trusts those "fixes" correct code.
Not included, deliberately
The read-only mask granting DELETE/WRITE_EA/WRITE_ATTRIBUTES is entangled
with the open write-policy decision, which is still open. Fixing the mask
would commit to an answer there.