CVE-2017-1000248: Deserialization of Untrusted Data (CWE-502)
value: Redis-store <=v1.3.0 allows unsafe objects to be loaded from redis
CWE description: The product deserializes untrusted data without sufficiently verifying that the resulting data will be valid.
CVSS v3 base score: 9.8
Published: 2017-11-17T04:29Z
Repository: https://github.com/redis-store/redis-store
Language: Ruby
Files changed: lib/redis-store.rb, lib/redis/store.rb, lib/redis/store/factory.rb, lib/redis/store/namespace.rb, lib/redis/store/serialization.rb, test/redis/store/factory_test.rb, test/redis/store/namespace_test.rb, test/redis/store/serialization_test.rb
Commit date: 2017-08-15 11:46:36 -0400
Commit message: Replace marshalling with pluggable serializers

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

Diff: diff --git a/lib/redis-store.rb b/lib/redis-store.rb
index 5b33c29..a7de2c0 100644
--- a/lib/redis-store.rb
+++ b/lib/redis-store.rb
@@ -1,13 +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'
-require 'redis/store/redis_version'
-
-class Redis
-  class Store < self
-  end
-end
diff --git a/lib/redis/store.rb b/lib/redis/store.rb
index f7c6262..18088d7 100644
--- a/lib/redis/store.rb
+++ b/lib/redis/store.rb
@@ -1,15 +1,40 @@
+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/redis_version'
 require 'redis/store/ttl'
 require 'redis/store/interface'
 require 'redis/store/redis_version'
 
 class Redis
   class Store < self
     include Ttl, Interface, RedisVersion
 
     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
 
     def reconnect
@@ -21,12 +46,11 @@ class Redis
       "Redis Client connected to #{/:/ =~ h ? '['+h+']' : h}:#{@client.port} against DB #{@client.db}"
     end
 
     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)
         @namespace = options[:namespace]
         extend Namespace
diff --git a/lib/redis/store/factory.rb b/lib/redis/store/factory.rb
index 598e210..9829f2d 100644
--- a/lib/redis/store/factory.rb
+++ b/lib/redis/store/factory.rb
@@ -48,11 +48,18 @@ class Redis
       def self.normalize_key_names(options)
         options = options.dup
         if options.key?(:key_prefix) && !options.key?(:namespace)
           options[:namespace] = options.delete(:key_prefix) # RailsSessionStore
         end
-        options[:raw] = !options[:marshalling]
+        options[:raw] = case
+                        when options.key?(:serializer)
+                          options[:serializer].nil?
+                        when options.key?(:marshalling)
+                          !options[:marshalling]
+                        else
+                          false
+                        end
         options
       end
 
       def self.host_options?(options)
         if options.keys.any? {|n| [:host, :db, :port].include?(n) }
diff --git a/lib/redis/store/namespace.rb b/lib/redis/store/namespace.rb
index 2eb3b40..99e41c8 100644
--- a/lib/redis/store/namespace.rb
+++ b/lib/redis/store/namespace.rb
@@ -44,12 +44,12 @@ class Redis
       end
 
       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) })
           end
         end
diff --git a/lib/redis/store/marshalling.rb b/lib/redis/store/serialization.rb
similarity index 90%
rename from lib/redis/store/marshalling.rb
rename to lib/redis/store/serialization.rb
index c2e51e4..0838d15 100644
--- a/lib/redis/store/marshalling.rb
+++ b/lib/redis/store/serialization.rb
@@ -1,8 +1,8 @@
 class Redis
   class Store < self
-    module Marshalling
+    module Serialization
       def set(key, value, options = nil)
         _marshal(value, options) { |v| super encode(key), encode(v), options }
       end
 
       def setnx(key, value, options = nil)
@@ -34,15 +34,15 @@ class Redis
         super(*updates)
       end
 
       private
         def _marshal(val, options)
-          yield marshal?(options) ? Marshal.dump(val) : val
+          yield marshal?(options) ? @serializer.dump(val) : val
         end
 
         def _unmarshal(val, options)
-          unmarshal?(val, options) ? Marshal.load(val) : val
+          unmarshal?(val, options) ? @serializer.load(val) : val
         end
 
         def marshal?(options)
           !(options && options[:raw])
         end
diff --git a/test/redis/store/factory_test.rb b/test/redis/store/factory_test.rb
index 74cd7af..73a6691 100644
--- a/test/redis/store/factory_test.rb
+++ b/test/redis/store/factory_test.rb
@@ -1,6 +1,7 @@
 require 'test_helper'
+require 'json'
 
 describe "Redis::Store::Factory" do
   describe ".create" do
     describe "when not given any arguments" do
       it "instantiates Redis::Store" do
@@ -49,16 +50,49 @@ describe "Redis::Store::Factory" do
       it 'uses nil password' do
         store = Redis::Store::Factory.create :password => nil
         assert_nil(store.instance_variable_get(:@client).password)
       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 "configures pluggable serialization backend" do
+        store = Redis::Store::Factory.create :serializer => JSON
+        store.instance_variable_get(:@serializer).must_equal(JSON)
+        store.instance_variable_get(:@options)[:raw].must_equal(false)
+      end
+
+      describe 'with stdout disabled' do
+        before do
+          @original_stderr = $stderr
+          @original_stdout = $stdout
+
+          $stderr = Tempfile.new('stderr')
+          $stdout = Tempfile.new('stdout')
+        end
+
+        it "disables marshalling and provides deprecation warning" do
+          store = Redis::Store::Factory.create :marshalling => false
+          store.instance_variable_get(:@serializer).must_be_nil
+          store.instance_variable_get(:@options)[:raw].must_equal(true)
+        end
+
+        it "enables marshalling but provides warning to use :serializer instead" do
+          store = Redis::Store::Factory.create :marshalling => true
+          store.instance_variable_get(:@serializer).must_equal(Marshal)
+          store.instance_variable_get(:@options)[:raw].must_equal(false)
+        end
+
+        after do
+          $stderr = @original_stderr
+          $stdout = @original_stdout
+        end
+      end
+
       it "should instantiate a Redis::DistributedStore store" do
         store = Redis::Store::Factory.create(
           {:host => "localhost", :port => 6379},
           {:host => "localhost", :port => 6380}
         )
diff --git a/test/redis/store/namespace_test.rb b/test/redis/store/namespace_test.rb
index ac991f7..eac7ad1 100644
--- a/test/redis/store/namespace_test.rb
+++ b/test/redis/store/namespace_test.rb
@@ -1,11 +1,11 @@
 require 'test_helper'
 
 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
     @other_namespace = 'other'
     @other_store = Redis::Store.new :namespace => @other_namespace
@@ -88,11 +88,11 @@ describe "Redis::Store::Namespace" do
     @other_store.ttl('foo').must_equal(-2)
     @other_store.get('foo').must_be_nil
   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
        client.expects(:call).with([:get, "#{@namespace}:rabbit"]).once
        store.get("rabbit")
diff --git a/test/redis/store/marshalling_test.rb b/test/redis/store/serialization_test.rb
similarity index 98%
rename from test/redis/store/marshalling_test.rb
rename to test/redis/store/serialization_test.rb
index b4e49b1..9dac9eb 100644
--- a/test/redis/store/marshalling_test.rb
+++ b/test/redis/store/serialization_test.rb
@@ -1,10 +1,10 @@
 require 'test_helper'
 
-describe "Redis::Marshalling" do
+describe "Redis::Serialization" do
   def setup
-    @store = Redis::Store.new :marshalling => true
+    @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

Vulnerable code: require 'redis'
require 'redis/store/factory'
require 'redis/distributed_store'
require 'redis/store/namespace'
require 'redis/store/marshalling'
require 'redis/store/version'
require 'redis/store/redis_version'

class Redis
  class Store < self
  end
end
        @marshalling = !(options[:marshalling] === false) # HACK - TODO delegate to Factory
        extend Marshalling if @marshalling
        options[:raw] = !options[:marshalling]
          # Marshalling gets extended before Namespace does, so we need to pass options further
          if singleton_class.ancestors.include? Marshalling
    module Marshalling
          yield marshal?(options) ? Marshal.dump(val) : val
          unmarshal?(val, options) ? Marshal.load(val) : val
      it "allows/disable marshalling" do
        store = Redis::Store::Factory.create :marshalling => false
        store.instance_variable_get(:@marshalling).must_equal(false)
    @store  = Redis::Store.new :namespace => @namespace, :marshalling => false # TODO remove mashalling option
    let(:store){Redis::Store.new :namespace => @namespace, :marshalling => false}
describe "Redis::Marshalling" do
    @store = Redis::Store.new :marshalling => true
Fixed code: 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/redis_version'

      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 Serialization unless @serializer.nil?
        options[:raw] = case
                        when options.key?(:serializer)
                          options[:serializer].nil?
                        when options.key?(:marshalling)
                          !options[:marshalling]
                        else
                          false
                        end
          # Serialization gets extended before Namespace does, so we need to pass options further
          if singleton_class.ancestors.include? Serialization
    module Serialization
          yield marshal?(options) ? @serializer.dump(val) : val
          unmarshal?(val, options) ? @serializer.load(val) : val
require 'json'
      it "disables serialization" do
        store = Redis::Store::Factory.create :serializer => nil
        store.instance_variable_get(:@serializer).must_be_nil
      it "configures pluggable serialization backend" do
        store = Redis::Store::Factory.create :serializer => JSON
        store.instance_variable_get(:@serializer).must_equal(JSON)
        store.instance_variable_get(:@options)[:raw].must_equal(false)
      end

      describe 'with stdout disabled' do
        before do
          @original_stderr = $stderr
          @original_stdout = $stdout

          $stderr = Tempfile.new('stderr')
          $stdout = Tempfile.new('stdout')
        end

        it "disables marshalling and provides deprecation warning" do
          store = Redis::Store::Factory.create :marshalling => false
          store.instance_variable_get(:@serializer).must_be_nil
          store.instance_variable_get(:@options)[:raw].must_equal(true)
        end

        it "enables marshalling but provides warning to use :serializer instead" do
          store = Redis::Store::Factory.create :marshalling => true
          store.instance_variable_get(:@serializer).must_equal(Marshal)
          store.instance_variable_get(:@options)[:raw].must_equal(false)
        end

        after do
          $stderr = @original_stderr
          $stdout = @original_stdout
        end
      end

    @store  = Redis::Store.new :namespace => @namespace, :serializer => nil
    let(:store){Redis::Store.new :namespace => @namespace, :serializer => nil}
describe "Redis::Serialization" do
    @store = Redis::Store.new serializer: Marshal