diff --git a/context/headers.md b/context/headers.md index bd947c1..191a0ef 100644 --- a/context/headers.md +++ b/context/headers.md @@ -74,12 +74,13 @@ headers.trailer! headers.add("etag", '"12345"') headers.add("date", Time.now.httpdate) -# These will be silently ignored for security -headers.add("authorization", "Bearer token") # Ignored - credential leakage risk -headers.add("connection", "close") # Ignored - hop-by-hop header +# Each of these will raise InvalidTrailerError for security: +headers.add("authorization", "Bearer token") # => raises InvalidTrailerError (credential leakage risk) +headers.add("connection", "close") # => raises InvalidTrailerError (hop-by-hop header) +headers["content-length"] = "0" # => raises InvalidTrailerError (framing header) ``` -The trailer security system prevents HTTP request smuggling by restricting which headers can appear in trailers: +The trailer security system prevents HTTP request smuggling by restricting which headers can appear in trailers. Once `trailer!` has been called, every method which appends fields (`add`, `set`, `[]=` and `merge!`) validates the header against the policy before modifying the headers, and raises {ruby Protocol::HTTP::InvalidTrailerError} if it is not permitted: **Allowed headers** (return `true` for `trailer?`): - `date` - Response generation timestamps. diff --git a/guides/headers/readme.md b/guides/headers/readme.md index bd947c1..191a0ef 100644 --- a/guides/headers/readme.md +++ b/guides/headers/readme.md @@ -74,12 +74,13 @@ headers.trailer! headers.add("etag", '"12345"') headers.add("date", Time.now.httpdate) -# These will be silently ignored for security -headers.add("authorization", "Bearer token") # Ignored - credential leakage risk -headers.add("connection", "close") # Ignored - hop-by-hop header +# Each of these will raise InvalidTrailerError for security: +headers.add("authorization", "Bearer token") # => raises InvalidTrailerError (credential leakage risk) +headers.add("connection", "close") # => raises InvalidTrailerError (hop-by-hop header) +headers["content-length"] = "0" # => raises InvalidTrailerError (framing header) ``` -The trailer security system prevents HTTP request smuggling by restricting which headers can appear in trailers: +The trailer security system prevents HTTP request smuggling by restricting which headers can appear in trailers. Once `trailer!` has been called, every method which appends fields (`add`, `set`, `[]=` and `merge!`) validates the header against the policy before modifying the headers, and raises {ruby Protocol::HTTP::InvalidTrailerError} if it is not permitted: **Allowed headers** (return `true` for `trailer?`): - `date` - Response generation timestamps. diff --git a/lib/protocol/http/error.rb b/lib/protocol/http/error.rb index 5e5911f..9307198 100644 --- a/lib/protocol/http/error.rb +++ b/lib/protocol/http/error.rb @@ -68,6 +68,8 @@ class InvalidTrailerError < InvalidHeaderError # @parameter key [String] The trailer key that is invalid. def initialize(key) super("Invalid trailer key: #{key.inspect}") + + @key = key end # @attribute [String] key The trailer key that is invalid. diff --git a/lib/protocol/http/headers.rb b/lib/protocol/http/headers.rb index ddccf02..ea6ae47 100644 --- a/lib/protocol/http/headers.rb +++ b/lib/protocol/http/headers.rb @@ -251,15 +251,12 @@ def extract(keys) # @parameter key [String] the header key. # @parameter value [String] the header value to assign. # @parameter trailer [Boolean] whether this header is being added as a trailer. + # @raises [InvalidTrailerError] If the header is not permitted in the trailer section. def add(key, value, trailer: self.trailer?) value = value.to_s if trailer - policy = @policy[key.downcase] - - if !policy or !policy.trailer? - raise InvalidTrailerError, key - end + validate_trailer!(key, @policy[key.downcase]) end if @indexed @@ -273,7 +270,13 @@ def add(key, value, trailer: self.trailer?) # # @parameter key [String] the header key to replace. # @parameter value [String] the header value to assign. + # @raises [InvalidTrailerError] If the header is not permitted in the trailer section. def set(key, value) + # Validate before deleting, so that a rejected assignment leaves the headers unchanged: + if self.trailer? + validate_trailer!(key, @policy[key.downcase]) + end + self.delete(key) self.add(key, value) end @@ -284,13 +287,21 @@ def set(key, value) # # @parameter key [String] the header key. # @parameter value [String | Array] the header value to assign. + # @raises [InvalidTrailerError] If the header is not permitted in the trailer section. def []=(key, value) + policy = @policy[key.downcase] + + # Validate before deleting, so that a rejected assignment leaves the headers unchanged: + if self.trailer? + validate_trailer!(key, policy) + end + key = key.downcase # Delete existing value if any: self.delete(key) - if policy = @policy[key] + if policy unless value.is_a?(policy) value = policy.coerce(value) end @@ -448,6 +459,19 @@ def delete(key) end end + # Ensure the given header is permitted in the trailer section. + # + # Headers which affect framing, routing, connection handling or authorization must not appear in trailers, as they could otherwise be used for request smuggling or to bypass processing that occurred while the header section was being handled. + # + # @parameter key [String] The header key. + # @parameter policy [Class | Boolean | Nil] The policy for the header key, `false` if the header is a simple singleton string, or `nil` if the policy has no entry for the key. + # @raises [InvalidTrailerError] If the header is not permitted in the trailer section. + protected def validate_trailer!(key, policy) + if !policy or !policy.trailer? + raise InvalidTrailerError, key + end + end + # Merge the value into the hash according to the policy for the given key. # # @parameter hash [Hash] The hash to merge into. diff --git a/releases.md b/releases.md index 9480d0a..6869d30 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,11 @@ # Releases +## Unreleased + + - `Protocol::HTTP::Headers#[]=` now validates trailers in the same way as `#add`, raising `InvalidTrailerError` when a forbidden header (e.g. `content-length`, `transfer-encoding`, `authorization`) is assigned after `#trailer!` has been called. + - `Protocol::HTTP::Headers#set` now validates the trailer before deleting the existing header, so a rejected assignment no longer removes the existing value. + - `Protocol::HTTP::InvalidTrailerError#key` now returns the rejected header key instead of `nil`. + ## v0.72.0 - Clarified body stream lifecycle and ownership, including the directional semantics of `Protocol::HTTP::Body::Stream#close_read`, `#close_write`, and `#close`, how premature input closure affects the associated HTTP exchange, and ownership of streams passed to `Streamable#call`. diff --git a/test/protocol/http/headers.rb b/test/protocol/http/headers.rb index 9b0c504..1c537a1 100644 --- a/test/protocol/http/headers.rb +++ b/test/protocol/http/headers.rb @@ -274,6 +274,59 @@ headers["Content-Length"] = "1" expect(headers["content-length"]).to be == "1" end + + with "trailer" do + it "can assign a permitted trailer" do + trailer = headers.trailer! + + headers["ETag"] = "abcd" + + expect(trailer.to_a).to be == [["etag", "abcd"]] + expect(headers["etag"]).to be == "abcd" + end + + %w[content-length transfer-encoding authorization].each do |key| + with "forbidden trailer #{key.inspect}", unique: key do + it "can't assign a #{key.inspect} header in the trailer" do + headers.trailer! + + expect do + headers[key] = "example" + end.to raise_exception(Protocol::HTTP::InvalidTrailerError).and(have_attributes(key: be == key)) + + expect(headers.trailer.to_a).to be(:empty?) + expect(headers).not.to be(:include?, key) + end + end + end + + it "reports the key as given in the error" do + headers.trailer! + + expect do + headers["Content-Length"] = "1" + end.to raise_exception(Protocol::HTTP::InvalidTrailerError, message: be == 'Invalid trailer key: "Content-Length"').and(have_attributes(key: be == "Content-Length")) + end + + it "does not delete an existing header when the trailer is rejected" do + headers["content-length"] = "1" + headers.trailer! + + expect do + headers["content-length"] = "999" + end.to raise_exception(Protocol::HTTP::InvalidTrailerError) + + expect(headers["content-length"]).to be == "1" + expect(headers.trailer.to_a).to be(:empty?) + end + + it "can assign a forbidden trailer before the trailer section starts" do + headers["content-length"] = "1" + + expect(headers).not.to be(:trailer?) + expect(headers["content-length"]).to be == "1" + end + end end with "#add" do @@ -293,6 +346,26 @@ expect(headers["accept-encoding"]).to be == ["gzip"] end + + it "can't set a forbidden header in the trailer" do + headers.trailer! + + expect do + headers.set("content-length", "999") + end.to raise_exception(Protocol::HTTP::InvalidTrailerError) + end + + it "does not delete an existing header when the trailer is rejected" do + headers.set("content-length", "1") + headers.trailer! + + expect do + headers.set("content-length", "999") + end.to raise_exception(Protocol::HTTP::InvalidTrailerError) + + expect(headers["content-length"]).to be == "1" + expect(headers.trailer.to_a).to be(:empty?) + end end with "#extract" do @@ -339,6 +412,16 @@ end end + with "#merge!" do + it "can't merge a forbidden header into the trailer" do + headers.trailer! + + expect do + headers.merge!("content-length" => "999") + end.to raise_exception(Protocol::HTTP::InvalidTrailerError) + end + end + with "#trailer!" do it "can add trailer" do headers.add("trailer", "etag")