Merge pull request #1675 from versity/sis/unsigned-streaming-payload-chunk-size

fix: adds validation for chunk sizes in unsigned streaming trailer upload

s3api/utils/unsigned-chunk-reader.go

@@ -35,8 +35,9 @@ import (
 )
 
 var (
-	trailerDelim         = []byte{'\n', '\r', '\n'}
-	errMalformedEncoding = errors.New("malformed chunk encoding")
+	trailerDelim               = []byte{'\n', '\r', '\n'}
+	minChunkSize         int64 = 8192
+	errMalformedEncoding       = errors.New("malformed chunk encoding")
 )
 
 type UnsignedChunkReader struct {
@@ -46,6 +47,9 @@ type UnsignedChunkReader struct {
 	hasher         hash.Hash
 	stash          []byte
 	offset         int
+	// this data is necessary for 'InvalidChunkSizeError' error
+	// TODO: add 'Chunk' and 'BadChunkSize' in the error
+	chunkSizes []int64
 }
 
 func NewUnsignedChunkReader(r io.Reader, ct checksumType) (*UnsignedChunkReader, error) {
@@ -65,6 +69,7 @@ func NewUnsignedChunkReader(r io.Reader, ct checksumType) (*UnsignedChunkReader,
 		checksumType: ct,
 		stash:        make([]byte, 0),
 		hasher:       hasher,
+		chunkSizes:   []int64{},
 	}, nil
 }
 
@@ -188,11 +193,36 @@ func (ucr *UnsignedChunkReader) extractChunkSize() (int64, error) {
 		return 0, s3err.GetAPIError(s3err.ErrIncompleteBody)
 	}
 
+	if !ucr.isValidChunkSize(chunkSize) {
+		return chunkSize, s3err.GetAPIError(s3err.ErrInvalidChunkSize)
+	}
+
+	ucr.chunkSizes = append(ucr.chunkSizes, chunkSize)
+
 	debuglogger.Infof("chunk size extracted: %v", chunkSize)
 
 	return chunkSize, nil
 }
 
+// isValidChunkSize checks if the parsed chunk size is valid
+// they follow one rule: all chunk sizes except for the last one
+// should be greater than 8192
+func (ucr *UnsignedChunkReader) isValidChunkSize(size int64) bool {
+	if len(ucr.chunkSizes) == 0 {
+		// any valid number is valid as a first chunk size
+		return true
+	}
+
+	lastChunkSize := ucr.chunkSizes[len(ucr.chunkSizes)-1]
+	// any chunk size, except the last one should be greater than 8192
+	if size != 0 && lastChunkSize < minChunkSize {
+		debuglogger.Logf("invalid chunk size %v", lastChunkSize)
+		return false
+	}
+
+	return true
+}
+
 // Reads and validates the trailer at the end
 func (ucr *UnsignedChunkReader) readTrailer() error {
 	var trailerBuffer bytes.Buffer

s3err/s3err.go

@@ -180,6 +180,7 @@ const (
 	ErrInvalidLocationConstraint
 	ErrInvalidArgument
 	ErrMalformedTrailer
+	ErrInvalidChunkSize
 
 	// Non-AWS errors
 	ErrExistingObjectIsDirectory
@@ -804,6 +805,11 @@ var errorCodeResponse = map[ErrorCode]APIError{
 		Description:    "The request contained trailing data that was not well-formed or did not conform to our published schema.",
 		HTTPStatusCode: http.StatusBadRequest,
 	},
+	ErrInvalidChunkSize: {
+		Code:           "InvalidChunkSizeError",
+		Description:    "Only the last chunk is allowed to have a size less than 8192 bytes",
+		HTTPStatusCode: http.StatusBadRequest,
+	},
 
 	// non aws errors
 	ErrExistingObjectIsDirectory: {

tests/integration/group-tests.go

@@ -1101,6 +1101,7 @@ func TestUnsignedStreaminPayloadTrailer(ts *TestState) {
 		ts.Run(UnsignedStreamingPayloadTrailer_multiple_checksum_headers)
 		ts.Run(UnsignedStreamingPayloadTrailer_sdk_algo_and_trailer_mismatch)
 		ts.Run(UnsignedStreamingPayloadTrailer_incomplete_body)
+		ts.Run(UnsignedStreamingPayloadTrailer_invalid_chunk_size)
 		ts.Run(UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme)
 		ts.Run(UnsignedStreamingPayloadTrailer_no_payload_trailer_only_headers)
 		ts.Run(UnsignedStreamingPayloadTrailer_success_both_sdk_algo_and_trailer)
@@ -1754,6 +1755,7 @@ func GetIntTests() IntTests {
 		"UnsignedStreamingPayloadTrailer_multiple_checksum_headers":                UnsignedStreamingPayloadTrailer_multiple_checksum_headers,
 		"UnsignedStreamingPayloadTrailer_sdk_algo_and_trailer_mismatch":            UnsignedStreamingPayloadTrailer_sdk_algo_and_trailer_mismatch,
 		"UnsignedStreamingPayloadTrailer_incomplete_body":                          UnsignedStreamingPayloadTrailer_incomplete_body,
+		"UnsignedStreamingPayloadTrailer_invalid_chunk_size":                       UnsignedStreamingPayloadTrailer_invalid_chunk_size,
 		"UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme":    UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme,
 		"UnsignedStreamingPayloadTrailer_no_payload_trailer_only_headers":          UnsignedStreamingPayloadTrailer_no_payload_trailer_only_headers,
 		"UnsignedStreamingPayloadTrailer_success_both_sdk_algo_and_trailer":        UnsignedStreamingPayloadTrailer_success_both_sdk_algo_and_trailer,

tests/integration/unsigned_streaming_payload_trailer.go

@@ -211,6 +211,63 @@ func UnsignedStreamingPayloadTrailer_incomplete_body(s *S3Conf) error {
 	})
 }
 
+func UnsignedStreamingPayloadTrailer_invalid_chunk_size(s *S3Conf) error {
+	testName := "UnsignedStreamingPayloadTrailer_invalid_chunk_size"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		object := "my-object"
+		chSizeErr := s3err.GetAPIError(s3err.ErrInvalidChunkSize)
+		for i, test := range []struct {
+			chunkSizes  []int64
+			expectedErr error
+		}{
+			{[]int64{8192, 8192, 100, 0}, nil},
+			{[]int64{100, 0}, nil},
+			{[]int64{10000, 9500, 20, 0}, nil},
+			{[]int64{8200, 8201, 10000, 10, 0}, nil},
+			{[]int64{100, 8192, 100, 0}, chSizeErr},
+			{[]int64{8192, 10, 100, 0}, chSizeErr},
+			{[]int64{10, 10, 10, 0}, chSizeErr},
+			{[]int64{8192, 10, 100, 0}, chSizeErr},
+			{[]int64{10000, 10, 8192, 8192, 0}, chSizeErr},
+			{[]int64{8192, 10, 10, 8192, 20, 20000, 0}, chSizeErr},
+		} {
+			cLength, payload, err := constructUnsignedPaylod(test.chunkSizes...)
+			if err != nil {
+				return fmt.Errorf("test %v failed: %w", i+1, err)
+			}
+
+			reqHeaders := map[string]string{
+				"x-amz-decoded-content-length": fmt.Sprint(cLength),
+			}
+
+			_, apiErr, err := testUnsignedStreamingPayloadTrailerObjectPut(s, bucket, object, payload, reqHeaders)
+			if err != nil {
+				return fmt.Errorf("test %v failed: %w", i+1, err)
+			}
+
+			if test.expectedErr == nil && apiErr != nil {
+				return fmt.Errorf("test %v failed: (%s) %s", i+1, apiErr.Code, apiErr.Message)
+			}
+
+			if test.expectedErr != nil {
+				if apiErr == nil {
+					return fmt.Errorf("test %v failed: expected %w, instead got nil", i+1, test.expectedErr)
+				}
+
+				expErr, ok := test.expectedErr.(s3err.APIError)
+				if !ok {
+					return fmt.Errorf("test %v failed: invalid expected error type", i+1)
+				}
+
+				if err := compareS3ApiError(expErr, apiErr); err != nil {
+					return fmt.Errorf("test %v failed: %w", i+1, err)
+				}
+			}
+		}
+		return nil
+	})
+}
+
 func UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme(s *S3Conf) error {
 	testName := "UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {

tests/integration/utils.go

@@ -2093,3 +2093,28 @@ func testUnsignedStreamingPayloadTrailerUploadPart(s *S3Conf, bucket, object str
 
 	return sendSignedRequest(s, req, cancel)
 }
+
+// constructUnsignedPaylod constructs an unsigned streaming upload payload
+// and returns the decoded content length and the payload
+func constructUnsignedPaylod(chunkSizes ...int64) (int64, []byte, error) {
+	var cLength int64
+	buffer := bytes.NewBuffer([]byte{})
+
+	for _, chunkSize := range chunkSizes {
+		cLength += chunkSize
+		_, err := buffer.WriteString(fmt.Sprintf("%x\r\n", chunkSize))
+		if err != nil {
+			return 0, nil, err
+		}
+		_, err = buffer.WriteString(strings.Repeat("a", int(chunkSize)))
+		if err != nil {
+			return 0, nil, err
+		}
+		_, err = buffer.WriteString("\r\n")
+		if err != nil {
+			return 0, nil, err
+		}
+	}
+
+	return cLength, buffer.Bytes(), nil
+}