Fixes #932

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