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

Enforce signed cookie expiration #11168

Closed
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
@josh
Member

josh commented Jun 28, 2013

Extracting a chainable cookie jar for a technique we use at @github to prevent cookie replays after the cookie expires from the users browser.

cookies.signed.expires[:user_id] = { :value => 137, :expires => 1.week.from_now }
cookies.signed.expires[:user_id] # => 137

The same browser expiry value is baked into the cookie value, signed and verified on read. Obvious this requires the cookie be signed or encrypted or else you could easy forge whatever expiry time. So you'll need to chain it with the existing signed or encrypted jars.

I'm still really unsure about the name. "expire", "expires", "expiry", etc. The idea it is enforces the expire time. Any suggestions? Was hoping @dhh could wordsmith it.

/cc @dhh @josevalim @trevorturk @mastahyeti@spastorino

@ghost ghost assigned josh Jun 28, 2013

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jun 28, 2013

Contributor

Thanks @josh! Would we have any downside on doing this by default for signed/encrypted jars? I know it will expire all previously signed cookies, but we can likely provide an upgrade plan.

Contributor

josevalim commented Jun 28, 2013

Thanks @josh! Would we have any downside on doing this by default for signed/encrypted jars? I know it will expire all previously signed cookies, but we can likely provide an upgrade plan.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jun 28, 2013

Member

@josevalim I'd love that actually. But I was concerned it was going to be too backwards incompatible.

Member

josh commented Jun 28, 2013

@josevalim I'd love that actually. But I was concerned it was going to be too backwards incompatible.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 28, 2013

Member

This seems good to me.

Member

rafaelfranca commented Jun 28, 2013

This seems good to me.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jun 29, 2013

Member

I like this. Let's not require the the #expires chain, though, but figure another backwards compatible way to do it. Using "expires" as the key is good too since that's the same as we already had. 👍

Member

dhh commented Jun 29, 2013

I like this. Let's not require the the #expires chain, though, but figure another backwards compatible way to do it. Using "expires" as the key is good too since that's the same as we already had. 👍

@trevorturk

This comment has been minimized.

Show comment
Hide comment
@trevorturk

trevorturk Jul 1, 2013

Contributor

I agree this should be the new default for signed and encrypted jars. It seems like a no-brainer security upgrade and it would be nice to avoid introducing another jar if possible.

If we only trigger the verification when there's an expires key, wouldn't that be enough?

For signed and encrypted cookies, a user wouldn't be able to manually strip an expires key, so if we encounter a cookie without an expires key, we could just assume that was a "legacy" cookie and should be accepted.

If someone wanted to forcibly expire all of their legacy cookies, they could choose to change their secret key.

Contributor

trevorturk commented Jul 1, 2013

I agree this should be the new default for signed and encrypted jars. It seems like a no-brainer security upgrade and it would be nice to avoid introducing another jar if possible.

If we only trigger the verification when there's an expires key, wouldn't that be enough?

For signed and encrypted cookies, a user wouldn't be able to manually strip an expires key, so if we encounter a cookie without an expires key, we could just assume that was a "legacy" cookie and should be accepted.

If someone wanted to forcibly expire all of their legacy cookies, they could choose to change their secret key.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jul 1, 2013

Member

For sure. I'm liking that approach.

But I still think we'll need a flag to disable upgrading the "legacy cookies".

Member

josh commented Jul 1, 2013

For sure. I'm liking that approach.

But I still think we'll need a flag to disable upgrading the "legacy cookies".

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jul 8, 2013

Member

Thought of a good way to do this in a backwards compat way.

I want to add the expiration support down at the message verifier level. This will allow us to read back both permanent and perishable signed messages.

Member

josh commented Jul 8, 2013

Thought of a good way to do this in a backwards compat way.

I want to add the expiration support down at the message verifier level. This will allow us to read back both permanent and perishable signed messages.

Show outdated Hide outdated activesupport/lib/active_support/message_verifier.rb
def generate(value, options = {})
if options[:expires]
data = ::Base64.strict_encode64(@serializer.dump([value, options[:expires].to_i]))
"#{data}-@#{generate_digest(data)}"

This comment has been minimized.

@josh

josh Jul 8, 2013

Member

Inventing a new separator format here. Makes it possible to tell the different between permanent and perishable messages.

Is -@ aesthetically pleasing?

@josh

josh Jul 8, 2013

Member

Inventing a new separator format here. Makes it possible to tell the different between permanent and perishable messages.

Is -@ aesthetically pleasing?

This comment has been minimized.

@dhh

dhh Jul 8, 2013

Member

Looks fine to me!

On Jul 8, 2013, at 10:12 PM, Joshua Peek notifications@github.com wrote:

In activesupport/lib/active_support/message_verifier.rb:

   else
     raise InvalidSignature
   end
 end
  • def generate(value)
  •  data = ::Base64.strict_encode64(@serializer.dump(value))
    
  •  "#{data}--#{generate_digest(data)}"
    
  • def generate(value, options = {})
  •  if options[:expires]
    
  •    data = ::Base64.strict_encode64(@serializer.dump([value, options[:expires].to_i]))
    
  •    "#{data}-@#{generate_digest(data)}"
    

Inventing a new separator format here. Makes it possible to tell the different between permanent and perishable messages.

Is -@ aesthetically pleasing?


Reply to this email directly or view it on GitHub.

@dhh

dhh Jul 8, 2013

Member

Looks fine to me!

On Jul 8, 2013, at 10:12 PM, Joshua Peek notifications@github.com wrote:

In activesupport/lib/active_support/message_verifier.rb:

   else
     raise InvalidSignature
   end
 end
  • def generate(value)
  •  data = ::Base64.strict_encode64(@serializer.dump(value))
    
  •  "#{data}--#{generate_digest(data)}"
    
  • def generate(value, options = {})
  •  if options[:expires]
    
  •    data = ::Base64.strict_encode64(@serializer.dump([value, options[:expires].to_i]))
    
  •    "#{data}-@#{generate_digest(data)}"
    

Inventing a new separator format here. Makes it possible to tell the different between permanent and perishable messages.

Is -@ aesthetically pleasing?


Reply to this email directly or view it on GitHub.

Show outdated Hide outdated activesupport/lib/active_support/message_verifier.rb
def generate(value)
data = ::Base64.strict_encode64(@serializer.dump(value))
"#{data}--#{generate_digest(data)}"
def generate(value, options = {})

This comment has been minimized.

@josh

josh Jul 8, 2013

Member

Should we be using more keyword args going forward?

@josh

josh Jul 8, 2013

Member

Should we be using more keyword args going forward?

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jul 8, 2013

Member

Think I still need to add support for this to the MessageEncrypter class.

Member

josh commented Jul 8, 2013

Think I still need to add support for this to the MessageEncrypter class.

Show outdated Hide outdated activesupport/lib/active_support/message_verifier.rb
@serializer.load(::Base64.decode64(data))
value = @serializer.load(::Base64.decode64(data))
if signed_message =~ /-@/

This comment has been minimized.

@mastahyeti

mastahyeti Jul 8, 2013

Contributor

It seems like this could be abused. If someone were to take a normal signed message and change the -- to -@ it would be treated differently and would have totally different results. Whatever is telling us to treat it as an expiring signed message also needs to be signed.

@mastahyeti

mastahyeti Jul 8, 2013

Contributor

It seems like this could be abused. If someone were to take a normal signed message and change the -- to -@ it would be treated differently and would have totally different results. Whatever is telling us to treat it as an expiring signed message also needs to be signed.

This comment has been minimized.

@josh

josh Jul 8, 2013

Member

Hmm, I'm a little unsure how to do it. Cause we can't at all touch the format of the permanent message.

@josh

josh Jul 8, 2013

Member

Hmm, I'm a little unsure how to do it. Cause we can't at all touch the format of the permanent message.

This comment has been minimized.

@josh

josh Jul 8, 2013

Member

Hopefully we can make it so existing permanent messages could never be interpreted as perishable ones.

@verifier.generate([123, 0]).sub('--', '-@')
# shouldn't equal
@verifier.generate(123, expires: Time.at(0))
@josh

josh Jul 8, 2013

Member

Hopefully we can make it so existing permanent messages could never be interpreted as perishable ones.

@verifier.generate([123, 0]).sub('--', '-@')
# shouldn't equal
@verifier.generate(123, expires: Time.at(0))

This comment has been minimized.

@mastahyeti

mastahyeti Jul 8, 2013

Contributor

It is a bit janky, but we could have the expires integer be a permanent signed message nested within the impermanent signed message. Alternatively, we could just add a third value that is a meaningless token that hopefully would never exist in an actual signed message. Both of those options feel shitty though.

@mastahyeti

mastahyeti Jul 8, 2013

Contributor

It is a bit janky, but we could have the expires integer be a permanent signed message nested within the impermanent signed message. Alternatively, we could just add a third value that is a meaningless token that hopefully would never exist in an actual signed message. Both of those options feel shitty though.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jul 9, 2013

Member

cc'ing some security people that should have a look. @NZKoz @tenderlove @ptoomey3 @postmodern

Member

josh commented Jul 9, 2013

cc'ing some security people that should have a look. @NZKoz @tenderlove @ptoomey3 @postmodern

Show outdated Hide outdated activesupport/lib/active_support/message_verifier.rb
if Time.at(Integer(expires)) >= Time.now
value
else
raise Expired

This comment has been minimized.

@NZKoz

NZKoz Jul 9, 2013

Member

Would be nice to include the time that it expired in this message, obviously we don't include the failed digest but in this case I can't see a reason not to include it

@NZKoz

NZKoz Jul 9, 2013

Member

Would be nice to include the time that it expired in this message, obviously we don't include the failed digest but in this case I can't see a reason not to include it

This comment has been minimized.

@josh

josh Jul 9, 2013

Member

Definitely.

On Jul 9, 2013, at 6:26 PM, Michael Koziarski notifications@github.com wrote:

In activesupport/lib/active_support/message_verifier.rb:

  •  parts = signed_message.split("--")
    
  •  if parts.length == 3
    
  •    data, expires, digest = parts
    
  •  else
    
  •    data, digest = parts
    
  •    expires = nil
    
  •  end
    
  •  if data.present? && digest.present? && secure_compare(digest, generate_digest(data, expires))
    
  •    value = @serializer.load(::Base64.decode64(data))
    
  •    if expires
    
  •      if Time.at(Integer(expires)) >= Time.now
    
  •        value
    
  •      else
    
  •        raise Expired
    
    Would be nice to include the time that it expired in this message, obviously we don't include the failed digest but in this case I can't see a reason not to include it


Reply to this email directly or view it on GitHub.

@josh

josh Jul 9, 2013

Member

Definitely.

On Jul 9, 2013, at 6:26 PM, Michael Koziarski notifications@github.com wrote:

In activesupport/lib/active_support/message_verifier.rb:

  •  parts = signed_message.split("--")
    
  •  if parts.length == 3
    
  •    data, expires, digest = parts
    
  •  else
    
  •    data, digest = parts
    
  •    expires = nil
    
  •  end
    
  •  if data.present? && digest.present? && secure_compare(digest, generate_digest(data, expires))
    
  •    value = @serializer.load(::Base64.decode64(data))
    
  •    if expires
    
  •      if Time.at(Integer(expires)) >= Time.now
    
  •        value
    
  •      else
    
  •        raise Expired
    
    Would be nice to include the time that it expired in this message, obviously we don't include the failed digest but in this case I can't see a reason not to include it


Reply to this email directly or view it on GitHub.

Show outdated Hide outdated activesupport/lib/active_support/message_verifier.rb
if data.present? && digest.present? && secure_compare(digest, generate_digest(data))
@serializer.load(::Base64.decode64(data))
parts = signed_message.split("--")
if parts.length == 3

This comment has been minimized.

@postmodern

postmodern Jul 9, 2013

Contributor

Might want to explicitly accept parts of length 2 or 3, otherwise error out.

@postmodern

postmodern Jul 9, 2013

Contributor

Might want to explicitly accept parts of length 2 or 3, otherwise error out.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 9, 2013

Member

This isn't actually backwards compatible either as digests generated by this code can't be read by the old code, I don't think this is a bad thing necessarily, but it's not backwards compatible:

irb(main):001:0> mv = ActiveSupport::MessageVerifier.new("lolsosecret")
=> #<ActiveSupport::MessageVerifier:0x007f82aa71e740 @secret="lolsosecret", @digest="SHA1", @serializer=Marshal>
irb(main):002:0> old = mv.generate("I am old")
=> "BAhJIg1JIGFtIG9sZAY6BkVU--caff953d49a2d68443051b40ad6a36bd6b850970"
irb(main):003:0> new = "BAhJIg1JIGFtIG9sZAY6BkVU--#{Time.now.to_i}--caff953d49a2d68443051b40ad6a36bd6b850970"
=> "BAhJIg1JIGFtIG9sZAY6BkVU--1373412772--caff953d49a2d68443051b40ad6a36bd6b850970"
irb(main):004:0> mv.verify(new)
ActiveSupport::MessageVerifier::InvalidSignature: ActiveSupport::MessageVerifier::InvalidSignature

I don't think there's actually a way to make this reliably backwards compatible without introducing issues anyway.

As for the encryption case, the encryptor signs the data too so there's no need to add expiry to the encryption code, just make sure the args are passed down into the relevant verifier.

Finally, an option to allow disabling this would be good so that people who do have to share cookies between apps which don't get upgraded simultaneously won't suddenly have things stop working.

Member

NZKoz commented Jul 9, 2013

This isn't actually backwards compatible either as digests generated by this code can't be read by the old code, I don't think this is a bad thing necessarily, but it's not backwards compatible:

irb(main):001:0> mv = ActiveSupport::MessageVerifier.new("lolsosecret")
=> #<ActiveSupport::MessageVerifier:0x007f82aa71e740 @secret="lolsosecret", @digest="SHA1", @serializer=Marshal>
irb(main):002:0> old = mv.generate("I am old")
=> "BAhJIg1JIGFtIG9sZAY6BkVU--caff953d49a2d68443051b40ad6a36bd6b850970"
irb(main):003:0> new = "BAhJIg1JIGFtIG9sZAY6BkVU--#{Time.now.to_i}--caff953d49a2d68443051b40ad6a36bd6b850970"
=> "BAhJIg1JIGFtIG9sZAY6BkVU--1373412772--caff953d49a2d68443051b40ad6a36bd6b850970"
irb(main):004:0> mv.verify(new)
ActiveSupport::MessageVerifier::InvalidSignature: ActiveSupport::MessageVerifier::InvalidSignature

I don't think there's actually a way to make this reliably backwards compatible without introducing issues anyway.

As for the encryption case, the encryptor signs the data too so there's no need to add expiry to the encryption code, just make sure the args are passed down into the relevant verifier.

Finally, an option to allow disabling this would be good so that people who do have to share cookies between apps which don't get upgraded simultaneously won't suddenly have things stop working.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 9, 2013

Member

Worth noting that when you're serializing the cookie you could just jam the expires time in there and therefore need no changes to the MessageVerifier.

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/cookies.rb#L385-L389

instead of serializing the raw value serialize {_expires: int, value: value} then detect that on read

Member

NZKoz commented Jul 9, 2013

Worth noting that when you're serializing the cookie you could just jam the expires time in there and therefore need no changes to the MessageVerifier.

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/cookies.rb#L385-L389

instead of serializing the raw value serialize {_expires: int, value: value} then detect that on read

Show outdated Hide outdated activesupport/lib/active_support/message_verifier.rb
require 'openssl' unless defined?(OpenSSL)
data += expires.to_s if expires

This comment has been minimized.

@ptoomey3

ptoomey3 Jul 10, 2013

Contributor

Doesn't this lead to the following property:

<base64_data>--<expires_int>--<hmac>

<base64_data><expires_int>--<hmac>

Wouldn't both of these values result in the same value for generate_digest?

Then, the comparison done above on line 53 would fall through (all of the validations would pass), and would then depend on the base64 decode and/or the serializer to fail. If these didn't fail, then this value would be accepted and time validation would NOT be performed (since the submitted value only contained a single "--" delimiter). Thus, if the moons aligned (fairly unlikely I acknowledge), a perishable cookie could be submitted as a permanent cookie. While unlikely to be exploited, it seems incorrect for the integrity/identification of a perishable cookie to depend on failure during base64 decode and/or deserialization.

@ptoomey3

ptoomey3 Jul 10, 2013

Contributor

Doesn't this lead to the following property:

<base64_data>--<expires_int>--<hmac>

<base64_data><expires_int>--<hmac>

Wouldn't both of these values result in the same value for generate_digest?

Then, the comparison done above on line 53 would fall through (all of the validations would pass), and would then depend on the base64 decode and/or the serializer to fail. If these didn't fail, then this value would be accepted and time validation would NOT be performed (since the submitted value only contained a single "--" delimiter). Thus, if the moons aligned (fairly unlikely I acknowledge), a perishable cookie could be submitted as a permanent cookie. While unlikely to be exploited, it seems incorrect for the integrity/identification of a perishable cookie to depend on failure during base64 decode and/or deserialization.

This comment has been minimized.

@NZKoz

NZKoz Jul 10, 2013

Member

Kind of, the deserialization would fail at line 54, but I agree that we shouldn't allow that kind of tampering to pass the hmac check in any case.

Perhaps the best approach is to push this all into the cookie jar thereby not adding risks that we subtly mess up a cryptographic API and end up having to do a painful security release.

@NZKoz

NZKoz Jul 10, 2013

Member

Kind of, the deserialization would fail at line 54, but I agree that we shouldn't allow that kind of tampering to pass the hmac check in any case.

Perhaps the best approach is to push this all into the cookie jar thereby not adding risks that we subtly mess up a cryptographic API and end up having to do a painful security release.

Show outdated Hide outdated activesupport/lib/active_support/message_verifier.rb
end
if data.present? && digest.present? && secure_compare(digest, generate_digest(data, expires))
value = @serializer.load(::Base64.decode64(data))

This comment has been minimized.

@ptoomey3

ptoomey3 Jul 10, 2013

Contributor

Assuming you don't depend on deserialization and/or base64 decoding for verifying integrity (see my other comment below), then maybe you could defer deserialization until after you have verified expiry. You don't need the deserialized object to verify integrity of the message, so you might as well check expiry first and "fail fast" if the received cookie has expired.

@ptoomey3

ptoomey3 Jul 10, 2013

Contributor

Assuming you don't depend on deserialization and/or base64 decoding for verifying integrity (see my other comment below), then maybe you could defer deserialization until after you have verified expiry. You don't need the deserialized object to verify integrity of the message, so you might as well check expiry first and "fail fast" if the received cookie has expired.

@ptoomey3

This comment has been minimized.

Show comment
Hide comment
@ptoomey3

ptoomey3 Jul 10, 2013

Contributor

Regarding embedding the expiry into the message.. This sounds appealing, as you don't have to make any modifications to MessageVerifier. However, what about the following downsides:

  • If you embed the expiry into the serialized data then you are required to deserialize to check expiry. If you keep it separate you can check expiry first and not bother with needless deserialization.
  • It would seem there would be a small, though probably negligible, chance that this format could be misinterpreted upon deserialization. What if a permanent signed cookie has a deserialized object that matches {_expires: int, value: value}. How would you distinguish a legacy permanent cookie from from a perishable one?
Contributor

ptoomey3 commented Jul 10, 2013

Regarding embedding the expiry into the message.. This sounds appealing, as you don't have to make any modifications to MessageVerifier. However, what about the following downsides:

  • If you embed the expiry into the serialized data then you are required to deserialize to check expiry. If you keep it separate you can check expiry first and not bother with needless deserialization.
  • It would seem there would be a small, though probably negligible, chance that this format could be misinterpreted upon deserialization. What if a permanent signed cookie has a deserialized object that matches {_expires: int, value: value}. How would you distinguish a legacy permanent cookie from from a perishable one?
@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 10, 2013

Member

@ptoomey3 yes, you'd have to deserialize to check expiry, this is a small performance cost but not one of any consequence really. In the normal case it has to be deserialized anyway, and in the 'expired' case you're still forced to do the HMAC checks, I don't see a huge issue here.

As for your second point, you can't actually serialize hashes in cookies at present because of this code

      def []=(name, options)
        if options.is_a?(Hash)
          options.symbolize_keys!
          value = options[:value]
        else
          value = options
          options = { :value => value }
        end

        handle_options(options)

        if @cookies[name.to_s] != value or options[:expires]
          @cookies[name.to_s] = value
          @set_cookies[name.to_s] = options
          @delete_cookies.delete(name.to_s)
        end

        value
      end

So it's something we should write in the release notes, but we're not breaking anyone's apps by doing this unless they're doing some EGREGIOUS hax to avoid that check

Member

NZKoz commented Jul 10, 2013

@ptoomey3 yes, you'd have to deserialize to check expiry, this is a small performance cost but not one of any consequence really. In the normal case it has to be deserialized anyway, and in the 'expired' case you're still forced to do the HMAC checks, I don't see a huge issue here.

As for your second point, you can't actually serialize hashes in cookies at present because of this code

      def []=(name, options)
        if options.is_a?(Hash)
          options.symbolize_keys!
          value = options[:value]
        else
          value = options
          options = { :value => value }
        end

        handle_options(options)

        if @cookies[name.to_s] != value or options[:expires]
          @cookies[name.to_s] = value
          @set_cookies[name.to_s] = options
          @delete_cookies.delete(name.to_s)
        end

        value
      end

So it's something we should write in the release notes, but we're not breaking anyone's apps by doing this unless they're doing some EGREGIOUS hax to avoid that check

@ptoomey3

This comment has been minimized.

Show comment
Hide comment
@ptoomey3

ptoomey3 Jul 10, 2013

Contributor

@NZKoz Ah, right, I had forgotten about the inability to serialize hashes in cookies (just curious...what if the value of ":value" in the options hash is itself a hash?). And, yes, the timing benefit is nominal compared to the hmac validation. So, if there is negligible issues with this formatting causing backward compatibility problems, then this approach seems nice.

From a security standpoint, I like that the hmac would be over the entire composite object (data and expiry) in a format that doesn't require splitting across delimiters to reconstruct for verification.

Contributor

ptoomey3 commented Jul 10, 2013

@NZKoz Ah, right, I had forgotten about the inability to serialize hashes in cookies (just curious...what if the value of ":value" in the options hash is itself a hash?). And, yes, the timing benefit is nominal compared to the hmac validation. So, if there is negligible issues with this formatting causing backward compatibility problems, then this approach seems nice.

From a security standpoint, I like that the hmac would be over the entire composite object (data and expiry) in a format that doesn't require splitting across delimiters to reconstruct for verification.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jul 10, 2013

Member

@NZKoz wdyt about a PerishableMessageVerifier subclass? Definitely worried about whatever crazy things people are already using MessageVerifier for.

I don't think there's actually a way to make this reliably backwards compatible without introducing issues anyway.

Sorry, I meant as an upgrade path, any existing non-timestamped cookies will be still valid.

As for your second point, you can't actually serialize hashes in cookies at present because of this code

I was actually avoiding {:_expires} because I thought that might be possible. That exception might be worth exploiting.

Member

josh commented Jul 10, 2013

@NZKoz wdyt about a PerishableMessageVerifier subclass? Definitely worried about whatever crazy things people are already using MessageVerifier for.

I don't think there's actually a way to make this reliably backwards compatible without introducing issues anyway.

Sorry, I meant as an upgrade path, any existing non-timestamped cookies will be still valid.

As for your second point, you can't actually serialize hashes in cookies at present because of this code

I was actually avoiding {:_expires} because I thought that might be possible. That exception might be worth exploiting.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 10, 2013

Member

@josh a seperate subclass would be fine which would let you use custom serialization to encode the timestamp to avoid the issue @ptoomey3 mentioned.

But I still think the right approach is to exploit the fact you can store hashes, and just do it that way (for signed and encrypted cookies)

Member

NZKoz commented Jul 10, 2013

@josh a seperate subclass would be fine which would let you use custom serialization to encode the timestamp to avoid the issue @ptoomey3 mentioned.

But I still think the right approach is to exploit the fact you can store hashes, and just do it that way (for signed and encrypted cookies)

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Jul 10, 2013

Contributor

@ptoomey3 is right. For signed and encrypted cookies you can do cookies.signed[:a] = {:value => {:b => 'c'}}

Contributor

mastahyeti commented Jul 10, 2013

@ptoomey3 is right. For signed and encrypted cookies you can do cookies.signed[:a] = {:value => {:b => 'c'}}

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 10, 2013

Member

@mastahyeti which would continue to work no?

Member

NZKoz commented Jul 10, 2013

@mastahyeti which would continue to work no?

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Jul 10, 2013

Contributor

The concern is just that someone else might already be using the namespace _expires or there might be some crazy app in which an attacker has control over the hash keys.

Contributor

mastahyeti commented Jul 10, 2013

The concern is just that someone else might already be using the namespace _expires or there might be some crazy app in which an attacker has control over the hash keys.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 10, 2013

Member

There'd be a small risk of people who just happened to store cookies[:a] = {:value=>{:_expires=>1, :value=>"asdf"}} but honestly, that's not worth the problems with the MessageVerifier approach where we have to attempt to provide backwards compatibility which is risky stuff.

What's the attack where an attacker has control of your hash keys? They could cause the application to generate an expired cookie that expired at a known time in cases where you currently generate non-expiring cookies? They couldn't force your application to generate unsigned cookie or force your app to generate cookies which expire later than you wanted them to?

Member

NZKoz commented Jul 10, 2013

There'd be a small risk of people who just happened to store cookies[:a] = {:value=>{:_expires=>1, :value=>"asdf"}} but honestly, that's not worth the problems with the MessageVerifier approach where we have to attempt to provide backwards compatibility which is risky stuff.

What's the attack where an attacker has control of your hash keys? They could cause the application to generate an expired cookie that expired at a known time in cases where you currently generate non-expiring cookies? They couldn't force your application to generate unsigned cookie or force your app to generate cookies which expire later than you wanted them to?

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Jul 10, 2013

Contributor

There isn't much of an attack scenario, I agree. If the application had two cookies, one of which was a normal signed cookie and entirely controlled by the attacker, and the other of which was a expiring signed cookie that wasn't controlled by the attacker. He could modify the cookie that he controls to look like an expiring signed cookie. He could then take that cookie and replace the actual expiring signed cookie with the spoofed one in the browser.

There are a lot of ifs and buts in that scenario, and it is really more of an issue with the fact that signed cookie values can be swapped between cookies. That issue should probably be addressed separately because it does pose a moderate risk for any app that has multiple, similarly formatted, signed cookies.

Contributor

mastahyeti commented Jul 10, 2013

There isn't much of an attack scenario, I agree. If the application had two cookies, one of which was a normal signed cookie and entirely controlled by the attacker, and the other of which was a expiring signed cookie that wasn't controlled by the attacker. He could modify the cookie that he controls to look like an expiring signed cookie. He could then take that cookie and replace the actual expiring signed cookie with the spoofed one in the browser.

There are a lot of ifs and buts in that scenario, and it is really more of an issue with the fact that signed cookie values can be swapped between cookies. That issue should probably be addressed separately because it does pose a moderate risk for any app that has multiple, similarly formatted, signed cookies.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 10, 2013

Member

The only way to conclusively avoid those issues is to actually require a seperate jar like @josh initially mentioned, or to have it as a configuration option where we document "hey, you turn this on here are the risks"

Member

NZKoz commented Jul 10, 2013

The only way to conclusively avoid those issues is to actually require a seperate jar like @josh initially mentioned, or to have it as a configuration option where we document "hey, you turn this on here are the risks"

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Jul 10, 2013

Contributor

Yep. It might not be worth worrying about it here.

Contributor

mastahyeti commented Jul 10, 2013

Yep. It might not be worth worrying about it here.

@ptoomey3

This comment has been minimized.

Show comment
Hide comment
@ptoomey3

ptoomey3 Jul 10, 2013

Contributor

Yeah, I'll think it over some more, but my concern was more about breaking someone else's code rather than a specific security issue. But, I agree, the probability of this causing a breakage are fairly low, and possibly not worth worrying too much about.

Contributor

ptoomey3 commented Jul 10, 2013

Yeah, I'll think it over some more, but my concern was more about breaking someone else's code rather than a specific security issue. But, I agree, the probability of this causing a breakage are fairly low, and possibly not worth worrying too much about.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 10, 2013

Member

I think in this case we should take a risk of a breakage over a risk of an attacker being able to circumvent this stuff. Especially if we have an option in config. for people who find some obscure breakage.

Member

NZKoz commented Jul 10, 2013

I think in this case we should take a risk of a breakage over a risk of an attacker being able to circumvent this stuff. Especially if we have an option in config. for people who find some obscure breakage.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jul 10, 2013

Member

If we're going to have to do the "legacy cookie jar" upgrade path thing again, I'd like to try to slip signing the cookie name into the value as well.

Its kinda scary that you could swap the cookie value from some other value into an auth token.

cookies.signed[:user_id] = 42
cookies.signed[:referrer_id] = params[:referrer_id] # 42

cookies[:user_id] == cookies[:referrer_id]
Member

josh commented Jul 10, 2013

If we're going to have to do the "legacy cookie jar" upgrade path thing again, I'd like to try to slip signing the cookie name into the value as well.

Its kinda scary that you could swap the cookie value from some other value into an auth token.

cookies.signed[:user_id] = 42
cookies.signed[:referrer_id] = params[:referrer_id] # 42

cookies[:user_id] == cookies[:referrer_id]
@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jul 10, 2013

Member

@NZKoz what do you think about this approach? Not yet done yet. Still need to figure out the upgrade shit.

Member

josh commented Jul 10, 2013

@NZKoz what do you think about this approach? Not yet done yet. Still need to figure out the upgrade shit.

@@ -384,18 +384,33 @@ def initialize(parent_jar, key_generator, options = {})
def [](name)
if signed_message = @parent_jar[name]
verify(signed_message)
if message = verify(signed_message)
if message[:name] != name

This comment has been minimized.

@mastahyeti

mastahyeti Jul 11, 2013

Contributor

🤘

@mastahyeti

mastahyeti Jul 11, 2013

Contributor

🤘

@ptoomey3

This comment has been minimized.

Show comment
Hide comment
@ptoomey3

ptoomey3 Jul 11, 2013

Contributor

@josh Does your above comment imply that, currently, signed session cookies uses the same key as other signed cookies (it looked like this was the case from a quick 30 second look at the code from my phone, but I will take a closer look when I am on my laptop)? In other words, is is possible that these two could be equal?

cookies.signed[:referer_id] = { value: arbitrary_user_controlled_value } # contains session_id, user_id, etc.
session[:user_id] = 42

It seems pretty unlikely you would have a cookies.signed[:referrer_id] cookie that is completely user controlled (why would you be signing a value that is completely user controlled anyway?). But, I was just curious if that was what you were implying? Like @mastahyeti 🤘 on including the name in the signature to prevent this kind of thing.

Contributor

ptoomey3 commented Jul 11, 2013

@josh Does your above comment imply that, currently, signed session cookies uses the same key as other signed cookies (it looked like this was the case from a quick 30 second look at the code from my phone, but I will take a closer look when I am on my laptop)? In other words, is is possible that these two could be equal?

cookies.signed[:referer_id] = { value: arbitrary_user_controlled_value } # contains session_id, user_id, etc.
session[:user_id] = 42

It seems pretty unlikely you would have a cookies.signed[:referrer_id] cookie that is completely user controlled (why would you be signing a value that is completely user controlled anyway?). But, I was just curious if that was what you were implying? Like @mastahyeti 🤘 on including the name in the signature to prevent this kind of thing.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 14, 2013

Member

including the name in the cookie signature is a good idea, but a complete pain in terms of backwards compatibility. If you're backwards compatible, then you're allowing attackers to skip the name verification which defeats the entire purpose of adding the name verification. We could add it as a config option though and warn that it'll be on in the future and you possibly want it now.

@ptoomey3 yeah, if you let users specify arbitrary hashes that you then sign with the cookie secret,then they can forge sessions.

Member

NZKoz commented Jul 14, 2013

including the name in the cookie signature is a good idea, but a complete pain in terms of backwards compatibility. If you're backwards compatible, then you're allowing attackers to skip the name verification which defeats the entire purpose of adding the name verification. We could add it as a config option though and warn that it'll be on in the future and you possibly want it now.

@ptoomey3 yeah, if you let users specify arbitrary hashes that you then sign with the cookie secret,then they can forge sessions.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jul 14, 2013

Member

@NZKoz I think we'll need the upgrade flag now just for expires if we are doing it at the cookie jar level. So it seems like a good time to introduce both, you think?

In terms of app upgrade flow, you'd have a flag that basically excepts both forms of cookies, the current signature and these new signed name and expires values. You'd maybe keep that flipped for a month in production while peoples cookies are getting upgraded then flip it over to the "more secure" default when you're ready. Green field apps can do this out of the box.

Member

josh commented Jul 14, 2013

@NZKoz I think we'll need the upgrade flag now just for expires if we are doing it at the cookie jar level. So it seems like a good time to introduce both, you think?

In terms of app upgrade flow, you'd have a flag that basically excepts both forms of cookies, the current signature and these new signed name and expires values. You'd maybe keep that flipped for a month in production while peoples cookies are getting upgraded then flip it over to the "more secure" default when you're ready. Green field apps can do this out of the box.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jul 14, 2013

Member

Sounds good, my only strong feelings would be:

  1. we don't default to accepting both formats, make someone turn on the upgrade code explicitly
  2. we should default new apps to the new format, old apps as is.
Member

NZKoz commented Jul 14, 2013

Sounds good, my only strong feelings would be:

  1. we don't default to accepting both formats, make someone turn on the upgrade code explicitly
  2. we should default new apps to the new format, old apps as is.

@mastahyeti mastahyeti referenced this pull request Jul 18, 2013

Closed

Add upgrade path #1

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Aug 8, 2013

Member

Giving up. The inheritance and mixins approach in the chained and legacy cookie jar code is absurd beyond my patience.

Someone else is free to run with this code as a start and take the credit.

Member

josh commented Aug 8, 2013

Giving up. The inheritance and mixins approach in the chained and legacy cookie jar code is absurd beyond my patience.

Someone else is free to run with this code as a start and take the credit.

@josh josh closed this Aug 8, 2013

@josh josh deleted the josh:forced-expiry-cookie-jar branch Aug 8, 2013

@nikcub

This comment has been minimized.

Show comment
Hide comment
@nikcub

nikcub Oct 14, 2013

might want to reopen this:

http://maverickblogging.com/logout-is-broken-by-default-ruby-on-rails-web-applications/

this issue isn't unique to Rails, but getting this merged should be worked out

nikcub commented Oct 14, 2013

might want to reopen this:

http://maverickblogging.com/logout-is-broken-by-default-ruby-on-rails-web-applications/

this issue isn't unique to Rails, but getting this merged should be worked out

@thbar

This comment has been minimized.

Show comment
Hide comment
@thbar

thbar Oct 14, 2013

There is some code to add expiration here as well, at least until this issue is reconsidered.

https://www.coffeepowered.net/2013/09/26/rails-session-cookies/

thbar commented Oct 14, 2013

There is some code to add expiration here as well, at least until this issue is reconsidered.

https://www.coffeepowered.net/2013/09/26/rails-session-cookies/

@guilleiguaran guilleiguaran restored the josh:forced-expiry-cookie-jar branch Jan 23, 2014

@guilleiguaran guilleiguaran deleted the josh:forced-expiry-cookie-jar branch Jan 23, 2014

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