From a22ebdf76d11f50e0e924a191890571896b54361 Mon Sep 17 00:00:00 2001 From: A Galway Date: Fri, 7 May 2021 12:46:38 +0100 Subject: [PATCH 1/2] cookie cleanup --- .../core/exploit/remote/http/http_cookie.rb | 26 ++++--- .../exploit/remote/http/http_cookie_jar.rb | 2 +- lib/msf/core/exploit/remote/http_client.rb | 6 +- .../remote/remote/http/http_cookie_spec.rb | 67 ++++++++++++++----- 4 files changed, 73 insertions(+), 28 deletions(-) diff --git a/lib/msf/core/exploit/remote/http/http_cookie.rb b/lib/msf/core/exploit/remote/http/http_cookie.rb index ac0f77225b54..aaaa3e300d42 100644 --- a/lib/msf/core/exploit/remote/http/http_cookie.rb +++ b/lib/msf/core/exploit/remote/http/http_cookie.rb @@ -11,13 +11,17 @@ class HttpCookie def initialize(name, value = nil, **attr_hash) if name.is_a?(::HTTP::Cookie) @cookie = name - elsif name && value && attr_hash - @cookie = ::HTTP::Cookie.new(name, value, **attr_hash) - elsif name && value + elsif value @cookie = ::HTTP::Cookie.new(name, value) else @cookie = ::HTTP::Cookie.new(name) end + + attr_hash.each_pair do |k, v| + if respond_to?("#{k}=".to_sym) + send("#{k}=".to_sym, v) + end + end end def name @@ -65,10 +69,6 @@ def expires=(expires) end end - def expired?(time = Time.now) - @cookie.expired?(time) - end - def path @cookie.path end @@ -98,11 +98,15 @@ def httponly=(httponly) end def domain - @cookie.domain + if @cookie.domain.nil? + nil + else + @cookie.domain.to_s + end end def domain=(domain) - if domain.nil? || domain.is_a?(DomainName) + if domain.nil? @cookie.domain = domain else @cookie.domain = domain.to_s @@ -133,6 +137,10 @@ def created_at=(time) end end + def expired?(time = Time.now) + @cookie.expired?(time) + end + def session? @cookie.session? end diff --git a/lib/msf/core/exploit/remote/http/http_cookie_jar.rb b/lib/msf/core/exploit/remote/http/http_cookie_jar.rb index acec7dff15a7..7bf9a1246ba1 100644 --- a/lib/msf/core/exploit/remote/http/http_cookie_jar.rb +++ b/lib/msf/core/exploit/remote/http/http_cookie_jar.rb @@ -40,7 +40,7 @@ def clear @cookie_jar.clear end - # Removes expired cookies and returns self. If `session` is true, + # Removes expired cookies and returns self. If `expire_all` is true, # all session cookies are removed as well. def cleanup(expire_all = false) @cookie_jar.cleanup(expire_all) diff --git a/lib/msf/core/exploit/remote/http_client.rb b/lib/msf/core/exploit/remote/http_client.rb index c9b995898c68..eb51d18d5a9f 100644 --- a/lib/msf/core/exploit/remote/http_client.rb +++ b/lib/msf/core/exploit/remote/http_client.rb @@ -373,19 +373,19 @@ def send_request_raw(opts = {}, timeout = 20, disconnect = false) # reads the response # # If a +Msf::Exploit::Remote::HTTP::HttpCookieJar+ instance is passed in the +opts+ dict under a 'cookie' key, said CookieJar will be used in - # the request instead of the module +cookie_jar+ + # the request instead of the module +cookie_jar+. Any other object passed under the `cookie` key will be converted to a string using +to_s+ + # and set as the cookie header of the request. # # Passes `opts` through directly to {Rex::Proto::Http::Client#request_cgi}. # Set `opts['keep_cookies']` to keep cookies from responses for reuse in requests. # Cookies returned by the server will be stored in +cookie_jar+ # - # +expire_cookies+ will control if +cleanup+ is called on any passed +Msf::Exploit::Remote::HTTP::HttpCookieJar+ or the client cookiejar + # Set `opts['expire_cookies']` to false in order to disable automatic removal of expired cookies # # @return (see Rex::Proto::Http::Client#send_recv)) def send_request_cgi(opts = {}, timeout = 20, disconnect = true) if opts.has_key?('cookie') if opts['cookie'].is_a?(Msf::Exploit::Remote::HTTP::HttpCookieJar) - cookie_jar.cleanup unless opts['expire_cookies'] == false opts.merge({ 'cookie' => opts['cookie'].cookies.join('; ') }) else opts.merge({ 'cookie' => opts['cookie'].to_s }) diff --git a/spec/lib/msf/core/exploit/remote/remote/http/http_cookie_spec.rb b/spec/lib/msf/core/exploit/remote/remote/http/http_cookie_spec.rb index 00fa77f42d85..3b8682633c9d 100644 --- a/spec/lib/msf/core/exploit/remote/remote/http/http_cookie_spec.rb +++ b/spec/lib/msf/core/exploit/remote/remote/http/http_cookie_spec.rb @@ -256,16 +256,6 @@ def random_string(min_len = 1, max_len = 12) end describe 'domain' do - describe 'DomainName' do - it 'assigned to domain when origin is set will result in a domain based on origin.host' do - d = DomainName(random_string) - - cookie.domain = d - - expect(cookie.domain).to eql(d.hostname) - end - end - describe 'nil' do it 'assigned to domain when origin is not set will result in a nil domain' do n = nil @@ -657,7 +647,7 @@ def random_string(min_len = 1, max_len = 12) expect(a).to eq(false) end - it 'will return false if url without http(s) protocol is passed' do + it 'will return false if url without http or https protocol is passed' do generic_uri = random_string v = cookie.acceptable_from_uri?(generic_uri) @@ -665,7 +655,7 @@ def random_string(min_len = 1, max_len = 12) expect(v).to eq(false) end - it 'will return false if url with http(s) protocol is passed but has no host' do + it 'will return false if url with http protocol is passed but has no host' do protcol = 'http://' v = cookie.acceptable_from_uri?(protcol) @@ -674,7 +664,16 @@ def random_string(min_len = 1, max_len = 12) expect(v).to eq(false) end - it 'will return true if url with http(s) protocol is passed with a domain that matches the url domain' do + it 'will return false if url with https protocol is passed but has no host' do + protcol = 'https://' + + v = cookie.acceptable_from_uri?(protcol) + + expect(URI(protcol).is_a?(::URI::HTTP)).to eq(true) + expect(v).to eq(false) + end + + it 'will return true if url with http protocol is passed with a domain that matches the url domain' do host = random_string uri = "http://#{host}/#{random_string}" cookie.domain = host @@ -684,9 +683,47 @@ def random_string(min_len = 1, max_len = 12) expect(v).to eq(true) end - it "will return domain.nil? if url with http(s) protocol is passed with a domain that doesn't match the url domain" do + it 'will return true if url with https protocol is passed with a domain that matches the url domain' do + host = random_string + uri = "https://#{host}/#{random_string}" + cookie.domain = host + + v = cookie.acceptable_from_uri?(uri) + + expect(v).to eq(true) + end + + it "will return true if url with http protocol is passed to a nil domain" do uri = "http://#{random_string}/#{random_string}" - cookie.domain = rand(0..1) == 1 ? nil : random_string + cookie.domain = nil + + v = cookie.acceptable_from_uri?(uri) + + expect(v).to eq(cookie.domain.nil?) + end + + it "will return false if url with http protocol is passed with a domain that doesn't match the cookie domain" do + uri = "http://#{random_string}/#{random_string}" + cookie.domain = random_string + 'cookie_domain' + + v = cookie.acceptable_from_uri?(uri) + + expect(v).to eq(cookie.domain.nil?) + end + + + it "will return true if url with https protocol is passed to a nil domain" do + uri = "https://#{random_string}/#{random_string}" + cookie.domain = nil + + v = cookie.acceptable_from_uri?(uri) + + expect(v).to eq(cookie.domain.nil?) + end + + it "will return false if url with https protocol is passed with a domain that doesn't match the cookie domain" do + uri = "https://#{random_string}/#{random_string}" + cookie.domain = random_string + 'cookie_domain' v = cookie.acceptable_from_uri?(uri) From 6b61eed3cd745266e8e7f0e383edfd5a31bc1627 Mon Sep 17 00:00:00 2001 From: A Galway Date: Fri, 7 May 2021 14:14:46 +0100 Subject: [PATCH 2/2] documention --- .../core/exploit/remote/http/http_cookie.rb | 104 ++++++++++++++++-- .../exploit/remote/http/http_cookie_jar.rb | 50 +++++++-- 2 files changed, 130 insertions(+), 24 deletions(-) diff --git a/lib/msf/core/exploit/remote/http/http_cookie.rb b/lib/msf/core/exploit/remote/http/http_cookie.rb index aaaa3e300d42..480dd86a3640 100644 --- a/lib/msf/core/exploit/remote/http/http_cookie.rb +++ b/lib/msf/core/exploit/remote/http/http_cookie.rb @@ -4,10 +4,26 @@ module Msf class Exploit class Remote module HTTP - # Acts as a wrapper for the 3rd party Cookie (http-cookie) + # This class is a representation of a Http Cookie with some built in convenience methods. + # Acts as a wrapper for the +HTTP::Cookie+ (https://www.rubydoc.info/gems/http-cookie/1.0.3/HTTP/Cookie) class . class HttpCookie include Comparable + # Returns a new +HttpCookie+. + # + # Name can either be a string or an instance of +::HTTP::Cookie+. + # - If an instance of +::HTTP::Cookie+, all future method calls will return values and act on values of said + # +::HTTP::Cookie+. + # - If a +String+, the name of the cookie is set to the passed +name+. + # - If a +String+ is passed as a +name+, the cookie is set as a session cookie. + # + # Value can be a +String+ or +nil+. + # - If a +String+, the value of the cookie is set as the passed +cookie+. + # - If +nil+, the value of the cookie is set as an empty +String+ +''+ and the cookie is set to expire at + # +UNIX_EPOCH+ + # + # +attr_hash+ can be used to set the values of +domain+, +path+, +max_age+, +expires+, +secure+, +httponly+, + # +accessed_at+, +created_at+. def initialize(name, value = nil, **attr_hash) if name.is_a?(::HTTP::Cookie) @cookie = name @@ -24,18 +40,25 @@ def initialize(name, value = nil, **attr_hash) end end + # Returns the name of cookie of type +String+. def name @cookie.name end + # Sets the cookie name. def name=(name) @cookie.name = name.to_s end + # Returns the value of cookie of type +String+. def value @cookie.value end + # Sets the cookie value. + # + # Passed +value+ must be +nil+, an instance of +String+, or an object that can be converted successfully to + # a +String+ with +to_s+. def value=(value) if value.nil? || value.is_a?(String) @cookie.value = value @@ -44,10 +67,17 @@ def value=(value) end end + # Returns the value of max_age. + # + # max_age is the number of seconds until a cookie expires. def max_age @cookie.max_age end + # Sets the cookie max_age of type +Integer+. + # + # Passed +max_age+ must be +nil+, an +Integer+, or an object that can be converted successfully to an + # +Integer+ with +Integer(max_age)+. def max_age=(max_age) if max_age.nil? || max_age.is_a?(Integer) @cookie.max_age = max_age @@ -56,10 +86,17 @@ def max_age=(max_age) end end + # Returns the value of cookie expires of type +Time+. + # + # expires is the date and time at which a cookie expires. def expires @cookie.expires end + # Sets the cookie expires value. + # + # Passed +expires+ must be +nil+, an instance of +Time+, or an object that can be converted successfully to + # an +Time+ with +Time.parse(expires)+. def expires=(expires) if expires.nil? || expires.is_a?(Time) @cookie.expires = expires @@ -69,10 +106,17 @@ def expires=(expires) end end + # Returns the cookie path of type +String+. + # + # path is the URL for which the cookie is valid. def path @cookie.path end + # Sets the cookie path. + # + # Passed +path+ must be +nil+, an instance of +String+, or an object that can be converted successfully to a + # +String+ with +to_s+. def path=(path) if path.nil? || path.is_a?(String) @cookie.path = path @@ -81,22 +125,40 @@ def path=(path) end end + # Returns the cookie secure value of type +Boolean+. + # + # secure is a boolean that indicates if the cookie should be limited to the scope of secure channels as + # defined by the user agent. def secure @cookie.secure end + # Sets the cookie secure value. + # + # Passed +secure+ is converted to a Boolean with +!!secure+ and set. def secure=(secure) @cookie.secure = !!secure end + # Returns the cookie httponly value of type +Boolean+. + # + # httponly is a +Boolean+ that indicates if client-side scripts should be prevented from accessing data. def httponly @cookie.httponly end + # Sets the cookie httponly value. + # + # Passed +httponly+ is converted to a Boolean with +!!httponly+ and set. def httponly=(httponly) @cookie.httponly = !!httponly end + # Returns the cookie domain of type +String+. + # + # If omitted, defaults to the host of the current document URL, not including subdomains. Leading dots in + # domain names (.example.com) are ignored. Multiple host/domain values are not allowed, but if a domain is + # specified, then subdomains are always included. def domain if @cookie.domain.nil? nil @@ -105,6 +167,10 @@ def domain end end + # Sets the cookie domain. + # + # Passed +domain+ must be +nil+, an instance of +String+, or an object that can be converted successfully to + # an +String+ with +to_s+. def domain=(domain) if domain.nil? @cookie.domain = domain @@ -113,10 +179,16 @@ def domain=(domain) end end + # Returns the cookie accessed_at value of type +Time+. accessed_at indicates when a cookie was last interacted + # with. def accessed_at @cookie.accessed_at end + # Sets the cookie accessed_at time. + # + # Passed +time+ must be +nil+, an instance of +Time+, or an object that can be converted successfully to an + # +Time+ with +Time.parse+. def accessed_at=(time) if time.nil? || time.is_a?(Time) @cookie.accessed_at = time @@ -125,10 +197,15 @@ def accessed_at=(time) end end + # Returns the cookie created_at value of type +Time+. created_at indicates when a cookie was created. def created_at @cookie.created_at end + # Sets the cookie accessed_at time. + # + # Passed +time+ must be +nil+, an instance of +Time+, or an object that can be converted successfully to an + # +Time+ with +Time.parse+. def created_at=(time) if time.nil? || time.is_a?(Time) @cookie.created_at = time @@ -137,20 +214,31 @@ def created_at=(time) end end + # Returns a string representation of the cookie for use in a cookie header. + # Comes in format "#{name}=#{value}". + def cookie_value + @cookie.cookie_value + end + alias to_s cookie_value + + # Returns a boolean indicating if the cookie will have expired by the date and time represented by +time+. + # +time+ defaults to +Time.now+, so the method can return a different value after enough calls. def expired?(time = Time.now) @cookie.expired?(time) end + # Returns a boolean indicating if the cookie is a Session Cookie. def session? @cookie.session? end + # Tests if it is OK to accept this cookie. If either domain or path is missing an ArgumentError is raised. def acceptable? @cookie.acceptable? end - # Tests if it is OK to send this cookie to a given `uri`. An - # ArgumentError is raised if the cookie's domain is unknown. + # Returns a boolean indicating if the cookie can be sent to the passed +uri+. + # Raises an ArgumentError if domain is nil (unset). def valid_for_uri?(uri) return false if uri.nil? raise ArgumentError, 'cannot tell if this cookie is valid as domain is nil' if domain.nil? @@ -158,8 +246,7 @@ def valid_for_uri?(uri) @cookie.valid_for_uri?(uri) end - # Tests if it is OK to accept this cookie if it is sent from a given - # URI/URL, `uri`. + # Tests if it is OK to accept this cookie if it is sent from the passed +uri+. def acceptable_from_uri?(uri) return false if uri.nil? @@ -169,13 +256,6 @@ def acceptable_from_uri?(uri) def <=>(other) @cookie <=> other end - - # Returns a string for use in the Cookie header, i.e. `name=value` - # or `name="value"`. - def cookie_value - @cookie.cookie_value - end - alias to_s cookie_value end end end diff --git a/lib/msf/core/exploit/remote/http/http_cookie_jar.rb b/lib/msf/core/exploit/remote/http/http_cookie_jar.rb index 7bf9a1246ba1..eb712563bea2 100644 --- a/lib/msf/core/exploit/remote/http/http_cookie_jar.rb +++ b/lib/msf/core/exploit/remote/http/http_cookie_jar.rb @@ -3,18 +3,29 @@ require 'http/cookie_jar' require 'http/cookie' +# This class is a collection of Http Cookies with some built in convenience methods. +# Acts as a wrapper for the +::HTTP::CookieJar+ (https://www.rubydoc.info/gems/http-cookie/1.0.2/HTTP/CookieJar) class. + module Msf class Exploit class Remote module HTTP # Acts as a wrapper for the 3rd party CookieJar (http-cookie) class HttpCookieJar + + # Returns a new instance of +HttpCookieJar+. def initialize @cookie_jar = ::HTTP::CookieJar.new({ store: HashStoreWithoutAutomaticExpiration }) end + # Adds +cookie+ to the jar. + # + # +cookie+ must be an instance or subclass of +Msf::Exploit::Remote::HTTP::HttpCookie+, or a `TypeError` + # will be raised. + # + # Returns +self+. def add(cookie) raise TypeError, "Passed cookie is of class '#{cookie.class}' and not a subclass of '#{Msf::Exploit::Remote::HTTP::HttpCookie}" unless cookie.is_a?(Msf::Exploit::Remote::HTTP::HttpCookie) @@ -22,6 +33,9 @@ def add(cookie) self end + # Will remove any cookie from the jar that has the same +name+, +domain+ and +path+ as the passed +cookie+. + # + # Returns +self+. def delete(cookie) return if @cookie_jar.cookies.empty? raise TypeError, "Passed cookie is of class '#{cookie.class}' and not a subclass of '#{Msf::Exploit::Remote::HTTP::HttpCookie}" unless cookie.is_a?(Msf::Exploit::Remote::HTTP::HttpCookie) @@ -30,30 +44,42 @@ def delete(cookie) self end - # Iterates over all cookies that are not expired in no particular - # order. + # Returns an unordered array of all cookies stored in the jar. def cookies @cookie_jar.cookies end + # Will remove all cookies from the jar. + # + # Returns +nil+. def clear @cookie_jar.clear + self end - # Removes expired cookies and returns self. If `expire_all` is true, - # all session cookies are removed as well. + # Will remove all expired cookies. If +expire_all+ is set as true, all session cookies are removed as well. + # + # Returns +self+. def cleanup(expire_all = false) @cookie_jar.cleanup(expire_all) + self end + # Returns +true+ if the jar contains no cookies, else +false+. def empty? @cookie_jar.empty? end + # Parses a Set-Cookie header value +set_cookie_header+ assuming that it is sent from a source URI/URL + # +origin_url+, and returns an array of +::HTTP::Cookie+ (https://www.rubydoc.info/gems/http-cookie/1.0.3/HTTP/Cookie) + # objects. Parts (separated by commas) that are malformed or considered unacceptable are silently ignored. def parse(set_cookie_header, origin_url, options = nil) ::HTTP::Cookie.parse(set_cookie_header, origin_url, options) end + # Same as +parse+, but each +::HTTP::Cookie+ (https://www.rubydoc.info/gems/http-cookie/1.0.3/HTTP/Cookie) is + # converted to +HttpCookie+ (https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/exploit/remote/http/http_cookie.rb) + # and added to the jar. def parse_and_merge(set_cookie_header, origin_url, options = nil) parsed_cookies = ::HTTP::Cookie.parse(set_cookie_header, origin_url, options) parsed_cookies.each { |c| add(Msf::Exploit::Remote::HTTP::HttpCookie.new(c)) } @@ -61,15 +87,15 @@ def parse_and_merge(set_cookie_header, origin_url, options = nil) end end + # On top of iterating over every item in the store, +::HTTP::CookieJar::HashStore+ also deletes any expired cookies + # and has the option to filter cookies based on whether they are parent of a passed url. + # + # We've removed the extraneous features in the overwritten method. + # - The deletion of cookies while you're iterating over them complicated simple cookie management. It also + # prevented sending expired cookies if needed for an exploit + # - Any URL passed for filtering could be resolved to nil if it was improperly formed or resolved to a eTLD, + # which was too brittle for our uses class HashStoreWithoutAutomaticExpiration < ::HTTP::CookieJar::HashStore - # On top of iterating over every item in the store, +::HTTP::CookieJar::HashStore+ also deletes any expired cookies - # and has the option to filter cookies based on whether they are parent of a passed url. - # - # We've removed the extraneous features in the overwritten method. - # - The deletion of cookies while you're iterating over them complicated simple cookie management. It also - # prevented sending expired cookies if needed for an exploit - # - Any URL passed for filtering could be resolved to nil if it was improperly formed or resolved to a eTLD, - # which was too brittle for our uses def each(uri = nil) raise ArgumentError, "HashStoreWithoutAutomaticExpiration.each doesn't support url filtering" if uri