Repository navigation
Harden the parsers, gate on gosec, and fix the broken verification docs - #3
Merged
Merged
Conversation
…ty XOR key gosec's default ruleset flagged 47 sites in the credential, Kerberos and parser packages. None were exploitable, but none were documented either, so each is now annotated inline with the reason it cannot be reached by attacker input: protocol-mandated DES/RC4/MD5/MD4 (MS-SAMR, MS-NLMP, RFC 4757), width narrowing inside binary parsers bounded by their on-disk format, and fixed argv in the schtasks/powershell helpers. xorBytes divided by len(key) without checking it. No caller passes an empty key today, but the failure mode is a panic inside an elevated process, so it now returns nil and lets the AEAD reject the key size instead. Tighten the persist shim modes to 0700/0600: Windows derives the real ACL from the parent and only honours the read-only bit, so this costs nothing and keeps the intent explicit if the code is ever ported.
The regf hive parser, the KDC reply parser, the payload envelope reader, the string obfuscator and the persist name sanitizer all consume bytes this tool did not author, inside a process that holds an elevated token. They now have fuzz targets — FuzzRegf, FuzzParseRep, FuzzUnpack, FuzzMaskRoundTrip, FuzzSanitizeName — and the seeds run as ordinary tests, so the corpus is exercised on every `go test` without an explicit -fuzz. FuzzMaskRoundTrip asserts the mask is a true involution over arbitrary bytes; FuzzSanitizeName asserts the output is idempotent, bounded and shell-safe. Coverage 38% -> 44.7%, with tests for the portable hive parser (loadHive rejection, subkey/path walking, resident and hex-class values), WipeFile including the hard-link truncate path, the collector helpers (xorBytes, aesGCMOpen, forceCopy, truncateForLog), the persist backend, the opt-in insecure TLS config and the elevate error paths.
Two new jobs. `fuzz-smoke` runs the five parser targets for 30s each on every push and PR: a panic in one of them is an availability bug in the console that holds an elevated token, not a test artefact. `sast` runs gosec with GOOS pinned to windows — on linux the scan would silently skip the windows-only packages, which hold the entire security-sensitive surface. Two rules are excluded on the command line: G103 (37 hand-reviewed unsafe sites; the tool is an unsafe/syscall bridge) and G104 (14 unchecked errors, every one a deferred handle close). Everything else fails the build. gosec cannot build SSA for internal/research because go/types rejects a bodyless function (EnergyFlow lives in energyflow_amd64.s), so the job asserts that the missing-function-body error is the only tolerated one: a new type error would disable analysis just as silently, and now fails too. Raise the coverage floor 35 -> 42 to match the new whole-tree number, and add make targets (sast, fuzz-check, cover) so all of it is reachable locally.
…nity files docs/SIGNING.md told verifiers to expect the certificate identity DevCop95/obliteratus. That repository does not exist — the workflow runs in DevCop95/pullgoscript — so the documented command failed against the real v2.0.2 release. Replaced with the actual subject and the observed output of both the passing and the failing case, plus a note that the identity changes if the repository is ever renamed. The README's releases link pointed at the same wrong name. The Authenticode section now states the remaining supply-chain gap honestly rather than implying the artifacts are already Windows-trusted: cosign and the SBOM are live, Authenticode is blocked on acquiring a certificate. Add CODE_OF_CONDUCT.md (Contributor Covenant 2.1), issue forms that route vulnerability reports to private reporting instead of the public tracker, a PR checklist, and document the new CI gates in CONTRIBUTING.md.
setup-go does not add $(go env GOPATH)/bin to PATH, so the scan step died with 'gosec: command not found' (exit 127) after a successful install.
Job-level GOOS cross-compiled gosec itself into a PE binary (gosec.exe), which the Linux runner then could not execute. The runner's own GOOS is what installs a usable binary; only the scan needs to see the windows-only packages.
"0 findings" and "the scan never parsed any code" must not look the same. The job now asserts gosec's own Stats.files is non-zero and prints files/lines/nosec found, so a green result is auditable rather than assumed. Verified against gosec v2.29.0 locally before pushing: files=37, lines=7194, nosec=47, found=0.
21 tasks
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
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.
Hardening pass for #2. Four commits, one per theme, all gates green locally.
What this does
Fuzzing the parsers that eat attacker-influenced bytes —
FuzzRegf(registryhive),
FuzzParseRep(KDC reply),FuzzUnpack(payload envelope),FuzzMaskRoundTrip(obfuscator) andFuzzSanitizeName(persist backend). Allfive consume input this tool did not author, in a process holding an elevated
token, where a panic is an availability bug and not a test artefact. The seeds
run as ordinary tests, so the corpus is exercised on every
go test; CI adds a30s-per-target smoke run on every push and PR.
SAST, with every suppression justified — gosec flagged 47 sites. None were
exploitable, but none were documented either, so each now carries an inline
//#nosec <rule> -- <reason>: protocol-mandated DES/RC4/MD5/MD4 ([MS-SAMR],[MS-NLMP], RFC 4757), width narrowing bounded by an on-disk format, and fixed
argv in the
schtasks/powershellhelpers. Two rules are excluded on thecommand line with their rationale written next to them: G103 (37 hand-reviewed
unsafesites — the tool is anunsafe/syscall bridge) and G104 (14unchecked errors, every one a deferred handle close). File modes tightened
0755/0644 → 0700/0600.
A real bug found on the way —
xorBytesdivided bylen(key)with nocheck. No caller passes an empty key today, so it was latent, but the failure
mode is a panic inside an elevated process. It now fails closed and returns
nil, letting the AEAD reject the key size.Coverage 38% → 44.7%, floor 35 → 42 — tests for the portable hive parser,
WipeFile(including the hard-link truncate path), the collector helpers(
xorBytes,aesGCMOpen,forceCopy,truncateForLog), the persist backend,the opt-in insecure TLS config and the elevate error paths.
A documentation bug that broke verification —
docs/SIGNING.mdtoldverifiers to expect the certificate identity
DevCop95/obliteratus, arepository that does not exist; the workflow runs in
DevCop95/pullgoscript.The documented command therefore failed against the real v2.0.2 release. Now
corrected, with the observed output of both the passing and the failing case.
Community files — Contributor Covenant 2.1, issue forms that route
vulnerability reports to private reporting rather than the public tracker, a PR
checklist, and the CI gates documented in
CONTRIBUTING.md.Verification
Cross-compile was checked with
GOOS=linux go vet ./...because this machinehas no Docker or working WSL; the Linux job in CI is the real confirmation.
Known limitations, stated rather than hidden
Trusted Signing account — paid and identity-verified — so it cannot be closed
from here.
docs/SIGNING.mdnow carries a status table saying exactly thatinstead of implying the artifacts are Windows-trusted. No release is blocked
by its absence.
go/typesrejects the bodylessEnergyFlow(implemented in
energyflow_amd64.s), so gosec cannot build SSA forinternal/researchand its taint rules skip that package. The job assertsthat the missing-function-body error is the only tolerated one, so any new
type error — which would disable analysis just as silently — fails the build.
fuzz-smokeandsastmustbe added to the
mainruleset to actually block a merge.make sast/make fuzz-check/make covertargets were notexecuted here (
makeis not installed on this machine); each wraps a commandthat was run individually and is quoted in
CONTRIBUTING.mdfor anyonewithout
make.Refs #2.