release: binskim for Windows#924
Merged
Merged
Conversation
The 1ES PT Windows build job did not run binskim against the binaries
we ship. By default the template would point binskim at the published
artifact (the Inno Setup installer and the 7z self-extracting portable
.exe), neither of which binskim can crack open to find the PE files
inside, so any findings on those wrappers are also unactionable: they
are produced by external tools we do not control.
Opt the job into binskim explicitly and aim it at the actual product
binaries instead. Stage only the first-party pacman packages that
`please.sh build-mingw-w64-git` emits --
mingw-w64-<toolchain>-{git,git-credential-wincred,git-pdb}-*.pkg.tar.xz
-- into _bin/<mingwprefix>/ and scope the analyzer to the .exe/.dll
files in that tree. By construction those packages carry only the
binaries this repo's Makefile builds (git.exe, the dashed subcommands,
scalar.exe, headless-git.exe, git-gvfs-helper.exe,
git-credential-wincred.exe, ...) plus their cv2pdb-generated .pdbs,
so a broad **/*.{exe,dll} glob is safe.
Excluding everything else keeps the full Git for Windows installer
payload out of the scan: MSYS2/MinGW runtime, Perl, Tcl/Tk,
libcurl/libssl/libssh2, Git Credential Manager, Git LFS, tig, and the
build-extra git-wrapper launcher shims are all third-party content we
cannot fix from this repo.
Assisted-by: Claude Opus 4.7
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
BinSkim flags toolchain-rooted issues on every release build that we cannot fix from this repo: BA2008 (Control Flow Guard) and BA2012 (stack cookie not locatable) on every clangarm64 binary, and BA2025 (CET shadow stack) on every mingw64 binary. CFG and CET shadow stack are gated on linker support that lld's MinGW driver does not expose, and BinSkim's stack-cookie check uses an MSVC PE walker that does not find clang's emitted cookie. None are actionable from microsoft/git. Point the SDL templateContext at a per-arch suppression file at .azure-pipelines/sdl/$dim.id/gdnsuppress so Guardian skips these known-bad findings on each scan. Per-arch paths keep the entries isolated to the matching toolchain and let either arch grow new entries without touching the other. Seed windows_arm64/gdnsuppress with the 44 hydrated entries Guardian auto-published in the drop_build_windows_arm64_sdl_analysis artifact on the previous release run; the signatures are derived from (tool, ruleId, target URI) and remain stable across rebuilds, so the same file applies to future runs. windows_x64/gdnsuppress ships as a stub with no suppression entries. BA2025 is the only BinSkim finding on x64 and it is Warning-severity, so it does not break the build, and Guardian's pipeline-export only hydrates findings at or above Error severity, so no canonical entries were auto-generated to seed from. The stub keeps the per-arch path uniform without requiring a YAML conditional, and gives us a place to drop x64 entries later if we ever want to silence the warning. Assisted-by: Claude Opus 4.7 Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
dscho
approved these changes
May 28, 2026
| } | ||
| }, | ||
| "results": { | ||
| "700115aaeb52ef14c3ecbe6969846d61952c6d886621015dffde4e3bdb61da19": { |
Member
There was a problem hiding this comment.
These hashes look scary when you come from Git, as it looks like a content-based hash. However, as you told me, the Guardian documentation claims that this hash is based on the filename, the tool and the error ID. So I'm fine with that.
| "helpUri": "https://eng.ms/docs/microsoft-security/security/azure-security/cloudai-security-fundamentals-engineering/security-integration/guardian-wiki/microsoft-guardian/general/suppressions" | ||
| }, | ||
| "version": "1.0.0", | ||
| "suppressionSets": { |
Member
There was a problem hiding this comment.
Ideally, I'd love to include even the warning, just in case it gets bumped to an error in the future. We really cannot use MSVC to build Git, it's simply too far away from what upstream Git uses (there has been at least one crucial bug I identified in MSVC-based builds that would not occur in GCC-based builds, and I have no doubt that more footguns are lurking).
But as you've assured me, it's pretty impossible to generate those hashes manually, so we'll just have to trust our luck ;-)
Member
Author
There was a problem hiding this comment.
We can look at coaxing Guardian to emitting suppression files for these warnings, but the config to make it do so only runs on default branch runs.. at least that's what the docs + Copilot tells me, which seems to be the only way to get any glimpse of information about how this crappy tools work.
mpysson
reviewed
May 28, 2026
mpysson
approved these changes
May 28, 2026
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
3 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.
The 1ES PT Windows build job did not run binskim against the binaries we ship. By default the template would point binskim at the published artifact (the Inno Setup installer and the 7z self-extracting portable .exe), neither of which binskim can crack open to find the PE files inside, so any findings on those wrappers are also unactionable: they are produced by external tools we do not control.
We extract the
mingw-w64-{git,git-credential-wincred-git-pdbs}packages and point binskim explicitly at these actual product binaries. Only these packages because anything else is a 3rd party (Perl, MSYS2 runtime, GCM, Git LFS etc) binary that is not in this repository.Also add suppression files for issues we cannot fix. BinSkim flags toolchain-rooted issues on every release build from this repo:
CFG and CET shadow stack are gated on linker support that
lld's MinGW driver does not expose, and BinSkim's stack-cookie check uses an MSVC PE walker that does not find clang's emitted cookie. None are actionable from microsoft/git.Seed the
windows_arm64/gdnsuppressfile with the 44 hydrated entries Guardian auto-published in the previous release run; the signatures are derived from (tool, ruleId, target URI) and remain stable across rebuilds, so the same file applies to future runs.The
windows_x64/gdnsuppressfile ships as a stub with no suppression entries. BA2025 is the only BinSkim finding on x64 and it is Warning-severity, so it does not break the build, and Guardian's pipeline-export only hydrates findings at or above Error severity, so no canonical entries were auto-generated to seed from. The stub keeps the per-arch path gives us a place to drop x64 entries later if we ever want to silence the warning.