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

@@ -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)

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)

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

test/integration/test_network.rb

@@ -7,17 +7,17 @@
     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
 
         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,17 +26,17 @@
           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
           end
 
           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

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",

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