amadan
objstore: the POST-policy forgery, performed against a real gateway
Discussion #19's reproduction, restored now that it can pass — and rewritten, because the obvious version of it was vacuous and I shipped that version first in this working tree before the mutation caught it. THE OBVIOUS TEST PROVES NOTHING. Upload via POST policy with carlos-scan=clean and a nonsense carlos-scan-version, assert the scanner says unknown. It does say unknown — on the VERSION comparison, before the signature is ever consulted. Disabling verification entirely leaves that test green, which is the definition of testing the wrong thing. So this performs the whole attack. The ETag of a single-part upload is the MD5 of bytes the app chose; last-modified is the server clock at upload, to the second, which it can guess and retry until it lands. The test guesses, HEADs, and retries until the forged carlos-scan-version MATCHES the object's real version — and only then asserts. At that point every check except one says clean. With verification disabled the assertion now fails against a real server, with the sentence it was written for: verdict = "clean" for a tag the APP wrote, on a real gateway, with a matching version — an app just marked its own malware clean The test asserts both halves deliberately: that the tagged upload SUCCEEDS (no IAM condition key describes a form upload, so nothing in a policy document stops it) and that the tag is not believed. The first half is what every previous version of this property got wrong, and a test that only checked the second would let a future change quietly close the write path and leave nobody knowing the difference. It skips rather than fails if a gateway refuses the tagged upload, or if the retry does not land in eight attempts. A gateway that refuses is a better world and should say so; a race that does not reproduce is not evidence that the attack is unavailable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEkJh1ESqiA84quAHJV4K6
1 file changed,
+207
−0
internal/objstore/contract_test.go+207 −0
CI passed — run details
diff --git a/internal/objstore/contract_test.go b/internal/objstore/contract_test.go| index 8e6e84e71..3e0271970 100644 |
| --- a/internal/objstore/contract_test.go |
| +++ b/internal/objstore/contract_test.go |
| @@ -3,10 +3,20 @@ package objstore |
| import ( |
| "bytes" |
| "context" |
| + "crypto/hmac" |
| + "crypto/md5" |
| + "crypto/sha256" |
| + "encoding/base64" |
| + "encoding/hex" |
| "errors" |
| + "fmt" |
| "io" |
| + "mime/multipart" |
| + "net/http" |
| "os" |
| + "strings" |
| "testing" |
| + "time" |
| "github.com/aws/aws-sdk-go-v2/aws" |
| "github.com/aws/aws-sdk-go-v2/credentials" |
| @@ -327,6 +337,110 @@ func TestDeliveredContractMinio(t *testing.T) { |
| }) |
| }) |
| + // THE ROUTE IAM CANNOT CLOSE, AND WHAT ACTUALLY CLOSES IT. |
| + // |
| + // A browser-direct POST-policy upload carries its tags in a form |
| + // field. MinIO does not populate s3:RequestObjectTagKeys for that |
| + // handler, so the Deny that stops every other route does not fire and |
| + // THE UPLOAD SUCCEEDS — carlos-scan=clean and all. No IAM condition |
| + // key describes a form upload, and a bucket policy uses the same keys, |
| + // so nothing in a policy document prevents this. |
| + // |
| + // It does not need to. The tag is written and it means nothing. |
| + // |
| + // THIS TEST PERFORMS THE WHOLE ATTACK, and it has to, because the |
| + // obvious version of it is vacuous. A forged tag carrying a nonsense |
| + // carlos-scan-version is rejected on the version comparison and never |
| + // reaches the signature check at all — so it passes with verification |
| + // disabled, which is exactly what the first draft of this test did. |
| + // |
| + // The real attack is available to the app: the ETag of a single-part |
| + // upload is the MD5 of bytes it chose, and last-modified is the server |
| + // clock at upload to the second, which it can guess and retry until it |
| + // lands. So the loop below guesses, checks, and retries — and only |
| + // then asserts. A forged tag with a MATCHING version is believed by |
| + // every check except the one that matters. |
| + t.Run("a POST-policy forged tag with a matching version is still not a verdict", func(t *testing.T) { |
| + const forged = "contract-test/post-forged.txt" |
| + body := []byte("a payload the app would like called clean") |
| + etag := fmt.Sprintf("%x", md5.Sum(body)) |
| + |
| + defer client.DeleteObject(ctx, &awss3.DeleteObjectInput{ |
| + Bucket: aws.String(bucket), Key: aws.String(forged), |
| + }) |
| + |
| + var landed string |
| + for attempt := 0; attempt < 8 && landed == ""; attempt++ { |
| + guess := time.Now().UTC().Truncate(time.Second) |
| + want := etag + "@" + guess.Format(time.RFC3339) |
| + |
| + status, resp := postPolicyUploadBody(t, endpoint, key, secret, bucket, forged, body, |
| + "<Tagging><TagSet>"+ |
| + "<Tag><Key>carlos-scan</Key><Value>clean</Value></Tag>"+ |
| + "<Tag><Key>carlos-scan-version</Key><Value>"+want+"</Value></Tag>"+ |
| + "<Tag><Key>carlos-scan-at</Key><Value>"+guess.Format(time.RFC3339)+"</Value></Tag>"+ |
| + "</TagSet></Tagging>") |
| + if status < 200 || status >= 300 { |
| + // A gateway that refuses this is a better world, and this |
| + // test should say so rather than quietly passing. |
| + t.Skipf("this gateway refused a tagged POST-policy upload (%d) — the IAM condition key reaches it here:\n%s", status, resp) |
| + } |
| + head, err := client.HeadObject(ctx, &awss3.HeadObjectInput{ |
| + Bucket: aws.String(bucket), Key: aws.String(forged), |
| + }) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + obj := ScanObject{Key: forged, ETag: normalizeETag(aws.ToString(head.ETag))} |
| + if head.LastModified != nil { |
| + obj.Modified = *head.LastModified |
| + } |
| + if obj.Version() == want { |
| + landed = want |
| + } |
| + } |
| + if landed == "" { |
| + t.Skip("could not land a forged tag on a matching version in eight attempts — the attack is real but this run did not reproduce it") |
| + } |
| + |
| + // The tag really is on the object, with a version that really does |
| + // match. Everything short of the signature now says "clean". |
| + tags, err := client.GetObjectTagging(ctx, &awss3.GetObjectTaggingInput{ |
| + Bucket: aws.String(bucket), Key: aws.String(forged), |
| + }) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var gotVerdict, gotVersion string |
| + for _, tg := range tags.TagSet { |
| + switch aws.ToString(tg.Key) { |
| + case TagVerdict: |
| + gotVerdict = aws.ToString(tg.Value) |
| + case TagVersion: |
| + gotVersion = aws.ToString(tg.Value) |
| + } |
| + } |
| + if gotVerdict != string(VerdictClean) || gotVersion != landed { |
| + t.Fatalf("the forgery is not in place (verdict=%q version=%q), so the refusal below proves nothing", gotVerdict, gotVersion) |
| + } |
| + |
| + // And the part that matters. The engine is nil deliberately: |
| + // reaching it would mean the scanner decided to scan, which is the |
| + // right outcome but not what this asserts. This asserts the forged |
| + // verdict was not believed. |
| + sc := &clamavScanner{ |
| + s3: client, engine: nil, maxBytes: 1 << 20, |
| + signer: testSigner(t, "a-real-deployment-key", ""), |
| + } |
| + v, err := sc.Verdict(ctx, bucket, forged, landed) |
| + if err != nil { |
| + t.Fatalf("Verdict over a forged tag: %v", err) |
| + } |
| + if v != VerdictUnknown { |
| + t.Fatalf("verdict = %q for a tag the APP wrote, on a real gateway, with a matching version — an app just marked its own malware clean", v) |
| + } |
| + }) |
| + |
| // And the same control for the plain PUT route, which has the same |
| // hazard: an ordinary untagged upload must still work, or the Deny is |
| // firing on every write and the store is simply broken. |
| @@ -434,3 +548,96 @@ func requireDenied(t *testing.T, err error, what string) { |
| t.Fatalf("%s: error code = %q, want AccessDenied (message: %s)", what, apiErr.ErrorCode(), apiErr.ErrorMessage()) |
| } |
| } |
| + |
| +// postPolicyUpload performs a browser-style direct upload: a base64 policy |
| +// document signed with SigV4, POSTed as a multipart form. There is no helper |
| +// for this in the AWS SDK for Go, which is a good part of why this route goes |
| +// untested — and it is the route an app actually uses to take a file from a |
| +// user without proxying the bytes. |
| +// |
| +// Hand-rolled rather than pulled in: this repo adds no third-party |
| +// dependencies, and the whole of SigV4 for a POST policy is a four-step HMAC |
| +// chain over a date, a region, a service and a terminator. |
| +func postPolicyUpload(t *testing.T, endpoint, akid, secret, bucket, key, taggingXML string) (int, string) { |
| + t.Helper() |
| + return postPolicyUploadBody(t, endpoint, akid, secret, bucket, key, []byte("payload"), taggingXML) |
| +} |
| + |
| +// postPolicyUploadBody is the same with the bytes chosen by the caller, which |
| +// the forgery test needs: the ETag of a single-part upload is the MD5 of those |
| +// bytes, and predicting it is half of what makes the attack available. |
| +func postPolicyUploadBody(t *testing.T, endpoint, akid, secret, bucket, key string, body []byte, taggingXML string) (int, string) { |
| + t.Helper() |
| + |
| + const region = "us-east-1" |
| + now := time.Now().UTC() |
| + amzDate := now.Format("20060102T150405Z") |
| + scopeDate := now.Format("20060102") |
| + credential := akid + "/" + scopeDate + "/" + region + "/s3/aws4_request" |
| + |
| + // The policy the browser would be handed. `starts-with` on $tagging is |
| + // what an app would write if it wanted the browser to choose tags — which |
| + // is precisely the permissive shape worth testing. |
| + policy := fmt.Sprintf(`{"expiration":%q,"conditions":[ |
| + {"bucket":%q}, |
| + {"key":%q}, |
| + {"x-amz-algorithm":"AWS4-HMAC-SHA256"}, |
| + {"x-amz-credential":%q}, |
| + {"x-amz-date":%q}, |
| + ["starts-with","$tagging",""] |
| + ]}`, now.Add(15*time.Minute).Format(time.RFC3339), bucket, key, credential, amzDate) |
| + encoded := base64.StdEncoding.EncodeToString([]byte(policy)) |
| + |
| + mac := func(k []byte, s string) []byte { |
| + h := hmac.New(sha256.New, k) |
| + h.Write([]byte(s)) |
| + return h.Sum(nil) |
| + } |
| + signing := mac(mac(mac(mac([]byte("AWS4"+secret), scopeDate), region), "s3"), "aws4_request") |
| + sig := hex.EncodeToString(mac(signing, encoded)) |
| + |
| + var buf bytes.Buffer |
| + w := multipart.NewWriter(&buf) |
| + for _, f := range [][2]string{ |
| + {"key", key}, |
| + {"x-amz-algorithm", "AWS4-HMAC-SHA256"}, |
| + {"x-amz-credential", credential}, |
| + {"x-amz-date", amzDate}, |
| + {"policy", encoded}, |
| + {"x-amz-signature", sig}, |
| + {"tagging", taggingXML}, |
| + } { |
| + if err := w.WriteField(f[0], f[1]); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + // The file part must come last: S3 ignores anything after it. |
| + part, err := w.CreateFormFile("file", "upload.txt") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := part.Write(body); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := w.Close(); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + // Path-style, matching the client the rest of this test builds. |
| + req, err := http.NewRequest(http.MethodPost, strings.TrimSuffix(endpoint, "/")+"/"+bucket, &buf) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + req.Header.Set("Content-Type", w.FormDataContentType()) |
| + |
| + resp, err := http.DefaultClient.Do(req) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer resp.Body.Close() |
| + out, err := io.ReadAll(io.LimitReader(resp.Body, 1<<16)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return resp.StatusCode, string(out) |
| +} |