Make token interception more difficult

[atmos]

Right now it's fairly easy to intercept or mitm a rack session cookie and grab the user's oauth token from it. If you inspect the request headers for a browser session and look at the rack.session cookie you can pretty trivially get the token with something like this.

require 'cgi'
require 'base64'
require 'pp'
require 'ostruct'

def unmarshal(raw)
  begin
    Marshal.load raw
  rescue ArgumentError => e
    klass = e.message.split(' ').last.sub(/:+$/, '')
    klass.split('::').inject(Object) do |parent, k|
      parent.const_defined?(k) ? parent.const_get(k) : parent.const_set(k, Class.new(OpenStruct))
    end
    unmarshal raw
  end
end

STDOUT.write "Enter a session cookie: "
cookie = STDIN.gets.chomp
decoded = Base64.decode64(CGI.unescape(cookie.split('--').first))
unmarshaled = unmarshal decoded

PP.pp unmarshaled

There's a couple of questions for how this gets adopted. Do we make a new warden config option for it? Conditionally enable it if an environmental variable is set? Make it a breaking required change in order to get future support? Are there any downsides to requiring it?

@fphilipe Any drawbacks you can see bubbling this up to warden-github-rails?
@mastahyeti is this a correct fix to what we discussed previously around this issue? I can't extract the sessions anymore with burp.

[atmos]

Is there a better way to do this without monkey patching the whole class? These maybe?

[atmos]

Could we maybe use the client secret here?

[btoews]

It would be better not to reuse the secret. It just makes things more complicated when the secret needs to change.

[fphilipe]

@atmos This doesn't affect Rails, where cookies are signed, does it? I'll have a closer look later on but I don't think that this would have any effect on warden-github-rails as long as the warden-github's interface stays the same 😃

[btoews]

Is the serialized value normally a Hash? If so, it be less likely to cause a 500 in the app if this returned an empty Hash. Unless of course, this is only called from within Warden and it is checking for nil.

[btoews]

There's a couple of questions for how this gets adopted. Do we make a new warden config option for it? Conditionally enable it if an environmental variable is set? Make it a breaking required change in order to get future support? Are there any downsides to requiring it?

It would be good to give a transition path. In the transitional mode, it could accept old and new cookies, but replace the old cookies with new ones. Apps could run that way for a few days before switching over.

[atmos]

This doesn't affect Rails, where cookies are signed, does it?

This makes me want to just ensure that the cookie we're storing to is signed instead of adding this extra stuff. I wonder if there's a way to do that.

[fphilipe]

[…] just ensure that the cookie we're storing to is signed […]

AFAIK rack itself doesn't have any cookie signing and every framework implements it in its own way. That said, I doubt there's any other way than signing the information in warden-github itself.

[btoews]

✨

[keithduncan]

I think this should be verifier_key

[atmos]

I definitely have it working in simpler apps but there's problems with JSON user serialization in rails 4 and warden-github-rails that I'm hoping to have time for later this week.

[keithduncan]

Yep you’re right, I misread this. I’ve been seeing exceptions where #split is sent to a Warden user instance and thought it was here, it’s possibly an already decoded instance being decoded again.

[fphilipe]

@atmos So there's an open pull request fphilipe/warden-github-rails#12, but I'm not sure it's really required. Am I correct in assuming that you implemented this because, by default, most frameworks including sinatra don't encrypt the cookies? And since rails does encrypt the cookies, there's not much point in adding this to warden-github-rails?

[atmos]

Yup. If I remember right rails 3 did cookie verification, so the session could still be inspected and expose the oauth token, but the session couldn't be modified. I think rails 4 was the first to encrypt them, which is what you're getting from the warden-github verifier. There's probably a good bit of value in supporting both rails 3 and 4, but that'd require adding more stuff to warden-github. The problem is that switching between the different cookie types throws a ridiculous error that only fixes itself after you clear your browser cookies. It might just be easiest to throw a warning saying that people should be on > rails 4 and not change anything.

I'm using it in heaven without specifying the verifier stuff. I wrote the warden extensions to cover our sinatra and warden needs at GitHub.

[fphilipe]

@atmos Just got a bug report related to marshaling (fphilipe/warden-github-rails#16). This change introduced the bug. The problem is that this change now hard codes the class User, instantiates it and calls #marshal_load on it. This is all good, except when using the MockUser in warden-github-rails. Any ideas how to work around this? Would you be open to serializing the class name as well or is hard-coding User a security measure? If not, we could have something like this instead:

def serialize(user)
  cookie_verifier.generate([user.class.to_s, user.marshal_dump])
end

def deserialize(key)
  klass, data = cookie_verifier.verify(key)
  klass.constantize.new.tap do |u|
    u.marshal_load(data)
  end
rescue ::ActiveSupport::MessageVerifier::InvalidSignature
  nil
end

[atmos]

@fphilipe Is this still a problem? @mastahyeti do you have a strong opinion either way?

[fphilipe]

@atmos Yes, still a problem. 😕

[btoews]

I don't see a big problem with passing the class name along.

Reading back over this PR though, I'm wondering if we meant to use MessageEncrypter instead of MesageVerifier though. MessageVerifier doesn't prevent someone without the secret from decoding the value, it just prevents it from being tampered with.