Description: Replace marshalling with pluggable serializers Author: Tom Scott 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(%(#)) - 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(%(#)) - 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(%(#)) + 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(%(#)) + 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) }