XL/fs: GetObject should validate all its inputs. (#2142)

Fixes #2141
Fixes #2139

api-headers.go

@@ -74,8 +74,8 @@ func setObjectHeaders(w http.ResponseWriter, objInfo ObjectInfo, contentRange *h
 	w.Header().Set("Content-Length", strconv.FormatInt(objInfo.Size, 10))
 
 	// for providing ranged content
-	if contentRange != nil && contentRange.firstBytePos > -1 {
+	if contentRange != nil && contentRange.offsetBegin > -1 {
-		// override content-length
+		// Override content-length
 		w.Header().Set("Content-Length", strconv.FormatInt(contentRange.getLength(), 10))
 		w.Header().Set("Content-Range", contentRange.String())
 		w.WriteHeader(http.StatusPartialContent)

fs-v1.go

@@ -221,14 +221,24 @@ func (fs fsObjects) GetObject(bucket, object string, offset int64, length int64,
 	if offset < 0 || length < 0 {
 		return toObjectErr(errUnexpected, bucket, object)
 	}
+	// Writer cannot be nil.
+	if writer == nil {
+		return toObjectErr(errUnexpected, bucket, object)
+	}
 
+	// Stat the file to get file size.
 	fi, err := fs.storage.StatFile(bucket, object)
 	if err != nil {
 		return toObjectErr(err, bucket, object)
 	}
 
-	if offset > fi.Size {
+	// Reply back invalid range if the input offset and length fall out of range.
-		return InvalidRange{}
+	if offset > fi.Size || length > fi.Size {
+		return InvalidRange{offset, length, fi.Size}
+	}
+	// Reply if we have inputs with offset and length falling out of file size range.
+	if offset+length > fi.Size {
+		return InvalidRange{offset, length, fi.Size}
 	}
 
 	var totalLeft = length

httprange.go

@@ -32,10 +32,14 @@ const (
 var errInvalidRange = errors.New("Invalid range")
 
 // InvalidRange - invalid range typed error.
-type InvalidRange struct{}
+type InvalidRange struct {
+	offsetBegin  int64
+	offsetEnd    int64
+	resourceSize int64
+}
 
 func (e InvalidRange) Error() string {
-	return "The requested range is not satisfiable"
+	return fmt.Sprintf("The requested range \"bytes %d-%d/%d\" is not satisfiable.", e.offsetBegin, e.offsetEnd, e.resourceSize)
 }
 
 // Valid byte position regexp
@@ -43,22 +47,22 @@ var validBytePos = regexp.MustCompile(`^[0-9]+$`)
 
 // HttpRange specifies the byte range to be sent to the client.
 type httpRange struct {
-	firstBytePos int64
+	offsetBegin  int64
-	lastBytePos  int64
+	offsetEnd    int64
-	size         int64
+	resourceSize int64
 }
 
 // String populate range stringer interface
 func (hrange httpRange) String() string {
-	return fmt.Sprintf("bytes %d-%d/%d", hrange.firstBytePos, hrange.lastBytePos, hrange.size)
+	return fmt.Sprintf("bytes %d-%d/%d", hrange.offsetBegin, hrange.offsetEnd, hrange.resourceSize)
 }
 
-// getLength - get length from the range.
+// getlength - get length from the range.
 func (hrange httpRange) getLength() int64 {
-	return 1 + hrange.lastBytePos - hrange.firstBytePos
+	return 1 + hrange.offsetEnd - hrange.offsetBegin
 }
 
-func parseRequestRange(rangeString string, size int64) (hrange *httpRange, err error) {
+func parseRequestRange(rangeString string, resourceSize int64) (hrange *httpRange, err error) {
 	// Return error if given range string doesn't start with byte range prefix.
 	if !strings.HasPrefix(rangeString, byteRangePrefix) {
 		return nil, fmt.Errorf("'%s' does not start with '%s'", rangeString, byteRangePrefix)
@@ -73,73 +77,73 @@ func parseRequestRange(rangeString string, size int64) (hrange *httpRange, err e
 		return nil, fmt.Errorf("'%s' does not have a valid range value", rangeString)
 	}
 
-	firstBytePosString := byteRangeString[:sepIndex]
+	offsetBeginString := byteRangeString[:sepIndex]
-	firstBytePos := int64(-1)
+	offsetBegin := int64(-1)
-	// Convert firstBytePosString only if its not empty.
+	// Convert offsetBeginString only if its not empty.
-	if len(firstBytePosString) > 0 {
+	if len(offsetBeginString) > 0 {
-		if !validBytePos.MatchString(firstBytePosString) {
+		if !validBytePos.MatchString(offsetBeginString) {
 			return nil, fmt.Errorf("'%s' does not have a valid first byte position value", rangeString)
 		}
 
-		if firstBytePos, err = strconv.ParseInt(firstBytePosString, 10, 64); err != nil {
+		if offsetBegin, err = strconv.ParseInt(offsetBeginString, 10, 64); err != nil {
 			return nil, fmt.Errorf("'%s' does not have a valid first byte position value", rangeString)
 		}
 	}
 
-	lastBytePosString := byteRangeString[sepIndex+1:]
+	offsetEndString := byteRangeString[sepIndex+1:]
-	lastBytePos := int64(-1)
+	offsetEnd := int64(-1)
-	// Convert lastBytePosString only if its not empty.
+	// Convert offsetEndString only if its not empty.
-	if len(lastBytePosString) > 0 {
+	if len(offsetEndString) > 0 {
-		if !validBytePos.MatchString(lastBytePosString) {
+		if !validBytePos.MatchString(offsetEndString) {
 			return nil, fmt.Errorf("'%s' does not have a valid last byte position value", rangeString)
 		}
 
-		if lastBytePos, err = strconv.ParseInt(lastBytePosString, 10, 64); err != nil {
+		if offsetEnd, err = strconv.ParseInt(offsetEndString, 10, 64); err != nil {
 			return nil, fmt.Errorf("'%s' does not have a valid last byte position value", rangeString)
 		}
 	}
 
 	// rangeString contains first and last byte positions. eg. "bytes=2-5"
-	if firstBytePos > -1 && lastBytePos > -1 {
+	if offsetBegin > -1 && offsetEnd > -1 {
-		if firstBytePos > lastBytePos {
+		if offsetBegin > offsetEnd {
 			// Last byte position is not greater than first byte position. eg. "bytes=5-2"
 			return nil, fmt.Errorf("'%s' does not have valid range value", rangeString)
 		}
 
-		// First and last byte positions should not be >= size.
+		// First and last byte positions should not be >= resourceSize.
-		if firstBytePos >= size {
+		if offsetBegin >= resourceSize {
 			return nil, errInvalidRange
 		}
 
-		if lastBytePos >= size {
+		if offsetEnd >= resourceSize {
-			lastBytePos = size - 1
+			offsetEnd = resourceSize - 1
 		}
-	} else if firstBytePos > -1 {
+	} else if offsetBegin > -1 {
 		// rangeString contains only first byte position. eg. "bytes=8-"
-		if firstBytePos >= size {
+		if offsetBegin >= resourceSize {
-			// First byte position should not be >= size.
+			// First byte position should not be >= resourceSize.
 			return nil, errInvalidRange
 		}
 
-		lastBytePos = size - 1
+		offsetEnd = resourceSize - 1
-	} else if lastBytePos > -1 {
+	} else if offsetEnd > -1 {
 		// rangeString contains only last byte position. eg. "bytes=-3"
-		if lastBytePos == 0 {
+		if offsetEnd == 0 {
 			// Last byte position should not be zero eg. "bytes=-0"
 			return nil, errInvalidRange
 		}
 
-		if lastBytePos >= size {
+		if offsetEnd >= resourceSize {
-			firstBytePos = 0
+			offsetBegin = 0
 		} else {
-			firstBytePos = size - lastBytePos
+			offsetBegin = resourceSize - offsetEnd
 		}
 
-		lastBytePos = size - 1
+		offsetEnd = resourceSize - 1
 	} else {
 		// rangeString contains first and last byte positions missing. eg. "bytes=-"
 		return nil, fmt.Errorf("'%s' does not have valid range value", rangeString)
 	}
 
-	return &httpRange{firstBytePos, lastBytePos, size}, nil
+	return &httpRange{offsetBegin, offsetEnd, resourceSize}, nil
 }

httprange_test.go

@@ -22,10 +22,10 @@ import "testing"
 func TestParseRequestRange(t *testing.T) {
 	// Test success cases.
 	successCases := []struct {
-		rangeString  string
+		rangeString string
-		firstBytePos int64
+		offsetBegin int64
-		lastBytePos  int64
+		offsetEnd   int64
-		length       int64
+		length      int64
 	}{
 		{"bytes=2-5", 2, 5, 4},
 		{"bytes=2-20", 2, 9, 8},
@@ -42,12 +42,12 @@ func TestParseRequestRange(t *testing.T) {
 			t.Fatalf("expected: <nil>, got: %s", err)
 		}
 
-		if hrange.firstBytePos != successCase.firstBytePos {
+		if hrange.offsetBegin != successCase.offsetBegin {
-			t.Fatalf("expected: %d, got: %d", successCase.firstBytePos, hrange.firstBytePos)
+			t.Fatalf("expected: %d, got: %d", successCase.offsetBegin, hrange.offsetBegin)
 		}
 
-		if hrange.lastBytePos != successCase.lastBytePos {
+		if hrange.offsetEnd != successCase.offsetEnd {
-			t.Fatalf("expected: %d, got: %d", successCase.lastBytePos, hrange.lastBytePos)
+			t.Fatalf("expected: %d, got: %d", successCase.offsetEnd, hrange.offsetEnd)
 		}
 		if hrange.getLength() != successCase.length {
 			t.Fatalf("expected: %d, got: %d", successCase.length, hrange.getLength())

object-handlers.go

@@ -138,7 +138,7 @@ func (api objectAPIHandlers) GetObjectHandler(w http.ResponseWriter, r *http.Req
 	startOffset := int64(0)
 	length := objInfo.Size
 	if hrange != nil {
-		startOffset = hrange.firstBytePos
+		startOffset = hrange.offsetBegin
 		length = hrange.getLength()
 	}
 	// Reads the object at startOffset and writes to mw.

xl-v1-object.go

@@ -52,6 +52,10 @@ func (xl xlObjects) GetObject(bucket, object string, startOffset int64, length i
 	if startOffset < 0 || length < 0 {
 		return toObjectErr(errUnexpected, bucket, object)
 	}
+	// Writer cannot be nil.
+	if writer == nil {
+		return toObjectErr(errUnexpected, bucket, object)
+	}
 	// Lock the object before reading.
 	nsMutex.RLock(bucket, object)
 	defer nsMutex.RUnlock(bucket, object)
@@ -78,6 +82,16 @@ func (xl xlObjects) GetObject(bucket, object string, startOffset int64, length i
 		}
 	}
 
+	// Reply back invalid range if the input offset and length fall out of range.
+	if startOffset > xlMeta.Stat.Size || length > xlMeta.Stat.Size {
+		return InvalidRange{startOffset, length, xlMeta.Stat.Size}
+	}
+
+	// Reply if we have inputs with offset and length.
+	if startOffset+length > xlMeta.Stat.Size {
+		return InvalidRange{startOffset, length, xlMeta.Stat.Size}
+	}
+
 	// Get start part index and offset.
 	partIndex, partOffset, err := xlMeta.ObjectToPartOffset(startOffset)
 	if err != nil {