fix(scan): require a real bucket listing body before flagging s3 exposure (#282)

internal/scan/cloudstorage.go

@@ -15,6 +15,7 @@ package scan
 import (
 	"context"
 	"fmt"
+	"io"
 	"net/http"
 	"os"
 	"strings"
@@ -26,13 +27,33 @@ import (
 	"github.com/vmfunc/sif/internal/styles"
 )
 
+// maxBucketBodyReadBytes bounds how much of a bucket response we buffer to
+// look for the listing markers below. an XML/JSON listing root or an error
+// code always shows up well within this window; the rest is drained and
+// discarded by httpx.DrainClose.
+const maxBucketBodyReadBytes = 8 << 10
+
+// s3ListingMarker is the root element of a real S3 ListBucketResult XML
+// listing. its presence is what actually proves anonymous listing access;
+// AccessDenied/NoSuchBucket/redirect bodies also come back with status 200
+// and must not be mistaken for a listing.
+const s3ListingMarker = "<ListBucketResult"
+
+// s3DeniedMarkers are S3 error codes that can accompany a 200 (or that we
+// check regardless of status) and rule out a listing even if the listing
+// marker were somehow also present.
+var s3DeniedMarkers = []string{"AccessDenied", "NoSuchBucket", "PermanentRedirect", "AllAccessDisabled"}
+
 // s3EndpointFmt is a var so integration tests can repoint it at a fixture; the
 // %s is the bucket name.
 var s3EndpointFmt = "https://%s.s3.amazonaws.com"
 
 type CloudStorageResult struct {
 	BucketName string `json:"bucket_name"`
-	IsPublic   bool   `json:"is_public"`
+	// IsPublic means the bucket's listing was actually retrieved (the
+	// response body contains a ListBucketResult with no denial marker), not
+	// merely that the request returned 200.
+	IsPublic bool `json:"is_public"`
 }
 
 func CloudStorage(url string, timeout time.Duration, logdir string) ([]CloudStorageResult, error) {
@@ -115,9 +136,36 @@ func checkS3Bucket(ctx context.Context, bucket string, client *http.Client) (boo
 	if err != nil {
 		return false, err
 	}
-	// status only; drain on close so the conn returns to the pool.
+	// any remainder past maxBucketBodyReadBytes is drained on close so the
+	// conn returns to the pool.
 	defer httpx.DrainClose(resp)
 
-	// If we can access the bucket listing, it's public
-	return resp.StatusCode == http.StatusOK, nil
+	if resp.StatusCode != http.StatusOK {
+		return false, nil
+	}
+
+	body, err := io.ReadAll(io.LimitReader(resp.Body, maxBucketBodyReadBytes))
+	if err != nil {
+		return false, err
+	}
+
+	return isListableBucketBody(body), nil
+}
+
+// isListableBucketBody reports whether body proves an S3 bucket is
+// anonymously listable. a 200 status alone does not: AccessDenied pages,
+// provider landing pages, and locked buckets can all return 200, so the
+// listing root element must actually be present and no denial marker may
+// be present alongside it.
+func isListableBucketBody(body []byte) bool {
+	s := string(body)
+	if !strings.Contains(s, s3ListingMarker) {
+		return false
+	}
+	for _, marker := range s3DeniedMarkers {
+		if strings.Contains(s, marker) {
+			return false
+		}
+	}
+	return true
 }

internal/scan/cloudstorage_test.go

@@ -13,8 +13,14 @@
 package scan
 
 import (
+	"context"
+	"net/http"
+	"net/http/httptest"
 	"slices"
 	"testing"
+	"time"
+
+	"github.com/vmfunc/sif/internal/httpx"
 )
 
 func TestExtractPotentialBuckets(t *testing.T) {
@@ -60,3 +66,104 @@ func TestExtractPotentialBuckets(t *testing.T) {
 		})
 	}
 }
+
+// TestCheckS3Bucket_StatusOKAlone proves the false positive this fix closes:
+// a 200 response whose body is an AccessDenied error (or an unrelated
+// landing page) must not be reported as an anonymously listable bucket, even
+// though the bare status code says 200. against the pre-fix code (status
+// only) every one of these would have wrongly returned true.
+func TestCheckS3Bucket_StatusOKAlone(t *testing.T) {
+	tests := []struct {
+		name       string
+		statusCode int
+		body       string
+		want       bool
+	}{
+		{
+			name:       "200 with AccessDenied body is not a listing",
+			statusCode: http.StatusOK,
+			body: `<?xml version="1.0" encoding="UTF-8"?>
+<Error><Code>AccessDenied</Code><Message>Access Denied</Message></Error>`,
+			want: false,
+		},
+		{
+			name:       "200 with unrelated landing page is not a listing",
+			statusCode: http.StatusOK,
+			body:       `<html><body><h1>Welcome to example corp</h1></body></html>`,
+			want:       false,
+		},
+		{
+			name:       "200 with empty body is not a listing",
+			statusCode: http.StatusOK,
+			body:       ``,
+			want:       false,
+		},
+		{
+			name:       "403 AccessDenied is not a listing",
+			statusCode: http.StatusForbidden,
+			body: `<?xml version="1.0" encoding="UTF-8"?>
+<Error><Code>AccessDenied</Code><Message>Access Denied</Message></Error>`,
+			want: false,
+		},
+		{
+			name:       "200 with a real ListBucketResult is a listing",
+			statusCode: http.StatusOK,
+			body: `<?xml version="1.0" encoding="UTF-8"?>
+<ListBucketResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
+<Name>example-bucket</Name><Contents><Key>secret.txt</Key></Contents>
+</ListBucketResult>`,
+			want: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.WriteHeader(tt.statusCode)
+				_, _ = w.Write([]byte(tt.body))
+			}))
+			defer srv.Close()
+
+			orig := s3EndpointFmt
+			s3EndpointFmt = srv.URL + "/%s"
+			defer func() { s3EndpointFmt = orig }()
+
+			client := httpx.Client(5 * time.Second)
+			got, err := checkS3Bucket(context.Background(), "some-bucket", client)
+			if err != nil {
+				t.Fatalf("checkS3Bucket() error = %v", err)
+			}
+			if got != tt.want {
+				t.Errorf("checkS3Bucket() = %v, want %v (status=%d body=%q)", got, tt.want, tt.statusCode, tt.body)
+			}
+		})
+	}
+}
+
+func TestIsListableBucketBody(t *testing.T) {
+	tests := []struct {
+		name string
+		body string
+		want bool
+	}{
+		{
+			name: "real listing",
+			body: `<ListBucketResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Name>b</Name></ListBucketResult>`,
+			want: true,
+		},
+		{
+			name: "access denied even if listing marker somehow present",
+			body: `<ListBucketResult><Name>b</Name></ListBucketResult><Error><Code>AccessDenied</Code></Error>`,
+			want: false,
+		},
+		{name: "no marker at all", body: `<html>nothing here</html>`, want: false},
+		{name: "empty body", body: ``, want: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isListableBucketBody([]byte(tt.body)); got != tt.want {
+				t.Errorf("isListableBucketBody(%q) = %v, want %v", tt.body, got, tt.want)
+			}
+		})
+	}
+}

internal/scan/integration_test.go

@@ -371,11 +371,14 @@ func TestIntegrationSecurityTrails(t *testing.T) {
 }
 
 func TestIntegrationCloudStorage(t *testing.T) {
-	// the fixture returns 200 only for the planted bucket, so any candidate that
-	// matches it is reported public.
+	// the fixture serves a real ListBucketResult listing only for the planted
+	// bucket, so any candidate that matches it is reported public; a bare 200
+	// with no listing body is (correctly) not enough to flag one.
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Path == "/example" {
 			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`<?xml version="1.0" encoding="UTF-8"?>
+<ListBucketResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Name>example</Name><Contents><Key>f</Key></Contents></ListBucketResult>`))
 			return
 		}
 		w.WriteHeader(http.StatusNotFound)