Repository navigation
P05 type bound value tags - #7
Merged
Merged
Conversation
1024px and 256px PNGs for use as the repo avatar/social preview. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This reverts commit eda7cee. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Independently re-verifies the seven "Hoch" findings (P01-P07) from the 2026-09-29 repository analysis against current source, confirms all seven, and lays out a phased mitigation plan with effort estimates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ranks the seven confirmed High findings into tiers by real-world impact (silent total plaintext exposure > partial leak > data loss > conditional local exposure > integrity-only), and groups them into six work packages (A-F) that share code paths or defensive patterns, with a recommended shipping order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three independent fixes for the same underlying defect class from the 2026-09-29 audit's P07 finding — a registered rule doesn't guarantee git and nebel agree on what's matched: - Clean and Smudge now return ErrNoMatchingRule instead of silently passing content through when git invokes the filter for a path no rule matches. Git only calls this filter for a path .gitattributes marks filter=nebel, so a non-match here means .gitattributes and .nebel.yaml disagree — most commonly because a bare pattern like "*.env" matches recursively in .gitattributes but only at its own depth under doublestar. Before this fix, that mismatch let a subdirectory file's plaintext reach the git object database undetected; filter.nebel.required=true now turns the mismatch into a blocked `git add`/commit instead. Verified against a real git repo, including confirming the new e2e test fails against the pre-fix code. - Config.Validate rejects a syntactically invalid glob pattern (via doublestar.ValidatePattern) at config-load time, since MatchRule's bool-only signature has no room to propagate doublestar.Match's own error — an invalid pattern would otherwise silently behave as "never matches" instead of surfacing the typo. - addGitattributesPattern now escapes characters .gitattributes' line syntax gives special meaning to (a leading '#' or '!', embedded spaces) before writing a pattern, so a filename with a space no longer silently gets no effective filter assignment while the add command reports success. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A configured dot-notation path can silently resolve to more than one
distinct location in the same document, and Locate's targeted descent
can only ever return one of them:
- P03: a literal key containing a dot ("a.b") collides with the
equivalent nested path ("a" containing "b") — dot notation can't
tell them apart, and a dot inside a key can't be escaped (see
ParsePath's doc comment). Before this fix, Locate always resolved to
whichever candidate its traversal order reached first, silently
leaving the other value's secret unencrypted while reporting
success.
- P04: encoding/json's streaming decoder does not reject a duplicate
object key the way go-yaml's parser does (confirmed empirically:
go-yaml's parser.ParseBytes already errors "mapping key ... already
defined" on a duplicate YAML key, so only JSON needed this part of
the fix). descendObject returned the first occurrence's span,
leaving the second — which, per encoding/json's own map-unmarshal
semantics, most real consumers would treat as the effective value —
in plaintext.
Both are the same underlying problem: a path string that maps to more
than one leaf. Locate now calls the existing Leaves() walk (which,
unlike a targeted descent, visits every literal key and so surfaces a
collision a single-path lookup never would) and refuses with the new
ErrAmbiguousPath if more than one leaf shares the configured path,
before ever resolving to one of them.
Verified against the pre-fix code directly: Locate("a.b") on
{"a.b":"secret","a":{"b":"decoy"}} silently returned "decoy" with no
error, and Locate("password") on a duplicate-keyed JSON document
silently returned only "eins" — exactly the silent misresolution the
audit described. Full existing test suite passes unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
scalarText truncated a scalar token's Origin at the first " #" with no awareness of quoting, so a quoted value like password: "abc # geheim" was truncated to "abc, leaving the untruncated remainder (# geheim") as literal plaintext in the git blob right next to the encrypted tag, since the full value was still what got encrypted into the tag. Verified empirically against goccy/go-yaml (not just by reading the code) that a quoted scalar's Origin is, in every case tested - with or without a following real comment, with or without trailing whitespace, at end of file or followed by more document - always exactly the quoted literal verbatim, with no trailing trivia ever folded in. So the fix does not need to parse or search quoted content at all: scalarText now returns a quoted scalar's trimmed Origin as-is, and only applies the " #" search to plain (unquoted) scalars, where it was originally aimed and remains correct (a plain scalar's Origin can never contain " #" as its own content in the first place - the lexer itself treats that sequence as the start of a comment while still scanning the scalar, whatever the author intended). Verified directly against the pre-fix code: Locate on password: "abc # geheim" returned span [10:14) = "abc (4 bytes) while span.Value was already the full, correctly-parsed "abc # geheim" - the exact partial-plaintext-leak mechanism the audit described. Found and fixed along the way, not part of the audited P02 itself: ast.LiteralNode (go-yaml's AST type for both | literal and > folded block scalars) wraps a header token (just the indicator character) separate from its actual content token, and its GetToken() - which Locate and Leaves used unconditionally - returns only the header. Confirmed directly against the pre-fix code that Locate on a block scalar field returned span [10:11) = "|" with no error: a mode: value rule naming such a field would splice a ciphertext tag over the single header character, destroying the document instead of protecting the secret beneath it. Locate now refuses this with a new ErrBlockScalarUnsupported instead, and Leaves no longer offers such a field to nebel add field's picker. There is no existing format support to fall back on for these styles; refusing outright is the safe behavior pending proper support. Full existing test suite passes unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…bonus finding Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CheckoutAll's os.Remove-then-checkout had no backup step at all: it unconditionally deletes every managed file, then restores each from `git checkout HEAD -- <path>`, which overwrites the working tree from HEAD regardless of what's in the index or in an unstaged edit. Both `nebel init` (repeated) and `nebel init --version N` call this, so an ordinary re-init could silently destroy uncommitted work with no warning. Added DirtyFiles, which runs `git diff --name-only HEAD -- <files>` - this single command covers staged and unstaged changes alike, since it compares the working tree directly against HEAD regardless of the index. CheckoutAll now calls it before touching anything and refuses with a new ErrDirtyManagedFiles, naming every dirty path, if any managed file differs from HEAD. Verified directly against the pre-fix code with a standalone probe: an unstaged edit to a managed file was silently destroyed by CheckoutAll - it returned decrypted=1, failures=nil, err=nil, and the file's content reverted to what was committed, with the local edit gone. With the fix, the same scenario (both unstaged and staged variants) is refused outright via ErrDirtyManagedFiles, with the local edit left completely untouched; a clean repo (the ordinary post-init case) still succeeds normally. go build, go vet, and go test all pass on internal/gitutil. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…(P06) localkey.Set base64-encoded the full derived key and passed it to gitutil.ConfigSet, which runs `git config --local <name> <value>` with the key as a literal subprocess argument. During that subprocess's (brief) runtime, the ready-to-use decryption key sat in its argument list, readable by any other local user or process with sufficient privilege via /proc/<pid>/cmdline. This is the same class of exposure cmd/nebel/password.go's ErrPasswordArgument already refuses for the shared password itself; the derived key wasn't getting the same protection. Set now writes keys to a new dedicated file (internal/localkey's keyFileName, "nebel-keys") inside the git directory shared across every worktree of a clone (resolved via `git rev-parse --git-common-dir`, run directly rather than through gitutil, to keep this fix self-contained), with 0600 permissions — owner read/write only, tighter than .git/config's ambient permissions, which also answers a related "harden local key storage" suggestion from the same audit. This never puts the key on any process's argument list. Backward compatibility: Get and All still fall back to the legacy git-config location for any version registered there before this fix shipped, so an existing clone keeps working without needing to re-run `nebel init`. New registrations always go to the key file; nothing already in git config is migrated or removed. Several cmd/nebel e2e tests asserted directly on the old storage mechanism (`git config --local --get filter.nebel.key...`) as their way of checking whether a key was or wasn't registered; three of them failed outright once Set stopped writing there, and two others (which only checked for the key's *absence*) would have kept passing but vacuously, no longer proving what they claimed. Added a shared registeredKeyValue test helper that checks the actual current storage (the key file, falling back to legacy git config, mirroring internal/localkey.Get's own precedence) and updated all five call sites to use it. Verified directly against the pre-fix code: calling Set left the raw base64 key sitting in plain `.git/config` file content under `[filter "nebel"] key = ...` — written by exactly the `git config --local` subprocess call whose argument list this fix removes the key from. The audit's actual concern, the key's brief appearance in that subprocess's own argv, isn't itself observable from within a Go test (the subprocess exits before a test could inspect it); proving the key no longer ends up as a plain git-config value at all is the most direct available proxy, since getting it there is exactly what required passing it as that argument in the first place. Full existing test suite passes unchanged, including cmd/nebel's e2e suite (which exercises the full init/rotate/checkout flow depending on localkey end-to-end). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A value tag's type field (type:str, type:bool, ...) sat next to its
ciphertext as plaintext metadata, not inside anything SIV
authenticated - so changing type:str to type:bool needed no key at
all, only write access to the file the tag is stored in. Before this
fix, that silently changed how a decrypted string rendered (unquoted,
as a boolean or number, instead of a quoted string) without tripping
authentication at all. Confirmed live against the pre-fix code: a
type:str -> type:bool edit on an untouched ciphertext succeeded with
err=nil, and password: "s3cr3t" smudged back as the unquoted
password: s3cr3t.
This is a breaking ciphertext-tag format change, exactly as flagged in
the mitigation plan: every previously-committed value tag was
authenticated without its type, so simply adding type to the AAD
everywhere would fail every one of them, not just tampered ones. Fixed
via the same mechanism spec 03 AC-3.8 already uses to reject an
unsupported ALGO: value tags now come in two forms, distinguished by
that field.
- internal/siv: AAD gained a variadic extra parameter, appended as
more length-prefixed components after the existing three - existing
3-argument call sites are unaffected.
- internal/tag: a new AlgoAES256SIVTypeBound ("AES256_SIV_TB")
identifies a value tag whose type is bound into the AAD. EncodeValue
now always produces this form. Parse accepts either this or the
legacy AlgoAES256SIV, exposing which one via a new Tag.TypeBound
field - false for every whole-file tag (nothing to bind there) and
for a legacy value tag, true only for the new form. A type-bound
algo naming with no type field at all (nothing to bind) is rejected
as malformed rather than silently accepted.
- internal/filterop: cleanValues now always encrypts with type
included in the AAD (matching what EncodeValue just wrote);
smudgeValues reconstructs the AAD Tag.TypeBound calls for - with
type for a type-bound tag, without it for a legacy one - so each
authenticates against whatever it was actually encrypted under.
No separate migration tooling exists or is needed: cleanValues already
unconditionally re-encrypts any plaintext field it finds via
EncodeValue, so a legacy value converges onto the type-bound form the
next time anything re-encrypts it - an ordinary edit, nebel rotate, or
git add --renormalize - the same way a value converges onto a new key
version today. A legacy tag's type remains unauthenticated until that
happens; this is documented as an explicit, understood limitation
(TestSmudgeValuesLegacyTagTypeStillUnauthenticated), not an oversight,
since re-authenticating it retroactively is exactly what would break
every already-committed legacy tag.
Verified directly against the pre-fix code (reverting
internal/siv/siv.go, internal/tag/tag.go, internal/filterop/filterop.go
to HEAD and re-running the new test): flipping type:str to type:bool on
a freshly-cleaned tag produced no authentication error and rendered the
value unquoted, exactly the vulnerability described above. Restored the
fix afterward; full test suite (go build, go vet, go test ./...) passes
including cmd/nebel's e2e suite.
Also updates specs/03-value-encoding.md with AC-3.10 describing the two
tag forms and the authentication guarantee.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three follow-ups to ba8e9aa, found by a second review pass rather than by the original audit: 1. Restores a stopgap the mitigation plan called for and Package F dropped: a legacy (non-type-bound) tag's type field is still unauthenticated, so a successful flip could previously render arbitrary decrypted text unquoted for TypeInt/TypeFloat/TypeBool - a potential structural-injection vector for a crafted plaintext, not just type confusion. smudgeValues now rejects a decrypted value that isn't a genuine literal for its claimed type (validTypeLiteral, using strconv's parsers) before rendering it. A type-bound tag can never reach this in practice - its type can't change without failing decryption first - so this only ever narrows what a legacy tag's still-possible type flip can do, without weakening the new protection at all. 2. Corrects an overstated claim in ba8e9aa's tag.go/specs/03 comments: "a value converges onto the type-bound form the next time anything re-encrypts it" is true, but only protects the *current* committed tag going forward. An attacker with write access can still resurrect an old legacy ciphertext from git history (or a stale branch or clone) with its type re-flipped, and it authenticates exactly as it did when first committed, since spec 11 AC-11.5 keeps every key version registered indefinitely - upgrading the current commit doesn't revoke it. Fully closing that needs actively revoking the old key version, which rotation does not do; out of scope here. Documented plainly rather than left implied. 3. Fixes a real interaction between Package D and Package F, confirmed with a new e2e test rather than left as a hypothesis: DirtyFiles (gitutil, P01's dirty-check) compares by running the clean filter, which is unavoidable for a filtered path - `git diff HEAD` has to clean the working tree to compare it against HEAD's (also filtered) blob. A legacy value tag re-encrypts to different bytes the moment anything cleans it again, since the AAD it's produced under changed - so a repeated `nebel init` on a repo with an existing legacy tag, and no actual local edits, now refuses with ErrDirtyManagedFiles. The new e2e test (cmd/nebel/legacytag_e2e_test.go) reproduces this against a real repo with a real derived key and confirms the fix: `git add --renormalize` alone is not enough, since it only updates the index, and DirtyFiles compares against HEAD regardless - the renormalized tag must also be committed. ErrDirtyManagedFiles' error text and doc comment now say so explicitly. Full test suite (go build, go vet, go test ./...) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 tasks done
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
1 participant
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.
No description provided.