Merge pull request from GHSA-vj2m-9f5j-mpr5

* Add failing test for crash * Bounds check the range and add a couple more test cases * Fail if range start is negative * Safer parsing of integer additions * Check requested size for range header * Check for 0 * Log errors and catch invalid range headers * One more log
vapor · May 31, 2022 · 953a349 · 953a349
1 parent 354fd15
commit 953a349
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 3 deletions.
diff --git a/Sources/Vapor/Utilities/FileIO.swift b/Sources/Vapor/Utilities/FileIO.swift
@@ -1,4 +1,5 @@
 import NIO
+import Logging
 
 extension Request {
     public var fileio: FileIO {
@@ -138,6 +139,11 @@ public struct FileIO {
             } else {
                 contentRange = nil
             }
+        } else if request.headers.contains(name: .range) {
+            // Range header was supplied but could not be parsed i.e. it was invalid
+            request.logger.debug("Range header was provided in request but was invalid")
+            let response = Response(status: .badRequest)
+            return response
         } else {
             contentRange = nil
         }
@@ -163,7 +169,12 @@ public struct FileIO {
             if let firstRange = contentRange.ranges.first {
                 let range = firstRange.asResponseContentRange(limit: fileSize)
                 response.headers.contentRange = HTTPHeaders.ContentRange(unit: contentRange.unit, range: range)
-                (offset, byteCount) = firstRange.asByteBufferBounds(withMaxSize: fileSize)
+                do {
+                    (offset, byteCount) = try firstRange.asByteBufferBounds(withMaxSize: fileSize, logger: request.logger)
+                } catch {
+                    let response = Response(status: .badRequest)
+                    return response
+                }
             } else {
                 offset = 0
                 byteCount = fileSize
@@ -252,14 +263,31 @@ public struct FileIO {
 
 extension HTTPHeaders.Range.Value {
 
-    fileprivate func asByteBufferBounds(withMaxSize size: Int) -> (offset: Int64, byteCount: Int) {
+    fileprivate func asByteBufferBounds(withMaxSize size: Int, logger: Logger) throws -> (offset: Int64, byteCount: Int) {
         switch self {
             case .start(let value):
+                guard value <= size, value >= 0 else {
+                    logger.debug("Requested range start was invalid: \(value)")
+                    throw Abort(.badRequest)
+                }
                 return (offset: numericCast(value), byteCount: size - value)
             case .tail(let value):
+                guard value <= size, value >= 0 else {
+                    logger.debug("Requested range end was invalid: \(value)")
+                    throw Abort(.badRequest)
+                }
                 return (offset: numericCast(size - value), byteCount: value)
             case .within(let start, let end):
-                return (offset: numericCast(start), byteCount: end - start + 1)
+                guard start >= 0, end >= 0, start < end else {
+                    logger.debug("Requested range was invalid: \(start)-\(end)")
+                    throw Abort(.badRequest)
+                }
+                let (byteCount, overflow) =  (end - start).addingReportingOverflow(1)
+                guard !overflow else {
+                    logger.debug("Requested range was invalid: \(start)-\(end)")
+                    throw Abort(.badRequest)
+                }
+                return (offset: numericCast(start), byteCount: byteCount)
         }
     }
 }
diff --git a/Tests/VaporTests/FileTests.swift b/Tests/VaporTests/FileTests.swift
@@ -246,4 +246,55 @@ final class FileTests: XCTestCase {
             XCTAssertEqual(res.status, .notFound)
         }
     }
+
+    // https://github.com/vapor/vapor/security/advisories/GHSA-vj2m-9f5j-mpr5
+    func testInvalidRangeHeaderDoesNotCrash() throws {
+        let app = Application(.testing)
+        defer { app.shutdown() }
+
+        app.get("file-stream") { req in
+            return req.fileio.streamFile(at: #file)
+        }
+
+        var headers = HTTPHeaders()
+        headers.replaceOrAdd(name: .range, value: "bytes=0-9223372036854775807")
+        try app.testable(method: .running).test(.GET, "/file-stream", headers: headers) { res in
+            XCTAssertEqual(res.status, .badRequest)
+        }
+
+        headers.replaceOrAdd(name: .range, value: "bytes=-1-10")
+        try app.testable(method: .running).test(.GET, "/file-stream", headers: headers) { res in
+            XCTAssertEqual(res.status, .badRequest)
+        }
+
+        headers.replaceOrAdd(name: .range, value: "bytes=100-10")
+        try app.testable(method: .running).test(.GET, "/file-stream", headers: headers) { res in
+            XCTAssertEqual(res.status, .badRequest)
+        }
+
+        headers.replaceOrAdd(name: .range, value: "bytes=10--100")
+        try app.testable(method: .running).test(.GET, "/file-stream", headers: headers) { res in
+            XCTAssertEqual(res.status, .badRequest)
+        }
+
+        headers.replaceOrAdd(name: .range, value: "bytes=9223372036854775808-")
+        try app.testable(method: .running).test(.GET, "/file-stream", headers: headers) { res in
+            XCTAssertEqual(res.status, .badRequest)
+        }
+
+        headers.replaceOrAdd(name: .range, value: "bytes=922337203-")
+        try app.testable(method: .running).test(.GET, "/file-stream", headers: headers) { res in
+            XCTAssertEqual(res.status, .badRequest)
+        }
+
+        headers.replaceOrAdd(name: .range, value: "bytes=-922337203")
+        try app.testable(method: .running).test(.GET, "/file-stream", headers: headers) { res in
+            XCTAssertEqual(res.status, .badRequest)
+        }
+
+        headers.replaceOrAdd(name: .range, value: "bytes=-9223372036854775808")
+        try app.testable(method: .running).test(.GET, "/file-stream", headers: headers) { res in
+            XCTAssertEqual(res.status, .badRequest)
+        }
+    }
 }