From ec35330ebba81f834556e0ddd327e85f830c4f3a Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 8 Jul 2016 07:46:49 -0700 Subject: [PATCH] XL/fs: GetObject should validate all its inputs. (#2142) Fixes #2141 Fixes #2139 --- api-headers.go | 4 +-- fs-v1.go | 14 ++++++-- httprange.go | 80 ++++++++++++++++++++++++---------------------- httprange_test.go | 16 +++++----- object-handlers.go | 2 +- xl-v1-object.go | 14 ++++++++ 6 files changed, 79 insertions(+), 51 deletions(-) diff --git a/api-headers.go b/api-headers.go index f7fb43520..5e0743d8a 100644 --- a/api-headers.go +++ b/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 { - // override content-length + if contentRange != nil && contentRange.offsetBegin > -1 { + // Override content-length w.Header().Set("Content-Length", strconv.FormatInt(contentRange.getLength(), 10)) w.Header().Set("Content-Range", contentRange.String()) w.WriteHeader(http.StatusPartialContent) diff --git a/fs-v1.go b/fs-v1.go index 13f2b8a0d..9d4b87709 100644 --- a/fs-v1.go +++ b/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 { - return InvalidRange{} + // Reply back invalid range if the input offset and length fall out of range. + 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 diff --git a/httprange.go b/httprange.go index 23c898554..45ef90814 100644 --- a/httprange.go +++ b/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 - lastBytePos int64 - size int64 + offsetBegin int64 + offsetEnd 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] - firstBytePos := int64(-1) - // Convert firstBytePosString only if its not empty. - if len(firstBytePosString) > 0 { - if !validBytePos.MatchString(firstBytePosString) { + offsetBeginString := byteRangeString[:sepIndex] + offsetBegin := int64(-1) + // Convert offsetBeginString only if its not empty. + if len(offsetBeginString) > 0 { + 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:] - lastBytePos := int64(-1) - // Convert lastBytePosString only if its not empty. - if len(lastBytePosString) > 0 { - if !validBytePos.MatchString(lastBytePosString) { + offsetEndString := byteRangeString[sepIndex+1:] + offsetEnd := int64(-1) + // Convert offsetEndString only if its not empty. + if len(offsetEndString) > 0 { + 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 firstBytePos > lastBytePos { + if offsetBegin > -1 && offsetEnd > -1 { + 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. - if firstBytePos >= size { + // First and last byte positions should not be >= resourceSize. + if offsetBegin >= resourceSize { return nil, errInvalidRange } - if lastBytePos >= size { - lastBytePos = size - 1 + if offsetEnd >= resourceSize { + offsetEnd = resourceSize - 1 } - } else if firstBytePos > -1 { + } else if offsetBegin > -1 { // rangeString contains only first byte position. eg. "bytes=8-" - if firstBytePos >= size { - // First byte position should not be >= size. + if offsetBegin >= resourceSize { + // First byte position should not be >= resourceSize. return nil, errInvalidRange } - lastBytePos = size - 1 - } else if lastBytePos > -1 { + offsetEnd = resourceSize - 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 { - firstBytePos = 0 + if offsetEnd >= resourceSize { + 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 } diff --git a/httprange_test.go b/httprange_test.go index 5144be8ef..78eaa9a4b 100644 --- a/httprange_test.go +++ b/httprange_test.go @@ -22,10 +22,10 @@ import "testing" func TestParseRequestRange(t *testing.T) { // Test success cases. successCases := []struct { - rangeString string - firstBytePos int64 - lastBytePos int64 - length int64 + rangeString string + offsetBegin int64 + offsetEnd 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: , got: %s", err) } - if hrange.firstBytePos != successCase.firstBytePos { - t.Fatalf("expected: %d, got: %d", successCase.firstBytePos, hrange.firstBytePos) + if hrange.offsetBegin != successCase.offsetBegin { + t.Fatalf("expected: %d, got: %d", successCase.offsetBegin, hrange.offsetBegin) } - if hrange.lastBytePos != successCase.lastBytePos { - t.Fatalf("expected: %d, got: %d", successCase.lastBytePos, hrange.lastBytePos) + if hrange.offsetEnd != successCase.offsetEnd { + 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()) diff --git a/object-handlers.go b/object-handlers.go index 5c84b0137..ef5a9c8e1 100644 --- a/object-handlers.go +++ b/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. diff --git a/xl-v1-object.go b/xl-v1-object.go index 2d06f3e86..4c28bb284 100644 --- a/xl-v1-object.go +++ b/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 {