do not allow for unbounded reads for user controlled input (#681)

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

Author: wagoodman

.golangci.yaml

@@ -50,6 +50,10 @@ linters:
       - linters:
           - revive
         text: "var-naming: avoid package names that conflict"
+      # utils is a commonly used helper package name
+      - linters:
+          - revive
+        text: "var-naming: avoid meaningless package names"
     paths:
       - third_party$
       - builtin$

internal/utils/indent.go

@@ -1,4 +1,4 @@
-package utils //nolint:revive // existing package name
+package utils
 
 import "strings"
 

internal/utils/io.go

@@ -0,0 +1,19 @@
+package utils
+
+import (
+	"fmt"
+	"io"
+)
+
+// ReadAllLimited reads up to maxBytes from r. Returns error if limit exceeded.
+func ReadAllLimited(r io.Reader, maxBytes int64) ([]byte, error) {
+	limitedReader := io.LimitReader(r, maxBytes+1)
+	data, err := io.ReadAll(limitedReader)
+	if err != nil {
+		return nil, err
+	}
+	if int64(len(data)) > maxBytes {
+		return nil, fmt.Errorf("response size exceeds limit of %d bytes", maxBytes)
+	}
+	return data, nil
+}

internal/utils/io_test.go

@@ -0,0 +1,91 @@
+package utils
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestReadAllLimited(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		maxBytes int64
+		want     string
+		wantErr  require.ErrorAssertionFunc
+	}{
+		{
+			name:     "reads data under limit",
+			input:    "hello world",
+			maxBytes: 100,
+			want:     "hello world",
+		},
+		{
+			name:     "reads data exactly at limit",
+			input:    "hello",
+			maxBytes: 5,
+			want:     "hello",
+		},
+		{
+			name:     "returns error when data exceeds limit",
+			input:    "hello world",
+			maxBytes: 5,
+			wantErr:  require.Error,
+		},
+		{
+			name:     "handles empty input",
+			input:    "",
+			maxBytes: 100,
+			want:     "",
+		},
+		{
+			name:     "handles zero limit with empty input",
+			input:    "",
+			maxBytes: 0,
+			want:     "",
+		},
+		{
+			name:     "returns error for zero limit with data",
+			input:    "a",
+			maxBytes: 0,
+			wantErr:  require.Error,
+		},
+		{
+			name:     "handles large data at boundary minus one",
+			input:    strings.Repeat("x", 999),
+			maxBytes: 1000,
+			want:     strings.Repeat("x", 999),
+		},
+		{
+			name:     "handles large data at exact boundary",
+			input:    strings.Repeat("x", 1000),
+			maxBytes: 1000,
+			want:     strings.Repeat("x", 1000),
+		},
+		{
+			name:     "returns error for large data over boundary",
+			input:    strings.Repeat("x", 1001),
+			maxBytes: 1000,
+			wantErr:  require.Error,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.wantErr == nil {
+				tt.wantErr = require.NoError
+			}
+
+			reader := bytes.NewReader([]byte(tt.input))
+			got, err := ReadAllLimited(reader, tt.maxBytes)
+			tt.wantErr(t, err)
+
+			if err != nil {
+				return
+			}
+			require.Equal(t, tt.want, string(got))
+		})
+	}
+}

quill/notary/api_client.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"io"
 	"net/http"
 	"net/url"
 	"path"
@@ -22,6 +21,12 @@ import (
 	"github.com/anchore/quill/internal/log"
 	"github.com/anchore/quill/internal/redact"
 	"github.com/anchore/quill/internal/urlvalidate"
+	"github.com/anchore/quill/internal/utils"
+)
+
+const (
+	maxAPIResponseSize = 5 * 1024 * 1024  // 5 MB for API JSON responses
+	maxLogResponseSize = 50 * 1024 * 1024 // 50 MB for log files
 )
 
 type api interface {
@@ -63,7 +68,7 @@ func (s APIClient) submissionRequest(ctx context.Context, request submissionRequ
 		return nil, err
 	}
 
-	response, err := s.http.post(ctx, s.api, bytes.NewReader(requestBytes))
+	response, err := s.http.post(ctx, s.api, bytes.NewReader(requestBytes)) //nolint:bodyclose // body is closed in handleResponse
 	body, err := s.handleResponse(response, err)
 	if err != nil {
 		return nil, err
@@ -119,7 +124,7 @@ func (s APIClient) uploadBinary(ctx context.Context, response submissionResponse
 }
 
 func (s APIClient) submissionStatusRequest(ctx context.Context, id string) (*submissionStatusResponse, error) {
-	response, err := s.http.get(ctx, joinURL(s.api, id), nil)
+	response, err := s.http.get(ctx, joinURL(s.api, id), nil) //nolint:bodyclose // body is closed in handleResponse
 	body, err := s.handleResponse(response, err)
 	if err != nil {
 		return nil, err
@@ -133,7 +138,7 @@ func (s APIClient) submissionStatusRequest(ctx context.Context, id string) (*sub
 }
 
 func (s APIClient) submissionList(ctx context.Context) (*submissionListResponse, error) {
-	response, err := s.http.get(ctx, s.api, nil)
+	response, err := s.http.get(ctx, s.api, nil) //nolint:bodyclose // body is closed in handleResponse
 	body, err := s.handleResponse(response, err)
 	if err != nil {
 		return nil, err
@@ -147,7 +152,7 @@ func (s APIClient) submissionList(ctx context.Context) (*submissionListResponse,
 }
 
 func (s APIClient) submissionLogs(ctx context.Context, id string) (string, error) {
-	metadataResp, err := s.http.get(ctx, joinURL(s.api, id, "logs"), nil)
+	metadataResp, err := s.http.get(ctx, joinURL(s.api, id, "logs"), nil) //nolint:bodyclose // body is closed in handleResponse
 	body, err := s.handleResponse(metadataResp, err)
 	if err != nil {
 		return "", fmt.Errorf("unable to fetch log metadata with ID=%s: %w", id, err)
@@ -160,9 +165,10 @@ func (s APIClient) submissionLogs(ctx context.Context, id string) (string, error
 
 	redactPresignedURLParams(resp.Data.Attributes.DeveloperLogURL)
 
-	// fetch logs without auth header (it's a presigned URL with its own auth)
+	// fetch logs without auth (presigned URL), with redirect validation for SSRF protection.
+	// use a larger size limit since log files can be bigger than typical API responses.
 	logsResp, err := s.http.getUnauthenticated(ctx, resp.Data.Attributes.DeveloperLogURL)
-	contents, err := s.handleResponse(logsResp, err)
+	contents, err := s.handleResponseWithLimit(logsResp, err, maxLogResponseSize)
 	if err != nil {
 		return "", fmt.Errorf("unable to fetch log destination with ID=%s: %w", id, err)
 	}
@@ -171,16 +177,28 @@ func (s APIClient) submissionLogs(ctx context.Context, id string) (string, error
 }
 
 func (s APIClient) handleResponse(response *http.Response, err error) ([]byte, error) {
+	return s.handleResponseWithLimit(response, err, maxAPIResponseSize)
+}
+
+func (s APIClient) handleResponseWithLimit(response *http.Response, err error, maxBytes int64) ([]byte, error) {
+	// ensure body is always closed, even if there's an error
+	if response != nil && response.Body != nil {
+		defer response.Body.Close()
+	}
+
 	if err != nil {
 		return nil, err
 	}
 
+	if response == nil {
+		return nil, fmt.Errorf("nil response")
+	}
+
 	var body []byte
 
 	if response.Body != nil {
-		defer response.Body.Close()
-
-		body, err = io.ReadAll(response.Body)
+		// limit response size to prevent memory exhaustion from malicious responses
+		body, err = utils.ReadAllLimited(response.Body, maxBytes)
 		if err != nil {
 			return nil, err
 		}

quill/notary/api_client_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
@@ -195,3 +196,151 @@ func Test_apiClient_submissionLogs_rejectsDeniedURLs(t *testing.T) {
 		})
 	}
 }
+
+func Test_apiClient_handleResponse_enforcesMaxSize(t *testing.T) {
+	tests := []struct {
+		name        string
+		size        int
+		wantErr     require.ErrorAssertionFunc
+		errContains string
+	}{
+		{
+			name: "accepts response under limit",
+			size: 1024, // 1 KB
+		},
+		{
+			name: "accepts response at limit",
+			size: maxAPIResponseSize,
+		},
+		{
+			name:        "rejects response over limit",
+			size:        maxAPIResponseSize + 1,
+			wantErr:     require.Error,
+			errContains: "exceeds limit",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.wantErr == nil {
+				tt.wantErr = require.NoError
+			}
+
+			mux := http.NewServeMux()
+			mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+				// write a response of the specified size
+				data := strings.Repeat("x", tt.size)
+				w.Write([]byte(data))
+			})
+
+			s := httptest.NewServer(mux)
+			defer s.Close()
+
+			resp, err := http.Get(s.URL)
+			require.NoError(t, err)
+
+			c := APIClient{}
+			_, err = c.handleResponse(resp, nil)
+			tt.wantErr(t, err)
+
+			if tt.errContains != "" {
+				require.Contains(t, err.Error(), tt.errContains)
+			}
+		})
+	}
+}
+
+func Test_apiClient_handleResponseWithLimit_enforcesCustomLimit(t *testing.T) {
+	customLimit := int64(100)
+
+	tests := []struct {
+		name        string
+		size        int
+		wantErr     require.ErrorAssertionFunc
+		errContains string
+	}{
+		{
+			name: "accepts response under custom limit",
+			size: 50,
+		},
+		{
+			name: "accepts response at custom limit",
+			size: int(customLimit),
+		},
+		{
+			name:        "rejects response over custom limit",
+			size:        int(customLimit) + 1,
+			wantErr:     require.Error,
+			errContains: "exceeds limit",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.wantErr == nil {
+				tt.wantErr = require.NoError
+			}
+
+			mux := http.NewServeMux()
+			mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+				data := strings.Repeat("x", tt.size)
+				w.Write([]byte(data))
+			})
+
+			s := httptest.NewServer(mux)
+			defer s.Close()
+
+			resp, err := http.Get(s.URL)
+			require.NoError(t, err)
+
+			c := APIClient{}
+			_, err = c.handleResponseWithLimit(resp, nil, customLimit)
+			tt.wantErr(t, err)
+
+			if tt.errContains != "" {
+				require.Contains(t, err.Error(), tt.errContains)
+			}
+		})
+	}
+}
+
+func Test_apiClient_submissionLogs_usesLargerLimit(t *testing.T) {
+	// create a log response larger than maxAPIResponseSize but smaller than maxLogResponseSize
+	logSize := maxAPIResponseSize + 1024 // just over the API limit
+	require.True(t, logSize < maxLogResponseSize, "test assumes logSize < maxLogResponseSize")
+
+	id := "the-id"
+	expectedLogResponse := submissionLogsResponse{
+		Data: submissionLogsResponseData{
+			submissionResponseDescriptor: submissionResponseDescriptor{
+				Type: "the-ty",
+				ID:   id,
+			},
+		},
+	}
+
+	mux := http.NewServeMux()
+	mux.HandleFunc("/"+id+"/logs", func(w http.ResponseWriter, r *http.Request) {
+		by, err := json.Marshal(expectedLogResponse)
+		require.NoError(t, err)
+		w.Write(by)
+	})
+
+	mux.HandleFunc("/place-where-the-logs-are", func(w http.ResponseWriter, r *http.Request) {
+		// write a large log response
+		data := strings.Repeat("x", logSize)
+		w.Write([]byte(data))
+	})
+
+	s := httptest.NewServer(mux)
+	expectedLogResponse.Data.Attributes.DeveloperLogURL = s.URL + "/place-where-the-logs-are"
+	defer s.Close()
+
+	c := newTestAPIClient("the-token", time.Second*30)
+	c.api = s.URL
+
+	// this should succeed because logs use maxLogResponseSize, not maxAPIResponseSize
+	actual, err := c.submissionLogs(context.Background(), id)
+	require.NoError(t, err)
+	require.Len(t, actual, logSize)
+}