amadan
objstore: an app could make its own object untaggable
The second review of the policy fix found the other half of it, and this
one survives the fix that was just made.
6fba2b3 stopped an app writing `carlos-scan=clean`. It did not stop an
app filling the object's tag budget so that CARLOS can never write
anything. S3 caps an object at ten tags; tag() reads the existing set,
keeps every foreign tag, appends our three, and — when that overflows —
returns (false, nil). Silently.
So: upload malware with eight tags of your own. The sweep reads it,
clamd calls it infected, and the write is skipped. `scan enforce on`
denies reads keyed on `s3:ExistingObjectTag/carlos-scan = infected`, and
that Deny can only ever engage if the tag was WRITTEN — so it never
matches, and the object stays readable through the app's own credential.
Nothing alarms anywhere. The sweep counts the object as reached
(sweep.go ignores `recorded` deliberately), the on-demand path writes a
result saying recorded:false, and the status door reads no tag and
answers unknown. Every surface reports a system working normally.
THE YIELD WAS RIGHT AND IS KEPT, for clean. Erroring at the limit would
abandon the page unwritten every tick — the permanent stall the denied
classification was added to remove — and the only cost of not recording
a clean verdict is one re-scan. A tenant's tags, which on an ADOPTED
bucket may predate CARLOS entirely, are worth more than that.
For infected they are not. An infected verdict now evicts foreign tags
to fit, loudly, taking the minimum the limit forces: our three go in, as
many foreign tags as still fit are kept. That is a real loss of a
tenant's metadata and it is the right trade against a known-infected
object enforcement cannot touch.
Our three are also placed FIRST in the set now, so that truncating to
the limit drops foreign tags and never one of the three the whole
mechanism exists to write. That ordering is load-bearing and the
mutation below is why it is stated rather than assumed.
Three mutations, each caught, and the split pinned from both sides:
infected yields again -> TestInfectedIsRecordedEvenWhenForeignTagsCrowdItOut
evicts for every verdict -> TestCleanStillYieldsToForeignTagsAtTheLimit
(and the two existing limit tests)
truncates the wrong end -> both infected tests
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEkJh1ESqiA84quAHJV4K6
2 files changed,
+169
−8
internal/objstore/clamav.go+37 −8internal/objstore/clamav_test.go+132 −0
diff --git a/internal/objstore/clamav.go b/internal/objstore/clamav.go| index 7fa184d0e..0d1b30628 100644 |
| --- a/internal/objstore/clamav.go |
| +++ b/internal/objstore/clamav.go |
| @@ -5,6 +5,7 @@ import ( |
| "errors" |
| "fmt" |
| "io" |
| + "log" |
| "strings" |
| "time" |
| @@ -336,11 +337,14 @@ func verdictFor(s clamd.Status) Verdict { |
| // that is still using them for lifecycle rules or cost allocation. Replacing |
| // the set blindly would delete them, silently, on first scan. |
| func (c *clamavScanner) tag(ctx context.Context, bucket, key, version string, v Verdict) (recorded bool, err error) { |
| - set := []s3types.Tag{ |
| + scanTagSet := []s3types.Tag{ |
| {Key: s2aws(TagVerdict), Value: s2aws(string(v))}, |
| {Key: s2aws(TagVersion), Value: s2aws(version)}, |
| {Key: s2aws(TagAt), Value: s2aws(time.Now().UTC().Format(time.RFC3339))}, |
| } |
| + // OURS FIRST, so that truncating to the limit below drops foreign tags |
| + // and never one of the three this whole mechanism exists to write. |
| + set := append([]s3types.Tag(nil), scanTagSet...) |
| // Read-merge-write. One extra call per object per LIFETIME, not per sweep: |
| // an object whose verdict still matches its version never reaches Scan |
| // again. |
| @@ -369,14 +373,39 @@ func (c *clamavScanner) tag(ctx context.Context, bucket, key, version string, v |
| } |
| } |
| - // S3 caps an object at ten tags. An object already carrying eight foreign |
| - // ones cannot also hold our three, and erroring here would abandon the |
| - // page unwritten every single tick — the exact permanent stall the denied |
| - // classification was added to remove. Record the verdict for this sweep |
| - // and move on; the object is simply re-scanned next time rather than |
| - // blocking every object behind it. |
| + // S3 caps an object at ten tags, and an object already carrying eight |
| + // foreign ones cannot also hold our three. What happens next depends on |
| + // the verdict, and the two cases are genuinely different. |
| + // |
| + // CLEAN (or failed, or skipped): yield. Erroring would abandon the page |
| + // unwritten every single tick — the exact permanent stall the denied |
| + // classification was added to remove — and the only cost of not recording |
| + // is that the object is scanned again next time. A tenant's tags, which on |
| + // an ADOPTED bucket may predate CARLOS entirely, are worth more than one |
| + // avoided re-scan. |
| + // |
| + // INFECTED: do not yield. `scan enforce on` writes a Deny keyed on |
| + // `s3:ExistingObjectTag/carlos-scan = infected` (aws.go), so the Deny only |
| + // ever engages if this tag is WRITTEN. Yielding here means an app that |
| + // attaches eight tags of its own to an upload makes its objects |
| + // permanently unmarkable — verdict reached, write skipped, Deny never |
| + // matches, malware readable through the app's own credential, and nothing |
| + // anywhere alarms: the sweep counts the object as reached, the result |
| + // object says recorded:false, and the status door reads no tag and answers |
| + // unknown. |
| + // |
| + // So an infected verdict evicts foreign tags to fit. That is a real loss |
| + // of a tenant's metadata and it is the right trade against a known- |
| + // infected object that enforcement cannot touch. It takes the minimum the |
| + // limit forces — our three go in, as many foreign tags as still fit are |
| + // kept — because taking more than necessary would be gratuitous. |
| if len(set) > maxObjectTags { |
| - return false, nil |
| + if v != VerdictInfected { |
| + return false, nil |
| + } |
| + log.Printf("objstore: %s/%s is infected and carries %d foreign tags — evicting %d of them so the verdict can be recorded and enforcement can see it", |
| + bucket, key, len(set)-len(scanTagSet), len(set)-maxObjectTags) |
| + set = set[:maxObjectTags] |
| } |
| if _, err := c.s3.PutObjectTagging(ctx, &awss3.PutObjectTaggingInput{ |
diff --git a/internal/objstore/clamav_test.go b/internal/objstore/clamav_test.go| index c72978c59..3c4207fa2 100644 |
| --- a/internal/objstore/clamav_test.go |
| +++ b/internal/objstore/clamav_test.go |
| @@ -816,3 +816,135 @@ func TestScanDeniedIsNeverRecorded(t *testing.T) { |
| t.Fatal("a denied object reported its verdict as recorded") |
| } |
| } |
| + |
| +// AN INFECTED VERDICT MUST ALWAYS BE RECORDABLE, AND FOREIGN TAGS DO NOT |
| +// OUTRANK IT. |
| +// |
| +// The tag limit tolerance above is right for a clean verdict: the object is |
| +// simply re-scanned next tick, and an app's own tags — which on an ADOPTED |
| +// bucket may predate CARLOS entirely — are worth more than one avoided |
| +// re-scan. For infected it is a bypass of the whole enforcement mechanism. |
| +// |
| +// `scan enforce on` writes a Deny keyed on `s3:ExistingObjectTag/carlos-scan = |
| +// infected` (aws.go). That Deny only ever engages if the tag gets WRITTEN. An |
| +// app that attaches eight tags of its own to an upload makes its object |
| +// permanently unmarkable: the verdict is reached, the write is skipped, the |
| +// Deny never matches, and the malware stays readable through the app's own |
| +// credential. Nothing alarms — the sweep counts the object as reached, the |
| +// result object says recorded:false, and the status door reads no tag and |
| +// answers unknown. |
| +// |
| +// So: for infected, foreign tags are evicted to make room. That is a real |
| +// cost to a tenant's metadata and it is the right trade, because the |
| +// alternative is a known-infected object that enforcement cannot touch. |
| +func TestInfectedIsRecordedEvenWhenForeignTagsCrowdItOut(t *testing.T) { |
| + var foreign []string |
| + for i := 0; i < 8; i++ { |
| + foreign = append(foreign, "own-"+string(rune('a'+i)), "v") |
| + } |
| + var written []s3types.Tag |
| + s3 := &stubScanS3{t: t} |
| + s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| + return objectBody("hello"), nil |
| + } |
| + s3.allowTagRead(foreign...) |
| + s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| + written = in.Tagging.TagSet |
| + return &awss3.PutObjectTaggingOutput{}, nil |
| + } |
| + c := &clamavScanner{ |
| + s3: s3, |
| + engine: &fakeEngine{res: clamd.Result{Status: clamd.StatusInfected, Signature: "Eicar-Test"}}, |
| + maxBytes: 1 << 20, |
| + } |
| + |
| + v, recorded, err := c.Scan(context.Background(), "b", "k", "v1", 5) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if v != VerdictInfected { |
| + t.Fatalf("verdict = %q, want infected", v) |
| + } |
| + if !recorded { |
| + t.Fatal("an infected verdict went unrecorded — `scan enforce on` denies on the TAG, so an object that cannot be tagged cannot be quarantined") |
| + } |
| + if len(written) == 0 { |
| + t.Fatal("nothing was written to the object at all") |
| + } |
| + if len(written) > maxObjectTags { |
| + t.Fatalf("wrote %d tags, over S3's limit of %d — the write would be rejected", len(written), maxObjectTags) |
| + } |
| + var gotVerdict string |
| + for _, tg := range written { |
| + if aws2s(tg.Key) == TagVerdict { |
| + gotVerdict = aws2s(tg.Value) |
| + } |
| + } |
| + if gotVerdict != string(VerdictInfected) { |
| + t.Fatalf("the written set carries carlos-scan=%q, want infected:\n%v", gotVerdict, written) |
| + } |
| +} |
| + |
| +// The eviction is only as large as it needs to be: our three go in, and as |
| +// many foreign tags as still fit are kept. Dropping all of them would be a |
| +// simpler implementation and a gratuitous one — the object is a tenant's, and |
| +// this takes the minimum the limit forces. |
| +func TestInfectedEvictsOnlyAsManyForeignTagsAsItMustNot(t *testing.T) { |
| + var foreign []string |
| + for i := 0; i < 8; i++ { |
| + foreign = append(foreign, "own-"+string(rune('a'+i)), "v") |
| + } |
| + var written []s3types.Tag |
| + s3 := &stubScanS3{t: t} |
| + s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| + return objectBody("hello"), nil |
| + } |
| + s3.allowTagRead(foreign...) |
| + s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| + written = in.Tagging.TagSet |
| + return &awss3.PutObjectTaggingOutput{}, nil |
| + } |
| + c := &clamavScanner{ |
| + s3: s3, |
| + engine: &fakeEngine{res: clamd.Result{Status: clamd.StatusInfected, Signature: "Eicar-Test"}}, |
| + maxBytes: 1 << 20, |
| + } |
| + if _, _, err := c.Scan(context.Background(), "b", "k", "v1", 5); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + var kept int |
| + for _, tg := range written { |
| + if !isScanTag(aws2s(tg.Key)) { |
| + kept++ |
| + } |
| + } |
| + // Ten minus our three. |
| + if want := maxObjectTags - 3; kept != want { |
| + t.Fatalf("kept %d foreign tags, want %d — evict the minimum the limit forces, not all of them:\n%v", kept, want, written) |
| + } |
| +} |
| + |
| +// And a CLEAN verdict still yields, which is the behaviour the two tests above |
| +// this one pin. The split is deliberate: clean costs one re-scan, infected |
| +// costs the enforcement Deny never engaging. |
| +func TestCleanStillYieldsToForeignTagsAtTheLimit(t *testing.T) { |
| + var foreign []string |
| + for i := 0; i < 8; i++ { |
| + foreign = append(foreign, "own-"+string(rune('a'+i)), "v") |
| + } |
| + s3 := &stubScanS3{t: t} // putTagging unset: any write fails the test |
| + s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| + return objectBody("hello"), nil |
| + } |
| + s3.allowTagRead(foreign...) |
| + c := &clamavScanner{s3: s3, engine: &fakeEngine{res: clamd.Result{Status: clamd.StatusClean}}, maxBytes: 1 << 20} |
| + |
| + v, recorded, err := c.Scan(context.Background(), "b", "k", "v1", 5) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if v != VerdictClean || recorded { |
| + t.Fatalf("clean = %q recorded=%v, want clean and unrecorded — a tenant's tags outrank one avoided re-scan", v, recorded) |
| + } |
| +} |