From 48d594dae55934476fec61789e7a7c3700e0f50d Mon Sep 17 00:00:00 2001 From: Peter Goldstein Date: Thu, 27 Oct 2022 17:58:52 -0400 Subject: [PATCH] Fixes #932 (#933) Do some input sanitization for the meta protocol. Since this protocol uses raw test, it is possible to execute an injection attack against unsanitized inputs. This commit explicitly sanitizes the CAS arguments and the ttl passed to flush. I also added some missing tests for meta_arithmetic and fixed a few lints. --- CHANGELOG.md | 2 + lib/dalli/protocol/meta.rb | 1 + lib/dalli/protocol/meta/request_formatter.rb | 23 +++- test/integration/test_network.rb | 12 +-- test/protocol/meta/test_request_formatter.rb | 105 ++++++++++++++++++- test/test_rack_session.rb | 4 +- 6 files changed, 133 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7032cb3d..de0887b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ Dalli Changelog Unreleased ========== +- Sanitize CAS inputs to ensure additional commands are not passed to memcached (xhzeem / petergoldstein) +- Sanitize input to flush command to ensure additional commands are not passed to memcached (xhzeem / petergoldstein) - Namespaces passed as procs are now evaluated every time, as opposed to just on initialization (nrw505) - Fix missing require of uri in ServerConfigParser (adam12) - Fix link to the CHANGELOG.md file in README.md (rud) diff --git a/lib/dalli/protocol/meta.rb b/lib/dalli/protocol/meta.rb index 4d6c662d..b2e66c37 100644 --- a/lib/dalli/protocol/meta.rb +++ b/lib/dalli/protocol/meta.rb @@ -44,6 +44,7 @@ def gat(key, ttl, options = nil) end def touch(key, ttl) + ttl = TtlSanitizer.sanitize(ttl) encoded_key, base64 = KeyRegularizer.encode(key) req = RequestFormatter.meta_get(key: encoded_key, ttl: ttl, value: false, base64: base64) write(req) diff --git a/lib/dalli/protocol/meta/request_formatter.rb b/lib/dalli/protocol/meta/request_formatter.rb index b36a1219..7e485fea 100644 --- a/lib/dalli/protocol/meta/request_formatter.rb +++ b/lib/dalli/protocol/meta/request_formatter.rb @@ -31,7 +31,7 @@ def self.meta_set(key:, value:, bitflags: nil, cas: nil, ttl: nil, mode: :set, b cmd << ' c' unless %i[append prepend].include?(mode) cmd << ' b' if base64 cmd << " F#{bitflags}" if bitflags - cmd << " C#{cas}" if cas && !cas.zero? + cmd << cas_string(cas) cmd << " T#{ttl}" if ttl cmd << " M#{mode_to_token(mode)}" cmd << ' q' if quiet @@ -43,7 +43,7 @@ def self.meta_set(key:, value:, bitflags: nil, cas: nil, ttl: nil, mode: :set, b def self.meta_delete(key:, cas: nil, ttl: nil, base64: false, quiet: false) cmd = "md #{key}" cmd << ' b' if base64 - cmd << " C#{cas}" if cas && !cas.zero? + cmd << cas_string(cas) cmd << " T#{ttl}" if ttl cmd << ' q' if quiet cmd + TERMINATOR @@ -54,8 +54,9 @@ def self.meta_arithmetic(key:, delta:, initial:, incr: true, cas: nil, ttl: nil, cmd << ' b' if base64 cmd << " D#{delta}" if delta cmd << " J#{initial}" if initial - cmd << " C#{cas}" if cas && !cas.zero? - cmd << " N#{ttl}" if ttl + # Always set a TTL if an initial value is specified + cmd << " N#{ttl || 0}" if ttl || initial + cmd << cas_string(cas) cmd << ' q' if quiet cmd << " M#{incr ? 'I' : 'D'}" cmd + TERMINATOR @@ -75,7 +76,7 @@ def self.version def self.flush(delay: nil, quiet: false) cmd = +'flush_all' - cmd << " #{delay}" if delay + cmd << " #{parse_to_64_bit_int(delay, 0)}" if delay cmd << ' noreply' if quiet cmd + TERMINATOR end @@ -102,6 +103,18 @@ def self.mode_to_token(mode) end end # rubocop:enable Metrics/MethodLength + + def self.cas_string(cas) + cas = parse_to_64_bit_int(cas, nil) + cas.nil? || cas.zero? ? '' : " C#{cas}" + end + + def self.parse_to_64_bit_int(val, default) + val.nil? ? nil : Integer(val) + rescue ArgumentError + # Sanitize to default if it isn't parsable as an integer + default + end end end end diff --git a/test/integration/test_network.rb b/test/integration/test_network.rb index e0ea862c..bf20baae 100644 --- a/test/integration/test_network.rb +++ b/test/integration/test_network.rb @@ -7,8 +7,8 @@ describe "using the #{p} protocol" do describe 'assuming a bad network' do it 'handle no server available' do + dc = Dalli::Client.new 'localhost:19333' assert_raises Dalli::RingError, message: 'No server available' do - dc = Dalli::Client.new 'localhost:19333' dc.get 'foo' end end @@ -16,8 +16,8 @@ describe 'with a fake server' do it 'handle connection reset' do memcached_mock(->(sock) { sock.close }) do + dc = Dalli::Client.new('localhost:19123') assert_raises Dalli::RingError, message: 'No server available' do - dc = Dalli::Client.new('localhost:19123') dc.get('abc') end end @@ -26,8 +26,8 @@ it 'handle connection reset with unix socket' do socket_path = MemcachedMock::UNIX_SOCKET_PATH memcached_mock(->(sock) { sock.close }, :start_unix, socket_path) do + dc = Dalli::Client.new(socket_path) assert_raises Dalli::RingError, message: 'No server available' do - dc = Dalli::Client.new(socket_path) dc.get('abc') end end @@ -35,8 +35,8 @@ it 'handle malformed response' do memcached_mock(->(sock) { sock.write('123') }) do + dc = Dalli::Client.new('localhost:19123') assert_raises Dalli::RingError, message: 'No server available' do - dc = Dalli::Client.new('localhost:19123') dc.get('abc') end end @@ -47,8 +47,8 @@ sleep(0.6) sock.close }, :delayed_start) do + dc = Dalli::Client.new('localhost:19123') assert_raises Dalli::RingError, message: 'No server available' do - dc = Dalli::Client.new('localhost:19123') dc.get('abc') end end @@ -59,8 +59,8 @@ sleep(0.6) sock.write('giraffe') }) do + dc = Dalli::Client.new('localhost:19123') assert_raises Dalli::RingError, message: 'No server available' do - dc = Dalli::Client.new('localhost:19123') dc.get('abc') end end diff --git a/test/protocol/meta/test_request_formatter.rb b/test/protocol/meta/test_request_formatter.rb index 1eef499b..755e7305 100644 --- a/test/protocol/meta/test_request_formatter.rb +++ b/test/protocol/meta/test_request_formatter.rb @@ -77,11 +77,28 @@ Dalli::Protocol::Meta::RequestFormatter.meta_set(key: key, value: val, bitflags: bitflags, cas: cas) end + it 'excludes CAS if set to 0' do + assert_equal "ms #{key} #{val.bytesize} c F#{bitflags} MS\r\n#{val}\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_set(key: key, value: val, bitflags: bitflags, cas: 0) + end + + it 'excludes non-numeric CAS values' do + assert_equal "ms #{key} #{val.bytesize} c F#{bitflags} MS\r\n#{val}\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_set(key: key, value: val, bitflags: bitflags, + cas: "\nset importantkey 1 1000 8\ninjected") + end + it 'sets the quiet mode if configured' do assert_equal "ms #{key} #{val.bytesize} c F#{bitflags} MS q\r\n#{val}\r\n", Dalli::Protocol::Meta::RequestFormatter.meta_set(key: key, value: val, bitflags: bitflags, quiet: true) end + + it 'sets the base64 mode if configured' do + assert_equal "ms #{key} #{val.bytesize} c b F#{bitflags} MS\r\n#{val}\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_set(key: key, value: val, bitflags: bitflags, + base64: true) + end end describe 'meta_delete' do @@ -93,7 +110,7 @@ Dalli::Protocol::Meta::RequestFormatter.meta_delete(key: key) end - it 'returns incorporates CAS when passed cas' do + it 'incorporates CAS when passed cas' do assert_equal "md #{key} C#{cas}\r\n", Dalli::Protocol::Meta::RequestFormatter.meta_delete(key: key, cas: cas) end @@ -102,6 +119,87 @@ assert_equal "md #{key} q\r\n", Dalli::Protocol::Meta::RequestFormatter.meta_delete(key: key, quiet: true) end + + it 'excludes CAS when set to 0' do + assert_equal "md #{key}\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_delete(key: key, cas: 0) + end + + it 'excludes non-numeric CAS values' do + assert_equal "md #{key}\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_delete(key: key, + cas: "\nset importantkey 1 1000 8\ninjected") + end + + it 'sets the base64 mode if configured' do + assert_equal "md #{key} b\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_delete(key: key, base64: true) + end + end + + describe 'meta_arithmetic' do + let(:key) { SecureRandom.hex(4) } + let(:delta) { rand(500..999) } + let(:initial) { rand(500..999) } + let(:cas) { rand(500..999) } + let(:ttl) { rand(500..999) } + + it 'returns the expected string with the default N flag when passed non-nil key, delta, and initial' do + assert_equal "ma #{key} v D#{delta} J#{initial} N0 MI\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: delta, initial: initial) + end + + it 'excludes the J and N flags when initial is nil and ttl is not set' do + assert_equal "ma #{key} v D#{delta} MI\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: delta, initial: nil) + end + + it 'omits the D flag is delta is nil' do + assert_equal "ma #{key} v J#{initial} N0 MI\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: nil, initial: initial) + end + + it 'uses ttl for the N flag when ttl passed explicitly along with an initial value' do + assert_equal "ma #{key} v D#{delta} J#{initial} N#{ttl} MI\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: delta, initial: initial, + ttl: ttl) + end + + it 'incorporates CAS when passed cas' do + assert_equal "ma #{key} v D#{delta} J#{initial} N0 C#{cas} MI\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: delta, initial: initial, + cas: cas) + end + + it 'excludes CAS when CAS is set to 0' do + assert_equal "ma #{key} v D#{delta} J#{initial} N0 MI\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: delta, initial: initial, + cas: 0) + end + + it 'includes the N flag when ttl passed explicitly with a nil initial value' do + assert_equal "ma #{key} v D#{delta} N#{ttl} MI\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: delta, initial: nil, + ttl: ttl) + end + + it 'swaps from MI to MD when the incr value is explicitly false' do + assert_equal "ma #{key} v D#{delta} J#{initial} N0 MD\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: delta, initial: initial, + incr: false) + end + + it 'includes the quiet flag when specified' do + assert_equal "ma #{key} v D#{delta} J#{initial} N0 q MI\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: delta, initial: initial, + quiet: true) + end + + it 'sets the base64 mode if configured' do + assert_equal "ma #{key} v b D#{delta} J#{initial} N0 MI\r\n", + Dalli::Protocol::Meta::RequestFormatter.meta_arithmetic(key: key, delta: delta, initial: initial, + base64: true) + end end describe 'meta_noop' do @@ -130,6 +228,11 @@ assert_equal "flush_all #{delay}\r\n", Dalli::Protocol::Meta::RequestFormatter.flush(delay: delay) end + it 'santizes the delay argument' do + delay = "\nset importantkey 1 1000 8\ninjected" + assert_equal "flush_all 0\r\n", Dalli::Protocol::Meta::RequestFormatter.flush(delay: delay) + end + it 'adds noreply with a delay and quiet argument' do delay = rand(1000..1999) assert_equal "flush_all #{delay} noreply\r\n", diff --git a/test/test_rack_session.rb b/test/test_rack_session.rb index e56a098e..93860345 100644 --- a/test/test_rack_session.rb +++ b/test/test_rack_session.rb @@ -54,15 +54,15 @@ let(:incrementor) { Rack::Lint.new(incrementor_proc) } it 'faults on no connection' do + rsd = Rack::Session::Dalli.new(incrementor, memcache_server: 'nosuchserver') assert_raises Dalli::RingError do - rsd = Rack::Session::Dalli.new(incrementor, memcache_server: 'nosuchserver') rsd.data.with { |c| c.set('ping', '') } end end it 'connects to existing server' do + rsd = Rack::Session::Dalli.new(incrementor, namespace: 'test:rack:session') assert_silent do - rsd = Rack::Session::Dalli.new(incrementor, namespace: 'test:rack:session') rsd.data.with { |c| c.set('ping', '') } end end