Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't change the default session serializer (Marshal) #12881

Closed
mattetti opened this issue Nov 14, 2013 · 28 comments
Closed

Can't change the default session serializer (Marshal) #12881

mattetti opened this issue Nov 14, 2013 · 28 comments

Comments

@mattetti
Copy link
Contributor

Rails 4 currently allows sessions to be signed and/or encrypted which is great.

ActionPack relies on EncryptedCookieJar which itself relies on ActiveSupport::MessageEncryptor

Here is the current constructor:

    class EncryptedCookieJar #:nodoc:

      def initialize(parent_jar, key_generator, options = {})
        if ActiveSupport::LegacyKeyGenerator === key_generator
          raise "You didn't set config.secret_key_base, which is required for this cookie jar. " +
            "Read the upgrade documentation to learn more about this new config option."
        end

        @parent_jar = parent_jar
        @options = options
        secret = key_generator.generate_key(@options[:encrypted_cookie_salt])
        sign_secret = key_generator.generate_key(@options[:encrypted_signed_cookie_salt])
        @encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret)
      end

The encryptor can accept a serializer, if a serializer isn't passed, then Marshal is used.

class MessageEncryptor
    def initialize(secret, *signature_key_or_options)
      options = signature_key_or_options.extract_options!
      sign_secret = signature_key_or_options.first
      @secret = secret
      @sign_secret = sign_secret
      @cipher = options[:cipher] || 'aes-256-cbc'
      @verifier = MessageVerifier.new(@sign_secret || @secret, :serializer => NullSerializer)
      @serializer = options[:serializer] || Marshal
    end

The serializer is used to load/dump the session content.

I understand that we can't easily change the default serializer for historic reasons, but I would like to be able to use my own serializer (json, messagepack or whatever).

There are a few reasons for that, one mentioned by @tarcieri is security reason.
If the secret is leaked, the attacker can potentially execute ruby code on the server.

The second is to share the session across apps written in a different language.

Changing the serializer would obviously not guarantee that you can shove just whatever you want in the session like you currently can do with Marshal. Dumping the session object using a more strict serializer would potentially result in exceptions if the data format isn't supported and that's totally fine, actually I would like that.

I'm not asking to change the default, just to have a way to set my own serializer and deal with the consequences of my own choices. In other words, I'd like to be able to have a high level config flag to set the session serializer.

Others before me faced the same problem and monkey patched Rails: https://gist.github.com/jeffyip/4091166

I think an option would be a cleaner and safer alternative.

What do you think?

@NZKoz
Copy link
Member

NZKoz commented Nov 14, 2013

Ideally we'd have the option, default new apps to JSON and document the reasons you may want to flip to something else (in particular, people store objects in the session which aren't roundtrippable using JSON).

This would require a little bit of documentation and carefully considering the various error cases and how to securely handle them. I'm totally keen to apply a pull request which does this.

@mattetti
Copy link
Contributor Author

That's great news @NZKoz.
It looks like @jeffyip / @airbnb might like to be involved with the patch since they maintain their own version (explanation there: http://nerds.airbnb.com/upgrading-from-ree-187-to-ruby-193/ )

@rafaelfranca
Copy link
Member

👍 for @NZKoz plan.

[OFF TOPIC] We usually don't take feature requests in the issues tracker.
It is totally fine to open PR so we can discuss about the code, or to open
issues you are facing in your application/library, but for feature requests
we prefer that we use the Rails Core mailing list. And here is why: as a
person that live on this issue tracker, it is really hard to focus on
people problems if we have a lot of noise on it. This noise can be stale PR
and issues, questions, or feature requests. Sure, all these things are
improtant but they have also their proper place. Rails is a big project and
this issue tracker is full of work to do, so I really appreciate if we
could make my life (and the life of people working in the Rails issues
team) easier keeping the issues number small.

On Thursday, November 14, 2013, Michael Koziarski wrote:

Ideally we'd have the option, default new apps to JSON and document the
reasons you may want to flip to something else (in particular, people store
objects in the session which aren't roundtrippable using JSON).

This would require a little bit of documentation and carefully considering
the various error cases and how to securely handle them. I'm totally keen
to apply a pull request which does this.


Reply to this email directly or view it on GitHubhttps://github.com//issues/12881#issuecomment-28456230
.

Rafael Mendonça França
http://twitter.com/rafaelfranca
https://github.com/rafaelfranca

@mattetti
Copy link
Contributor Author

@rafaelfranca understood, thanks for your patience.

@mattetti
Copy link
Contributor Author

Here is a MVP showing the bare minimal monkey patching required to switch the main cookies from Marshal to JSON: https://gist.github.com/mattetti/7624413

The patch is divided in 3 parts:

  • a JSON wrapper so cookie parsing issues are silently ignored (should really be moved somewhere else)
  • the SignedCookieJar initializer patch to set the verifier with a passed or default serializer (JSON)
  • the EncryptedCookieJar initializer patch which does the same thing.

Part of the issue is that ActiveSupport's MessageEncryptor and MessageVerifier use Marshal as default serializer. Ideally, we would want AS to switch that to a safer default. Since that might not be possible, we should scan the entire code base to look for calls to these 2 classes and set our own more sensitive default.

Finally, we would need a way to set the serializer at the session_store method level, probably something like:

  Splice::Application.config.session_store :cookie_store, key: '_splice_session', domain: "splice.com", serializer: JSON

Getting a PR ready is quite a lot of work since as @NZKoz mentioned, documentation, upgrade/migration testing are needed. But at the same time, this is for me a major security concern, especially when working in big companies where usually employees have access to the session secret and can easily tamper with the cookie after they leave the company and execute any kind of nasty code server side.

@Bertg
Copy link
Contributor

Bertg commented Nov 25, 2013

Actually it would be possible to make a serialiser that will fall back to loading via Marshal, and will always dump as JSON. This could be a solution to migrate all sessions to a JSON serialised format.

@mattetti
Copy link
Contributor Author

It would still make the cookie content unsafe to load. I personally don't
think that's a good idea (that said airbnb does as you suggest by detecting
the format using the magic header of the message data and use marshal or
json accordingly)

@Bertg
Copy link
Contributor

Bertg commented Nov 25, 2013

I'm aware that it would still be unsafe, however it's a viable upgrade path for the people that don't want to invalidate all their current sessions, but still want to migrate to a safer serialiser.

@jeremy
Copy link
Member

jeremy commented Nov 25, 2013

Note that you're using the JSON.load/dump API, which is for marshaling Ruby objects. As long as you're unmarshaling Ruby objects, RCE is possible. Changing the format is a red herring and has no bearing on that.

Use the JSON.parse/generate API to restrain usage to JSON primitives.

@mattetti
Copy link
Contributor Author

@jeremy good call, you're absolutely right, fixing that now.

@rosenfeld
Copy link
Contributor

"The second is to share the session across apps written in a different language.", the application I maintain has this exact use case. In our case, a Grails application handles authentication (using Shiro) but some actions are performed in the Rails app. For simplicity, our client get cookies from both applications. In the Rails side, an authentication request will go through a custom strategy that will forward the cookies to the Grails app and check if it's valid. If so, it will create a Devise valid cookie valid for up to 10 minutes of inactivity. When signing out from Grails, the Rails cookie is deleted as well. This is not ideal, for some reasons like replay attacks targetting the Rails app after logout and other reasons. So, I totally support a cross-language serializer method for supporting cases like this.

@jeffrafter
Copy link

Hi all, I dropped this patch into my application to see how it would handle my existing sessions and immediately ran into JSON::GeneratorError in SessionsController#create, only generation of JSON objects or arrays allowed. I cleared the (now stale) cookies from the browser and the error persisted.

This was generated by the following code:

def set_remember_cookie
  cookies.permanent.signed[:remember] = {
    value: current_user.remember_token,
    secure: Rails.env.production?
  }
end

The token is a string and this was not serializable. Wrapping it in an array gets me past the error. This could be a bug in the my code (and this could be a bad approach) but it is common.

@chancancode
Copy link
Member

@jeffrafter / @mattetti, if you want to (de)serialize partial JSON (strings, numbers etc) you need to pass quirks_mode: true to JSON.{generate,parse}:

>> require 'json'
=> true
>> JSON.generate('hello')
JSON::GeneratorError: only generation of JSON objects or arrays allowed
    from /Users/godfrey/.rvm/gems/ruby-2.0.0-p195/gems/json-1.8.1/lib/json/common.rb:223:in `generate'
    from /Users/godfrey/.rvm/gems/ruby-2.0.0-p195/gems/json-1.8.1/lib/json/common.rb:223:in `generate'
    from (irb):3
    from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p195/bin/irb:16:in `<main>'
>> JSON.generate('hello', quirks_mode: true)
=> "\"hello\""

@mattetti
Copy link
Contributor Author

mattetti commented Dec 4, 2013

@chancancode good point, I didn't know about quirks_mode to generate invalid JSON.

@rosenfeld
Copy link
Contributor

I didn't know such values were invalid: http://www.json.org/. Indeed Chrome doesn't complain about "JSON.stringify('value')". I don't really understand why Ruby's JSON#unparse complains about this by default...

@jeffrafter
Copy link

@chancancode / @mattetti I can confirm that quirks_mode solves the problem. I looked into .to_json as well in thinking about multi_json support that looks to fallback to dump/load.

@chancancode
Copy link
Member

👎 on MultiJson, it has been removed from Rails master (see #13109).

If anything we should make this go through ActiveSupport::JSON.encode and AS::JSON.decode unless there are provable perf concerns


Sent from Mailbox for iPhone

On Wed, Dec 4, 2013 at 8:04 AM, Jeff Rafter notifications@github.com
wrote:

@chancancode / @mattetti I can confirm that quirks_mode solves the problem. I looked into .to_json as well in thinking about multi_json support that looks to fallback to dump/load.

Reply to this email directly or view it on GitHub:
#12881 (comment)

@jeffrafter
Copy link

Actually, another change was needed: create_additions: true. Without this, the quirks_mode generated tokens wouldn't parse. @chancancode I looked at your commit here https://github.com/rails/rails/pull/12207/files and you seemed to do the same. This fixed the last of my problems with signed single token cookies.

@chancancode
Copy link
Member

@jeffrafter: don't do that :( create_additions: true is exactly what makes JSON.{dump,load} dangerous: they make it possible to dump and load arbitrary Ruby objects instead of just the basic primitives (strings, numbers, true, false, nil, and array/hash containing these).

You'll need to pass quirks_mode: false to both JSON.generate and JSON.parse instead. If it still doesn't parse, then something else is broken.

This raises another point that should be considered along with this change. With JSON, only the JSON primitives can be deserialized correctly (e.g. if you pass a symbol or a Time object, it'll come out as string on the next request). This might be slightly too narrow for the expected usage of sessions? (Yes, create_additions: true "fixes" this, but that kind of defeats the entire purpose of this change, and will probably open up more cans of worms than it fixes.)

@betelgeuse
Copy link

@chancancode when it's easy to change the serializers then using yaml provides a round trip for dates at least:

2.0.0p247 :007 > YAML.load(YAML.dump({foo: Time.now}), safe: true)
 => {":foo"=>2013-12-13 23:05:25 UTC}

@chancancode
Copy link
Member

Not symbols though (and for good reason):

>> require 'safe_yaml'
=> true
>> YAML.load(YAML.dump(:symbol), safe: true)
=> ":symbol"

By the way, I'm not saying any of these restrictions are bad, I'm just saying they need to be understood properly.

@lukesarnacki
Copy link
Contributor

@mattetti Have you started or are you planning to start that PR? I am thinking about coding this, so just wanted to check status.

@mattetti
Copy link
Contributor Author

mattetti commented Jan 8, 2014

Unfortunately, I haven't started yet. Please feel feel to start
On Jan 8, 2014 7:31 AM, "Łukasz Sarnacki" notifications@github.com wrote:

@mattetti https://github.com/mattetti Have you started or are you
planning to start that PR? I am thinking about coding this, so just wanted
to check status.


Reply to this email directly or view it on GitHubhttps://github.com//issues/12881#issuecomment-31844173
.

@lukesarnacki
Copy link
Contributor

I created PR with what I came up with. It needs some more work, but as this is my first commit to rails where I actually add a feature, it would be awesome to hear if this is good direction at this stage.

@Bertg
Copy link
Contributor

Bertg commented Jan 13, 2014

@lukesarnacki That PR actually looks really good. +1

@lukesarnacki
Copy link
Contributor

@Bertg thanks ;) But I already noticed some kind of stupid mistakes ;) It breaks MessageEncryptor tests which I missed. I will add some fixes soon.

@lukesarnacki
Copy link
Contributor

As an update: I added test which actually checks if serializer passed in config is used, I fixed test suite, I changed load / dump to parse / generate with quirks_mode as @chancancode suggested and I think PR is ready. Please let me know if there is anything missing.

@rafaelfranca
Copy link
Member

Closed by #13692

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants