From 092c284ee3cdca444530a09ad526c4db16e6117e Mon Sep 17 00:00:00 2001 From: Alessandro Sacilotto Date: Mon, 24 Aug 2026 19:53:50 +0200 Subject: [PATCH] Security hardening pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings and rationale from the security review (plan: analyze-this- project-from-jolly-fountain): - Supply chain (High): the vcpkg checkout was cloned at master HEAD and auto-`git pull`ed on every configure, executing unpinned upstream code at configure time. The local .vcpkg clone is now pinned (detached HEAD) to VCPKG_PINNED_COMMIT — deliberately the same commit as vcpkg.json's builtin-baseline, so tool, scripts, and port baseline come from one audited revision. A user-set VCPKG_ROOT checkout is treated as user-managed and gets no git operations. - Worker-process DoS (High): the adapters allocate per request, so std::bad_alloc could unwind through OnBeginRequest into the IIS pipeline and crash w3wp.exe. OnBeginRequest now has a try/catch(...) boundary returning RQ_NOTIFICATION_CONTINUE (fail-open is a documented, deliberate tradeoff for this demo rule). - CI supply chain (Medium): all actions pinned by commit SHA (checkout v4, cache v4, get-cmake v4.4.2), cmakeVersion pinned to 4.4.2, and a Dependabot config added so the pins receive update PRs. - Binary hardening (Medium): /guard:cf (compile+link, cl and clang-cl) and /sdl (MSVC only) applied globally so every TU linked into the network-facing DLL is instrumented; verified CF-instrumented via dumpbin /loadconfig. - Hygiene (Low): .gitgnore renamed to .gitignore so the ignore rules actually apply, and the machine-specific .vscode/ untracked accordingly; register_module.cpp no longer leaks the factory when SetRequestNotifications fails; ModuleHandler's suffix match carries a SECURITY NOTE marking it demo-only (not an ACL primitive). CLAUDE.md documents the vcpkg bump procedure (baseline + pin move together), the no-throw boundary contract, and the SHA-pinning convention for new actions. Co-Authored-By: Claude Fable 5 --- .github/dependabot.yml | 8 ++++ .github/workflows/ci.yml | 17 ++++--- .gitgnore => .gitignore | 0 .vscode/c_cpp_properties.json | 22 ---------- CLAUDE.md | 4 +- CMakeLists.txt | 66 +++++++++++++++++++++++++--- src/core/ModuleHandler.cpp | 7 +++ src/module/IISRequestLevelModule.cpp | 31 ++++++++----- src/module/register_module.cpp | 9 +++- 9 files changed, 117 insertions(+), 47 deletions(-) create mode 100644 .github/dependabot.yml rename .gitgnore => .gitignore (100%) delete mode 100644 .vscode/c_cpp_properties.json diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..43789fd --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,8 @@ +version: 2 +updates: + # Keeps the SHA-pinned actions in .github/workflows current: Dependabot + # opens PRs bumping the pins so they don't silently rot. + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a2c279e..c1bc70b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,8 +22,9 @@ jobs: # Note: an earlier attempt used ilammy/msvc-dev-cmd + Ninja to sidestep # generator-version matching entirely, but that action's environment # activation sets its own VCPKG_ROOT pointing at VS's bundled (non-git) - # vcpkg copy, which this repo's CMakeLists.txt then tries to `git pull` - # and fails. The VS generator doesn't need a pre-activated dev + # vcpkg copy — a user-managed checkout this repo's CMakeLists.txt + # neither pins nor updates, so builds there wouldn't use the audited + # pinned revision. The VS generator doesn't need a pre-activated dev # environment, so it doesn't hit that. # # Also: -DCMAKE_TOOLCHAIN_FILE can't point directly at @@ -51,19 +52,23 @@ jobs: cxx: clang-cl toolset: "-T ClangCL" + # Third-party actions are pinned by commit SHA (tags are mutable, so + # a compromised action author could otherwise run arbitrary code in + # this workflow); Dependabot (.github/dependabot.yml) proposes pin + # updates. steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 # cmake_minimum_required(VERSION 4.2) is newer than what's preinstalled # on GitHub-hosted runners. - name: Install CMake >= 4.2 - uses: lukka/get-cmake@latest + uses: lukka/get-cmake@fffaaafeea488556c2c12dad60690008bc1caacb # v4.4.2 with: - cmakeVersion: "latest" + cmakeVersion: "4.4.2" - name: Cache vcpkg - uses: actions/cache@v4 + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4 with: path: | .vcpkg diff --git a/.gitgnore b/.gitignore similarity index 100% rename from .gitgnore rename to .gitignore diff --git a/.vscode/c_cpp_properties.json b/.vscode/c_cpp_properties.json deleted file mode 100644 index 40a7621..0000000 --- a/.vscode/c_cpp_properties.json +++ /dev/null @@ -1,22 +0,0 @@ -{ - "configurations": [ - { - "name": "Win32", - "includePath": [ - "${workspaceFolder}/**", - "G:\\CppProjects\\_Base\\0a8ede4\\afec2cd\\c2ff296\\Install\\include" - ], - "defines": [ - "_DEBUG", - "UNICODE", - "_UNICODE" - ], - "windowsSdkVersion": "10.0.19041.0", - "compilerPath": "C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\BuildTools\\VC\\Tools\\MSVC\\14.29.30133\\bin\\Hostx64\\x64\\cl.exe", - "cStandard": "c17", - "cppStandard": "c++17", - "intelliSenseMode": "windows-msvc-x64" - } - ], - "version": 4 -} \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index 70c0cd4..7d1a7cc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ A native HTTP module for IIS to experiment with new ways of unit testing. C++20, ## Build system -Dependencies are managed via **vcpkg**, bootstrapped automatically by the top-level `CMakeLists.txt`: if `VCPKG_ROOT` is unset, it clones vcpkg into `.vcpkg/` and bootstraps it before `project()` is even declared; the dependencies declared in `vcpkg.json` (GTest, fmt, spdlog) are then installed by vcpkg's manifest-mode resolution during configure. This means the *first* configure can take a while and requires network/git access; subsequent configures just auto-update the vcpkg checkout via `git pull`. +Dependencies are managed via **vcpkg**, bootstrapped automatically by the top-level `CMakeLists.txt`: if `VCPKG_ROOT` is unset, it clones vcpkg into `.vcpkg/`, **pins the checkout to `VCPKG_PINNED_COMMIT`** (deliberately the same commit as the `builtin-baseline` in `vcpkg.json`, so tool and ports come from one audited revision — a supply-chain measure; never reintroduce an auto-`git pull` here), and bootstraps it before `project()` is even declared; the dependencies declared in `vcpkg.json` (GTest, fmt, spdlog) are then installed by vcpkg's manifest-mode resolution during configure. The *first* configure can take a while and requires network/git access; later configures only re-checkout if the pin changed. **To bump vcpkg**: update the `builtin-baseline` in `vcpkg.json` AND `VCPKG_PINNED_COMMIT` in `CMakeLists.txt` to the same new commit, then re-configure. A user-set `VCPKG_ROOT` env checkout is treated as user-managed: no git operations are performed on it. Building uses **CMake presets** (`CMakePresets.json`), targeting Visual Studio 2026 (`vs2026` generator) on Windows only — this project builds on Windows exclusively. Presets combine an architecture with a configuration: @@ -78,3 +78,5 @@ Report lands at `_build\windows-x86-64-coverage\coverage-report\index.html`. - Minimum CMake version required is 4.2 (unusually new — verify your local CMake before assuming a configure failure is a code issue). CI installs `cmake >= 4.2` explicitly since GitHub-hosted runners don't have it preinstalled. - The toolchain pins Visual Studio 2026 (`Visual Studio 18 2026`); local build presets additionally pin an exact MSVC toolset version (`14.44.35207` for build presets, `host=x64,version=14.50` for the static-analysis configure preset) — build failures on other VS/MSVC versions are expected without adjusting `CMakePresets.json`. CI sidesteps this (see above). - `vcpkg.json` still declares `fmt` and `spdlog` (and the top-level `CMakeLists.txt` still `find_package`s them), but no target links them any more — they are kept available for future logging/formatting work. +- All targets compile with `/guard:cf` (Control Flow Guard, also passed at link) and, under MSVC, `/sdl` — hardening for a DLL that runs network-facing inside w3wp.exe. `IISRequestLevelModule::OnBeginRequest` wraps its body in `try/catch(...)`: IIS notification methods must never let a C++ exception escape (it would crash the worker process), so keep that boundary when adding new notifications. +- CI actions in `.github/workflows/ci.yml` are pinned by commit SHA (supply-chain); Dependabot (`.github/dependabot.yml`) proposes pin bumps. When adding an action, pin it by SHA with a `# vX` comment. diff --git a/CMakeLists.txt b/CMakeLists.txt index ddb931e..6f1d5dd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,26 +1,67 @@ cmake_minimum_required(VERSION 4.2) -# if the environment variable VCPKG_ROOT is not set -# default to the local .vcpkg directory and clone vcpkg if it doesn't exist +# vcpkg checkout pin: the same commit as the builtin-baseline in +# vcpkg.json, so the tool, its scripts, and the port baseline all come +# from one audited revision (supply-chain: never execute unpinned, +# auto-updated code at configure time). To bump vcpkg, update BOTH the +# baseline in vcpkg.json and this pin to the new commit, then wipe +# .vcpkg/ (or let the checkout below fetch it). +set(VCPKG_PINNED_COMMIT "d92484ed3c5020c6679d095ad3e5add907887b62") + +# If the environment variable VCPKG_ROOT is set, that checkout is +# user-managed: no git operations are performed on it (only the +# bootstrap-if-missing below). Otherwise a local .vcpkg clone is +# created and pinned to VCPKG_PINNED_COMMIT. if(NOT DEFINED ENV{VCPKG_ROOT}) set(VCPKG_ROOT ${CMAKE_CURRENT_SOURCE_DIR}/.vcpkg) + set(VCPKG_CHECKOUT_IS_MANAGED TRUE) else() set(VCPKG_ROOT $ENV{VCPKG_ROOT}) + set(VCPKG_CHECKOUT_IS_MANAGED FALSE) endif() if(NOT EXISTS ${VCPKG_ROOT}) + if(NOT VCPKG_CHECKOUT_IS_MANAGED) + message(FATAL_ERROR "***** FATAL ERROR: VCPKG_ROOT points at a non-existing path: ${VCPKG_ROOT} *****") + endif() message("Cloning vcpkg in ${VCPKG_ROOT}") execute_process(COMMAND git clone https://github.com/Microsoft/vcpkg.git ${VCPKG_ROOT}) -else() - # The following command has no effect if the vcpkg repository is in a detached head state. - message("Auto-updating vcpkg in ${VCPKG_ROOT}") - execute_process(COMMAND git pull WORKING_DIRECTORY ${VCPKG_ROOT}) endif() if(NOT EXISTS ${VCPKG_ROOT}/README.md) message(FATAL_ERROR "***** FATAL ERROR: Could not clone vcpkg *****") endif() +if(VCPKG_CHECKOUT_IS_MANAGED) + execute_process( + COMMAND git rev-parse HEAD + WORKING_DIRECTORY ${VCPKG_ROOT} + OUTPUT_VARIABLE VCPKG_CURRENT_COMMIT + OUTPUT_STRIP_TRAILING_WHITESPACE + ) + if(NOT VCPKG_CURRENT_COMMIT STREQUAL VCPKG_PINNED_COMMIT) + message("Pinning vcpkg to ${VCPKG_PINNED_COMMIT}") + execute_process( + COMMAND git -c advice.detachedHead=false checkout ${VCPKG_PINNED_COMMIT} + WORKING_DIRECTORY ${VCPKG_ROOT} + RESULT_VARIABLE VCPKG_CHECKOUT_RESULT + ) + if(NOT VCPKG_CHECKOUT_RESULT EQUAL 0) + # Commit not present locally (e.g. the pin moved forward + # since the clone): fetch it explicitly, then re-checkout. + execute_process(COMMAND git fetch origin ${VCPKG_PINNED_COMMIT} WORKING_DIRECTORY ${VCPKG_ROOT}) + execute_process( + COMMAND git -c advice.detachedHead=false checkout ${VCPKG_PINNED_COMMIT} + WORKING_DIRECTORY ${VCPKG_ROOT} + RESULT_VARIABLE VCPKG_CHECKOUT_RESULT + ) + endif() + if(NOT VCPKG_CHECKOUT_RESULT EQUAL 0) + message(FATAL_ERROR "***** FATAL ERROR: Could not pin vcpkg to ${VCPKG_PINNED_COMMIT} *****") + endif() + endif() +endif() + set(VCPKG_EXEC ${VCPKG_ROOT}/vcpkg.exe) set(VCPKG_BOOTSTRAP ${VCPKG_ROOT}/bootstrap-vcpkg.bat) @@ -55,6 +96,19 @@ set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED YES) set(CMAKE_CXX_EXTENSIONS OFF) +# --- Binary hardening --- +# The module DLL is network-facing native code inside w3wp.exe, so it +# gets Control Flow Guard (compile+link; supported by both cl and +# clang-cl) and /sdl (extra compile-time and runtime security checks; +# MSVC-only — clang-cl doesn't implement it). Applied globally so every +# translation unit linked into the DLL is instrumented; /GS, ASLR, and +# DEP are already MSVC defaults. +add_compile_options( + "$<$:/sdl>" + "/guard:cf" +) +add_link_options("/GUARD:CF") + # --- Coverage flags (applied only when ENABLE_COVERAGE is ON) --- option(ENABLE_COVERAGE "Enable code coverage instrumentation" OFF) diff --git a/src/core/ModuleHandler.cpp b/src/core/ModuleHandler.cpp index 5ec448f..1a14e9d 100644 --- a/src/core/ModuleHandler.cpp +++ b/src/core/ModuleHandler.cpp @@ -20,6 +20,13 @@ namespace { // Case-insensitive suffix check on the cooked URL path, which // carries the path without the query string. + // + // SECURITY NOTE: this is demo logic, not an access-control + // mechanism. A suffix-only match doesn't block a subtree + // ("/forbidden/", "/forbidden/x" pass), and std::towlower is + // locale-dependent (invariant ASCII folding is what a security + // comparison would need). Replace the matching semantics before + // building any real ACL on this. auto EndsWithForbidden(const RawHttpRequest * rawRequest) -> bool { if (rawRequest == nullptr) { return false; diff --git a/src/module/IISRequestLevelModule.cpp b/src/module/IISRequestLevelModule.cpp index 8c605ff..1a71a83 100644 --- a/src/module/IISRequestLevelModule.cpp +++ b/src/module/IISRequestLevelModule.cpp @@ -14,16 +14,27 @@ auto IISRequestLevelModule::OnBeginRequest( return RQ_NOTIFICATION_CONTINUE; } - IISContext context{pHttpContext}; - IISEventProvider provider{pProvider}; - - const core::Verdict verdict = module_->OnBeginRequest( - &context, - pProvider != nullptr ? &provider : nullptr); - - return verdict == core::Verdict::Finish - ? RQ_NOTIFICATION_FINISH_REQUEST - : RQ_NOTIFICATION_CONTINUE; + // IIS notification methods must not throw: an escaping C++ exception + // unwinds into the native pipeline and crashes the worker process + // (the adapters allocate, so std::bad_alloc is possible under memory + // pressure). Failing open (Continue) is a deliberate tradeoff for + // this demo rule: a dropped forbidden-check beats taking down the + // whole app pool. catch(...) under /EHsc catches C++ exceptions + // only, not SEH. + try { + IISContext context{pHttpContext}; + IISEventProvider provider{pProvider}; + + const core::Verdict verdict = module_->OnBeginRequest( + &context, + pProvider != nullptr ? &provider : nullptr); + + return verdict == core::Verdict::Finish + ? RQ_NOTIFICATION_FINISH_REQUEST + : RQ_NOTIFICATION_CONTINUE; + } catch (...) { + return RQ_NOTIFICATION_CONTINUE; + } } auto IISRequestLevelModule::Dispose() -> VOID { diff --git a/src/module/register_module.cpp b/src/module/register_module.cpp index 38abad3..f835c1f 100644 --- a/src/module/register_module.cpp +++ b/src/module/register_module.cpp @@ -18,6 +18,11 @@ HRESULT WINAPI RegisterModule( return HRESULT_FROM_WIN32(ERROR_NOT_ENOUGH_MEMORY); } // IISRequestLevelModule only implements OnBeginRequest, so that is - // the single request notification we subscribe to. - return pModuleInfo->SetRequestNotifications(pFactory, RQ_BEGIN_REQUEST, 0); + // the single request notification we subscribe to. IIS takes + // ownership of the factory only on success. + const HRESULT hr = pModuleInfo->SetRequestNotifications(pFactory, RQ_BEGIN_REQUEST, 0); + if (FAILED(hr)) { + delete pFactory; + } + return hr; }