Description: Replace marshalling with pluggable serializers
Author: Tom Scott <tscott@weblinc.com>
Bug: https://github.com/redis-store/redis-store/issues/289
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882034
Applied-Upstream: https://github.com/redis-store/redis-store/commit/e0c1398d54a9661c8c70267c3a925ba6b192142e
Origin: upstream
Last-Update: Tue, 15 Aug 2017 11:07:07 -0400

This is in response to a vulnerability warning we received on Friday,
August 11th, 2017. While most users will not be affected by this
change, we recommend that developers of new applications use a different
serializer other than `Marshal`. This, along with the removal of the
`:marshalling` option, will enforce "sane defaults" in terms of securely
serializing/de-serializing data.

- Add `:serializer` option and deprecate `:marshalling`. Although you
  will still be able to enable/disable serialization with Marshal using
  `:marshalling` in the 1.x series, this will be removed by 2.0.

- Rename `Redis::Store::Marshalling` to `Redis::Store::Serialization` to
  reflect its new purpose.

Fixes #289
---
 lib/redis-store.rb                                 | 12 -------
 lib/redis/store.rb                                 | 28 +++++++++++++--
 lib/redis/store/factory.rb                         |  9 ++++-
 lib/redis/store/namespace.rb                       |  4 +--
 .../store/{marshalling.rb => serialization.rb}     |  6 ++--
 test/redis/store/factory_test.rb                   | 40 ++++++++++++++++++++--
 test/redis/store/namespace_test.rb                 |  4 +--
 .../{marshalling_test.rb => serialization_test.rb} |  4 +--
 8 files changed, 80 insertions(+), 27 deletions(-)
 rename lib/redis/store/{marshalling.rb => serialization.rb} (90%)
 rename test/redis/store/{marshalling_test.rb => serialization_test.rb} (98%)

--- a/lib/redis-store.rb
+++ b/lib/redis-store.rb
@@ -1,12 +1 @@
-require 'redis'
 require 'redis/store'
-require 'redis/store/factory'
-require 'redis/distributed_store'
-require 'redis/store/namespace'
-require 'redis/store/marshalling'
-require 'redis/store/version'
-
-class Redis
-  class Store < self
-  end
-end
--- a/lib/redis/store.rb
+++ b/lib/redis/store.rb
@@ -1,3 +1,9 @@
+require 'redis'
+require 'redis/store/factory'
+require 'redis/distributed_store'
+require 'redis/store/namespace'
+require 'redis/store/serialization'
+require 'redis/store/version'
 require 'redis/store/ttl'
 require 'redis/store/interface'
 
@@ -7,6 +13,24 @@
 
     def initialize(options = { })
       super
+
+      unless options[:marshalling].nil?
+        puts %(
+          DEPRECATED: You are passing the :marshalling option, which has been
+          replaced with `serializer: Marshal` to support pluggable serialization
+          backends. To disable serialization (much like disabling marshalling),
+          pass `serializer: nil` in your configuration.
+
+          The :marshalling option will be removed for redis-store 2.0.
+        )
+      end
+
+      @serializer = options.key?(:serializer) ? options[:serializer] : Marshal
+
+      unless options[:marshalling].nil?
+        @serializer = options[:marshalling] ? Marshal : nil
+      end
+
       _extend_marshalling options
       _extend_namespace   options
     end
@@ -22,8 +46,7 @@
 
     private
       def _extend_marshalling(options)
-        @marshalling = !(options[:marshalling] === false) # HACK - TODO delegate to Factory
-        extend Marshalling if @marshalling
+        extend Serialization unless @serializer.nil?
       end
 
       def _extend_namespace(options)
--- a/lib/redis/store/marshalling.rb
+++ /dev/null
@@ -1,56 +0,0 @@
-class Redis
-  class Store < self
-    module Marshalling
-      def set(key, value, options = nil)
-        _marshal(value, options) { |value| super encode(key), encode(value), options }
-      end
-
-      def setnx(key, value, options = nil)
-        _marshal(value, options) { |value| super encode(key), encode(value), options }
-      end
-
-      def setex(key, expiry, value, options = nil)
-        _marshal(value, options) { |value| super encode(key), expiry, encode(value), options }
-      end
-
-      def get(key, options = nil)
-        _unmarshal super(key), options
-      end
-
-      def mget(*keys)
-        options = keys.pop if keys.last.is_a?(Hash)
-        super(*keys).map do |result|
-          _unmarshal result, options
-        end
-      end
-
-      private
-        def _marshal(val, options)
-          yield marshal?(options) ? Marshal.dump(val) : val
-        end
-
-        def _unmarshal(val, options)
-          unmarshal?(val, options) ? Marshal.load(val) : val
-        end
-
-        def marshal?(options)
-          !(options && options[:raw])
-        end
-
-        def unmarshal?(result, options)
-          result && result.size > 0 && marshal?(options)
-        end
-
-        if defined?(Encoding)
-          def encode(string)
-            key = string.to_s.dup
-            key.force_encoding(Encoding::BINARY)
-          end
-        else
-          def encode(string)
-            string
-          end
-        end
-    end
-  end
-end
--- /dev/null
+++ b/lib/redis/store/serialization.rb
@@ -0,0 +1,56 @@
+class Redis
+  class Store < self
+    module Serialization
+      def set(key, value, options = nil)
+        _marshal(value, options) { |value| super encode(key), encode(value), options }
+      end
+
+      def setnx(key, value, options = nil)
+        _marshal(value, options) { |value| super encode(key), encode(value), options }
+      end
+
+      def setex(key, expiry, value, options = nil)
+        _marshal(value, options) { |value| super encode(key), expiry, encode(value), options }
+      end
+
+      def get(key, options = nil)
+        _unmarshal super(key), options
+      end
+
+      def mget(*keys)
+        options = keys.pop if keys.last.is_a?(Hash)
+        super(*keys).map do |result|
+          _unmarshal result, options
+        end
+      end
+
+      private
+        def _marshal(val, options)
+          yield marshal?(options) ? @serializer.dump(val) : val
+        end
+
+        def _unmarshal(val, options)
+          unmarshal?(val, options) ? @serializer.load(val) : val
+        end
+
+        def marshal?(options)
+          !(options && options[:raw])
+        end
+
+        def unmarshal?(result, options)
+          result && result.size > 0 && marshal?(options)
+        end
+
+        if defined?(Encoding)
+          def encode(string)
+            key = string.to_s.dup
+            key.force_encoding(Encoding::BINARY)
+          end
+        else
+          def encode(string)
+            string
+          end
+        end
+    end
+  end
+end
--- a/test/redis/store/factory_test.rb
+++ b/test/redis/store/factory_test.rb
@@ -1,4 +1,5 @@
 require 'test_helper'
+require 'json'
 
 describe "Redis::Store::Factory" do
   describe ".create" do
@@ -41,9 +42,11 @@
         store.instance_variable_get(:@client).password.must_equal("secret")
       end
 
-      it "allows/disable marshalling" do
-        store = Redis::Store::Factory.create :marshalling => false
-        store.instance_variable_get(:@marshalling).must_equal(false)
+
+      it "disables serialization" do
+        store = Redis::Store::Factory.create :serializer => nil
+        store.instance_variable_get(:@serializer).must_be_nil
+        store.instance_variable_get(:@options)[:raw].must_equal(true)
       end
 
       it "should instantiate a Redis::DistributedStore store" do
--- a/test/redis/store/namespace_test.rb
+++ b/test/redis/store/namespace_test.rb
@@ -3,7 +3,7 @@
 describe "Redis::Store::Namespace" do
   def setup
     @namespace = "theplaylist"
-    @store  = Redis::Store.new :namespace => @namespace, :marshalling => false # TODO remove mashalling option
+    @store  = Redis::Store.new :namespace => @namespace, :serializer => nil
     @client = @store.instance_variable_get(:@client)
     @rabbit = "bunny"
     @default_store = Redis::Store.new
@@ -77,7 +77,7 @@
   end
 
   describe 'method calls' do
-    let(:store){Redis::Store.new :namespace => @namespace, :marshalling => false}
+    let(:store){Redis::Store.new :namespace => @namespace, :serializer => nil}
     let(:client){store.instance_variable_get(:@client)}
 
     it "should namespace get" do
--- a/test/redis/store/marshalling_test.rb
+++ /dev/null
@@ -1,128 +0,0 @@
-require 'test_helper'
-
-describe "Redis::Marshalling" do
-  def setup
-    @store = Redis::Store.new :marshalling => true
-    @rabbit = OpenStruct.new :name => "bunny"
-    @white_rabbit = OpenStruct.new :color => "white"
-    @store.set "rabbit", @rabbit
-    @store.del "rabbit2"
-  end
-
-  def teardown
-    @store.flushdb
-    @store.quit
-  end
-
-  it "unmarshals on get" do
-    @store.get("rabbit").must_equal(@rabbit)
-  end
-
-  it "marshals on set" do
-    @store.set "rabbit", @white_rabbit
-    @store.get("rabbit").must_equal(@white_rabbit)
-  end
-
-  if RUBY_VERSION.match /1\.9/
-    it "doesn't unmarshal on get if raw option is true" do
-      @store.get("rabbit", :raw => true).must_equal("\x04\bU:\x0FOpenStruct{\x06:\tnameI\"\nbunny\x06:\x06EF")
-    end
-  else
-    it "doesn't unmarshal on get if raw option is true" do
-      @store.get("rabbit", :raw => true).must_include("\x04\bU:\x0FOpenStruct{\x06:\tname")
-    end
-  end
-
-  it "doesn't marshal set if raw option is true" do
-    @store.set "rabbit", @white_rabbit, :raw => true
-    @store.get("rabbit", :raw => true).must_equal(%(#<OpenStruct color="white">))
-  end
-
-  it "doesn't unmarshal if get returns an empty string" do
-    @store.set "empty_string", ""
-    @store.get("empty_string").must_equal("")
-    # TODO use a meaningful Exception
-    # lambda { @store.get("empty_string").must_equal("") }.wont_raise Exception
-  end
-
-  it "doesn't set an object if already exist" do
-    @store.setnx "rabbit", @white_rabbit
-    @store.get("rabbit").must_equal(@rabbit)
-  end
-
-  it "marshals on set unless exists" do
-    @store.setnx "rabbit2", @white_rabbit
-    @store.get("rabbit2").must_equal(@white_rabbit)
-  end
-
-  it "doesn't marshal on set unless exists if raw option is true" do
-    @store.setnx "rabbit2", @white_rabbit, :raw => true
-    @store.get("rabbit2", :raw => true).must_equal(%(#<OpenStruct color="white">))
-  end
-
-  it "marshals on set expire" do
-    @store.setex "rabbit2", 1, @white_rabbit
-    @store.get("rabbit2").must_equal(@white_rabbit)
-    sleep 2
-    @store.get("rabbit2").must_be_nil
-  end
-
-  it "doesn't unmarshal on multi get" do
-    @store.set "rabbit2", @white_rabbit
-    rabbit, rabbit2 = @store.mget "rabbit", "rabbit2"
-    rabbit.must_equal(@rabbit)
-    rabbit2.must_equal(@white_rabbit)
-  end
-
-  if RUBY_VERSION.match /1\.9/
-    it "doesn't unmarshal on multi get if raw option is true" do
-      @store.set "rabbit2", @white_rabbit
-      rabbit, rabbit2 = @store.mget "rabbit", "rabbit2", :raw => true
-      rabbit.must_equal("\x04\bU:\x0FOpenStruct{\x06:\tnameI\"\nbunny\x06:\x06EF")
-      rabbit2.must_equal("\x04\bU:\x0FOpenStruct{\x06:\ncolorI\"\nwhite\x06:\x06EF")
-    end
-  else
-    it "doesn't unmarshal on multi get if raw option is true" do
-      @store.set "rabbit2", @white_rabbit
-      rabbit, rabbit2 = @store.mget "rabbit", "rabbit2", :raw => true
-      rabbit.must_include("\x04\bU:\x0FOpenStruct{\x06:\tname")
-      rabbit2.must_include("\x04\bU:\x0FOpenStruct{\x06:\ncolor")
-    end
-  end
-
-  describe "binary safety" do
-    it "marshals objects" do
-      utf8_key = [51339].pack("U*")
-      ascii_rabbit = OpenStruct.new(:name => [128].pack("C*"))
-
-      @store.set(utf8_key, ascii_rabbit)
-      @store.get(utf8_key).must_equal(ascii_rabbit)
-    end
-
-    it "gets and sets raw values" do
-      utf8_key = [51339].pack("U*")
-      ascii_string = [128].pack("C*")
-
-      @store.set(utf8_key, ascii_string, :raw => true)
-      @store.get(utf8_key, :raw => true).bytes.to_a.must_equal(ascii_string.bytes.to_a)
-    end
-
-    it "marshals objects on setnx" do
-      utf8_key = [51339].pack("U*")
-      ascii_rabbit = OpenStruct.new(:name => [128].pack("C*"))
-
-      @store.del(utf8_key)
-      @store.setnx(utf8_key, ascii_rabbit)
-      @store.get(utf8_key).must_equal(ascii_rabbit)
-    end
-
-    it "gets and sets raw values on setnx" do
-      utf8_key = [51339].pack("U*")
-      ascii_string = [128].pack("C*")
-
-      @store.del(utf8_key)
-      @store.setnx(utf8_key, ascii_string, :raw => true)
-      @store.get(utf8_key, :raw => true).bytes.to_a.must_equal(ascii_string.bytes.to_a)
-    end
-  end if defined?(Encoding)
-end
--- /dev/null
+++ b/test/redis/store/serialization_test.rb
@@ -0,0 +1,128 @@
+require 'test_helper'
+
+describe "Redis::Serialization" do
+  def setup
+    @store = Redis::Store.new serializer: Marshal
+    @rabbit = OpenStruct.new :name => "bunny"
+    @white_rabbit = OpenStruct.new :color => "white"
+    @store.set "rabbit", @rabbit
+    @store.del "rabbit2"
+  end
+
+  def teardown
+    @store.flushdb
+    @store.quit
+  end
+
+  it "unmarshals on get" do
+    @store.get("rabbit").must_equal(@rabbit)
+  end
+
+  it "marshals on set" do
+    @store.set "rabbit", @white_rabbit
+    @store.get("rabbit").must_equal(@white_rabbit)
+  end
+
+  if RUBY_VERSION.match /1\.9/
+    it "doesn't unmarshal on get if raw option is true" do
+      @store.get("rabbit", :raw => true).must_equal("\x04\bU:\x0FOpenStruct{\x06:\tnameI\"\nbunny\x06:\x06EF")
+    end
+  else
+    it "doesn't unmarshal on get if raw option is true" do
+      @store.get("rabbit", :raw => true).must_include("\x04\bU:\x0FOpenStruct{\x06:\tname")
+    end
+  end
+
+  it "doesn't marshal set if raw option is true" do
+    @store.set "rabbit", @white_rabbit, :raw => true
+    @store.get("rabbit", :raw => true).must_equal(%(#<OpenStruct color="white">))
+  end
+
+  it "doesn't unmarshal if get returns an empty string" do
+    @store.set "empty_string", ""
+    @store.get("empty_string").must_equal("")
+    # TODO use a meaningful Exception
+    # lambda { @store.get("empty_string").must_equal("") }.wont_raise Exception
+  end
+
+  it "doesn't set an object if already exist" do
+    @store.setnx "rabbit", @white_rabbit
+    @store.get("rabbit").must_equal(@rabbit)
+  end
+
+  it "marshals on set unless exists" do
+    @store.setnx "rabbit2", @white_rabbit
+    @store.get("rabbit2").must_equal(@white_rabbit)
+  end
+
+  it "doesn't marshal on set unless exists if raw option is true" do
+    @store.setnx "rabbit2", @white_rabbit, :raw => true
+    @store.get("rabbit2", :raw => true).must_equal(%(#<OpenStruct color="white">))
+  end
+
+  it "marshals on set expire" do
+    @store.setex "rabbit2", 1, @white_rabbit
+    @store.get("rabbit2").must_equal(@white_rabbit)
+    sleep 2
+    @store.get("rabbit2").must_be_nil
+  end
+
+  it "doesn't unmarshal on multi get" do
+    @store.set "rabbit2", @white_rabbit
+    rabbit, rabbit2 = @store.mget "rabbit", "rabbit2"
+    rabbit.must_equal(@rabbit)
+    rabbit2.must_equal(@white_rabbit)
+  end
+
+  if RUBY_VERSION.match /1\.9/
+    it "doesn't unmarshal on multi get if raw option is true" do
+      @store.set "rabbit2", @white_rabbit
+      rabbit, rabbit2 = @store.mget "rabbit", "rabbit2", :raw => true
+      rabbit.must_equal("\x04\bU:\x0FOpenStruct{\x06:\tnameI\"\nbunny\x06:\x06EF")
+      rabbit2.must_equal("\x04\bU:\x0FOpenStruct{\x06:\ncolorI\"\nwhite\x06:\x06EF")
+    end
+  else
+    it "doesn't unmarshal on multi get if raw option is true" do
+      @store.set "rabbit2", @white_rabbit
+      rabbit, rabbit2 = @store.mget "rabbit", "rabbit2", :raw => true
+      rabbit.must_include("\x04\bU:\x0FOpenStruct{\x06:\tname")
+      rabbit2.must_include("\x04\bU:\x0FOpenStruct{\x06:\ncolor")
+    end
+  end
+
+  describe "binary safety" do
+    it "marshals objects" do
+      utf8_key = [51339].pack("U*")
+      ascii_rabbit = OpenStruct.new(:name => [128].pack("C*"))
+
+      @store.set(utf8_key, ascii_rabbit)
+      @store.get(utf8_key).must_equal(ascii_rabbit)
+    end
+
+    it "gets and sets raw values" do
+      utf8_key = [51339].pack("U*")
+      ascii_string = [128].pack("C*")
+
+      @store.set(utf8_key, ascii_string, :raw => true)
+      @store.get(utf8_key, :raw => true).bytes.to_a.must_equal(ascii_string.bytes.to_a)
+    end
+
+    it "marshals objects on setnx" do
+      utf8_key = [51339].pack("U*")
+      ascii_rabbit = OpenStruct.new(:name => [128].pack("C*"))
+
+      @store.del(utf8_key)
+      @store.setnx(utf8_key, ascii_rabbit)
+      @store.get(utf8_key).must_equal(ascii_rabbit)
+    end
+
+    it "gets and sets raw values on setnx" do
+      utf8_key = [51339].pack("U*")
+      ascii_string = [128].pack("C*")
+
+      @store.del(utf8_key)
+      @store.setnx(utf8_key, ascii_string, :raw => true)
+      @store.get(utf8_key, :raw => true).bytes.to_a.must_equal(ascii_string.bytes.to_a)
+    end
+  end if defined?(Encoding)
+end
--- a/lib/redis/store/factory.rb
+++ b/lib/redis/store/factory.rb
@@ -50,6 +50,14 @@
         if options.key?(:key_prefix) && !options.key?(:namespace)
           options[:namespace] = options.delete(:key_prefix) # RailsSessionStore
         end
+        options[:raw] = case
+                        when options.key?(:serializer)
+                          options[:serializer].nil?
+                        when options.key?(:marshalling)
+                          !options[:marshalling]
+                        else
+                          false
+                        end
         options
       end
 
--- a/lib/redis/store/namespace.rb
+++ b/lib/redis/store/namespace.rb
@@ -44,8 +44,8 @@
       def mget(*keys)
         options = keys.pop if keys.last.is_a? Hash
         if keys.any?
-          # Marshalling gets extended before Namespace does, so we need to pass options further
-          if singleton_class.ancestors.include? Marshalling
+          # Serialization gets extended before Namespace does, so we need to pass options further
+          if singleton_class.ancestors.include? Serialization
             super *keys.map {|key| interpolate(key) }, options
           else
             super *keys.map {|key| interpolate(key) }