Skip to content
Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Tom Scott committed Aug 15, 2017
1 parent 589d2d0 commit e0c1398
Show file tree
Hide file tree
Showing 8 changed files with 80 additions and 27 deletions.
12 changes: 0 additions & 12 deletions 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
28 changes: 26 additions & 2 deletions lib/redis/store.rb
@@ -1,3 +1,10 @@
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'
Expand All @@ -8,6 +15,24 @@ class Store < self

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
Expand All @@ -23,8 +48,7 @@ def to_s

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)
Expand Down
9 changes: 8 additions & 1 deletion lib/redis/store/factory.rb
Expand Up @@ -50,7 +50,14 @@ def self.normalize_key_names(options)
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

Expand Down
4 changes: 2 additions & 2 deletions lib/redis/store/namespace.rb
Expand Up @@ -46,8 +46,8 @@ def del(*keys)
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) })
Expand Down
@@ -1,6 +1,6 @@
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
Expand Down Expand Up @@ -36,11 +36,11 @@ def mset(*args)

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)
Expand Down
40 changes: 37 additions & 3 deletions test/redis/store/factory_test.rb
@@ -1,4 +1,5 @@
require 'test_helper'
require 'json'

describe "Redis::Store::Factory" do
describe ".create" do
Expand Down Expand Up @@ -51,12 +52,45 @@
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},
Expand Down
4 changes: 2 additions & 2 deletions test/redis/store/namespace_test.rb
Expand Up @@ -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
Expand Down Expand Up @@ -90,7 +90,7 @@ def teardown
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
Expand Down
@@ -1,8 +1,8 @@
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
Expand Down

0 comments on commit e0c1398

Please sign in to comment.