Mask CSRF tokens to mitigate BREACH attack #11729

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
@bradleybuda
Contributor

bradleybuda commented Aug 3, 2013

The BREACH attack described at Black Hat this year allows an attacker to recover plaintext from SSL sessions if they have some idea what they're looking for. One high-value thing to steal that has a predictable plaintext format is the CSRF token (because it always appears in a meta tag and frequently in form tags as well).

The researchers who discovered the attack suggest mitigating it by "masking" secret tokens so they are different on each request. This implements their suggested masking approach from section 3.4 of the paper (PDF). The authenticity token is delivered as a 64-byte string, instead of a 32-byte string. The first 32 bytes are a one-time pad, and the second 32 are an XOR between the pad and the "real" CSRF token. The point is not to hide the token from the client, but to make sure it is different on every request so it's impossible for an attacker to recover by measuring compressability.

The code should be backwards-compatible with existing Rails installs; the format of session[:_csrf_token] is unchanged, and unmasked tokens will still be accepted from clients (with a warning) so that you don't invalidate all your users' sessions on deploy. However, if users have overridden ActionController#verfied_request?, this may break them (depending on whether or not they're calling super).

This is not a blanket fix for BREACH, just a way of protecting against one particular variant of attack. I am not a security expert; I've just implemented the fix as suggested in the paper. This should be reviewed by someone who knows what they're doing.

@@ -0,0 +1,54 @@
+module ActionController
+ class AuthenticityToken
+ class << self

This comment has been minimized.

@egilburg

egilburg Aug 3, 2013

Contributor

I wouldn't make this a use class methods. It seems most of the methods here revolve around the session object, and thus it would make sense to make AuthenticityToken be initialized with a readable attribute session as follows:

token = AuthenticityToken.new(session)
token.generate_masked

# ...

class AuthenticityToken
  attr_reader :session

  def initialize(session)
    @session = session
  end

  def generate_masked
    # ...
  end

  # ...
end

On the same topic, based on the Single Responsibility Principle, perhaps you could make different classes for the token generator and the token itself:

token_builder = AuthenticityTokenGenerator.new(session)
token = token_builder.generate_masked

Of course, for consumers who only need the token object iself, the above can be simplified to:

token = AuthenticityTokenGenerator.new(session).generate_masked  # => an AuthenticityToken object

Or an even simpler, with a class builder method:

token = AuthenticityTokenGenerator.generate(session) # => an AuthenticityToken object

This way, the methods related to generating the token can be on the builder class, while the methods related to validating it can be on the token class itself.

The xor_byte_strings method can still be a class method because it's accessed both by builder and by token itself, and does not operate on a session.

@egilburg

egilburg Aug 3, 2013

Contributor

I wouldn't make this a use class methods. It seems most of the methods here revolve around the session object, and thus it would make sense to make AuthenticityToken be initialized with a readable attribute session as follows:

token = AuthenticityToken.new(session)
token.generate_masked

# ...

class AuthenticityToken
  attr_reader :session

  def initialize(session)
    @session = session
  end

  def generate_masked
    # ...
  end

  # ...
end

On the same topic, based on the Single Responsibility Principle, perhaps you could make different classes for the token generator and the token itself:

token_builder = AuthenticityTokenGenerator.new(session)
token = token_builder.generate_masked

Of course, for consumers who only need the token object iself, the above can be simplified to:

token = AuthenticityTokenGenerator.new(session).generate_masked  # => an AuthenticityToken object

Or an even simpler, with a class builder method:

token = AuthenticityTokenGenerator.generate(session) # => an AuthenticityToken object

This way, the methods related to generating the token can be on the builder class, while the methods related to validating it can be on the token class itself.

The xor_byte_strings method can still be a class method because it's accessed both by builder and by token itself, and does not operate on a session.

+ def generate_masked(session)
+ one_time_pad = SecureRandom.random_bytes(LENGTH)
+ encrypted_csrf_token = xor_byte_strings(one_time_pad, master_csrf_token(session))
+ masked_token = one_time_pad + encrypted_csrf_token

This comment has been minimized.

@egilburg

egilburg Aug 3, 2013

Contributor

Consider using .concat here instead of + to avoid extra object creation.

@egilburg

egilburg Aug 3, 2013

Contributor

Consider using .concat here instead of + to avoid extra object creation.

+ end
+
+ def valid?(session, encoded_masked_token, logger = nil)
+ return false if encoded_masked_token.nil?

This comment has been minimized.

@egilburg

egilburg Aug 3, 2013

Contributor

Since you don't expect false to be passed for encoded_masked_token with different semantics than nil, it's simpler to write:

  return false unless encoded_masked_token
@egilburg

egilburg Aug 3, 2013

Contributor

Since you don't expect false to be passed for encoded_masked_token with different semantics than nil, it's simpler to write:

  return false unless encoded_masked_token
+
+ masked_token == master_csrf_token(session)
+
+ elsif masked_token.length == LENGTH * 2

This comment has been minimized.

@egilburg

egilburg Aug 3, 2013

Contributor

You don't have an else clause here. While it will return nil which acts as falsy, given the boolean method name valid?, it's probably better to explicitly return else false here.

@egilburg

egilburg Aug 3, 2013

Contributor

You don't have an else clause here. While it will return nil which acts as falsy, given the boolean method name valid?, it's probably better to explicitly return else false here.

+ elsif masked_token.length == LENGTH * 2
+ # Split the token into the one-time pad and the encrypted
+ # value and decrypt it
+ one_time_pad = masked_token[0...LENGTH]

This comment has been minimized.

@egilburg

egilburg Aug 3, 2013

Contributor

You can use masked_token.first(LENGTH) here.

@egilburg

egilburg Aug 3, 2013

Contributor

You can use masked_token.first(LENGTH) here.

+ # Split the token into the one-time pad and the encrypted
+ # value and decrypt it
+ one_time_pad = masked_token[0...LENGTH]
+ encrypted_csrf_token = masked_token[LENGTH..-1]

This comment has been minimized.

@egilburg

egilburg Aug 3, 2013

Contributor

You can use masked_token.last(LENGTH) here.

@egilburg

egilburg Aug 3, 2013

Contributor

You can use masked_token.last(LENGTH) here.

+ private
+
+ def xor_byte_strings(s1, s2)
+ s1.bytes.zip(s2.bytes).map { |(c1,c2)| c1 ^ c2 }.pack('c*')

This comment has been minimized.

@egilburg

egilburg Aug 3, 2013

Contributor

Can use map! to avoid extra object creation. Also, don't need parentheses around c1, c2.

If this is a hotspot, can optimize a bit more by extraciting 'c*' into a constant to avoid object creation each time.

@egilburg

egilburg Aug 3, 2013

Contributor

Can use map! to avoid extra object creation. Also, don't need parentheses around c1, c2.

If this is a hotspot, can optimize a bit more by extraciting 'c*' into a constant to avoid object creation each time.

+ end
+
+ def master_csrf_token(session)
+ session[:_csrf_token] ||= SecureRandom.base64(LENGTH)

This comment has been minimized.

@egilburg

egilburg Aug 3, 2013

Contributor

It doesn't seem worthwhile to pass around the entire session object where most methods (other than this one) just care about the token value itself, not mutating the session. I think that the code mutating the session should live separately from the methods/class that generates/validates the token - as early as possible, the token should be extracted and operated on, and as late as possible the session should be updated with the new token value.

@egilburg

egilburg Aug 3, 2013

Contributor

It doesn't seem worthwhile to pass around the entire session object where most methods (other than this one) just care about the token value itself, not mutating the session. I think that the code mutating the session should live separately from the methods/class that generates/validates the token - as early as possible, the token should be extracted and operated on, and as late as possible the session should be updated with the new token value.

end
# Sets the token value for the current session.
def form_authenticity_token
- session[:_csrf_token] ||= SecureRandom.base64(32)
+ AuthenticityToken.generate_masked(session)

This comment has been minimized.

@egilburg

egilburg Aug 3, 2013

Contributor

I wonder if this will impact performance. This method, and all string manipulation logic inside it, will be executed on every request (as opposed to current implementation, which caches the value in the session). Perhaps add some benchmarks to this method?

@egilburg

egilburg Aug 3, 2013

Contributor

I wonder if this will impact performance. This method, and all string manipulation logic inside it, will be executed on every request (as opposed to current implementation, which caches the value in the session). Perhaps add some benchmarks to this method?

This comment has been minimized.

@Alamoz

Alamoz Aug 3, 2013

Why not make it a configurable option, so users can decide which is more important to them?

config.csrf_masking - or - config.mask_csrf

The first form, though longer in length, hints the process recurs for each request. Perhaps also add a comment about decreased performance if decrease is significant.

@Alamoz

Alamoz Aug 3, 2013

Why not make it a configurable option, so users can decide which is more important to them?

config.csrf_masking - or - config.mask_csrf

The first form, though longer in length, hints the process recurs for each request. Perhaps also add a comment about decreased performance if decrease is significant.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Aug 3, 2013

Contributor

it's SSL problem, isn't it. Can you imagine hassle of generating new token everytime?

Contributor

homakov commented Aug 3, 2013

it's SSL problem, isn't it. Can you imagine hassle of generating new token everytime?

@Alamoz

This comment has been minimized.

Show comment
Hide comment
@Alamoz

Alamoz Aug 3, 2013

Doesn't look too bad. He is implementing the suggested solution.

Alamoz commented Aug 3, 2013

Doesn't look too bad. He is implementing the suggested solution.

@waynerobinson

This comment has been minimized.

Show comment
Hide comment
@waynerobinson

waynerobinson Aug 3, 2013

Would this have the side-effect of also protecting encrypted cookie sessions from this exploit as the ciphertext of the session cookie (containing the _csrf_token) would end up being different on every request too?

Would this have the side-effect of also protecting encrypted cookie sessions from this exploit as the ciphertext of the session cookie (containing the _csrf_token) would end up being different on every request too?

@waynerobinson

This comment has been minimized.

Show comment
Hide comment
@waynerobinson

waynerobinson Aug 3, 2013

After thinking about it, wouldn't just adding random characters to the session every request (when using encrypted sessions) mitigate this attack entirely for all data in the session?

Actually, on further reflection, would I be right in assuming that the session isn't at risk at all because HTTP headers aren't compressed?

I guess this still effects the _csrf_token included as a meta tag in the response body.

After thinking about it, wouldn't just adding random characters to the session every request (when using encrypted sessions) mitigate this attack entirely for all data in the session?

Actually, on further reflection, would I be right in assuming that the session isn't at risk at all because HTTP headers aren't compressed?

I guess this still effects the _csrf_token included as a meta tag in the response body.

@Alamoz

This comment has been minimized.

Show comment
Hide comment
@Alamoz

Alamoz Aug 3, 2013

The paper states HTTP-level compression as one of the preconditions. That is something users may configure their web servers to do, so such users may want this proposed behavior in their rails apps.

Alamoz commented Aug 3, 2013

The paper states HTTP-level compression as one of the preconditions. That is something users may configure their web servers to do, so such users may want this proposed behavior in their rails apps.

@jcoglan

This comment has been minimized.

Show comment
Hide comment
@jcoglan

jcoglan Aug 3, 2013

Contributor

I would be worth having the masking/unmasking functions available as a library interface rather than baked into AuthenticityToken since users probably have other data they would like to protect using this technique.

Contributor

jcoglan commented Aug 3, 2013

I would be worth having the masking/unmasking functions available as a library interface rather than baked into AuthenticityToken since users probably have other data they would like to protect using this technique.

@jcoglan

This comment has been minimized.

Show comment
Hide comment
@jcoglan

jcoglan Aug 3, 2013

Contributor

@waynerobinson The ciphertext of cookie-stored sessions is different every time anyway, because it uses a random IV during encryption. If you encrypt the same plaintext twice, you should get a different result each time since sending the same ciphertext over the wire tells the attacker that you sent the same data twice, which leaks information.

Contributor

jcoglan commented Aug 3, 2013

@waynerobinson The ciphertext of cookie-stored sessions is different every time anyway, because it uses a random IV during encryption. If you encrypt the same plaintext twice, you should get a different result each time since sending the same ciphertext over the wire tells the attacker that you sent the same data twice, which leaks information.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Aug 3, 2013

Member

Calling in the @NZKoz bat signal.

Member

steveklabnik commented Aug 3, 2013

Calling in the @NZKoz bat signal.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Aug 3, 2013

Contributor

We need something universal, maybe we should transfer csrf token as an additional cookie readable on client side.

var token=$.cookie('csrf_token')

It will be a header, not included in response body

Contributor

homakov commented Aug 3, 2013

We need something universal, maybe we should transfer csrf token as an additional cookie readable on client side.

var token=$.cookie('csrf_token')

It will be a header, not included in response body

@bradleybuda

This comment has been minimized.

Show comment
Hide comment
@bradleybuda

bradleybuda Aug 3, 2013

Contributor

@waynerobinson Nothing in the HTTP headers is at risk because they're not in the same compression context as the body (you're right; they're not compressed at all).

@homakov Not sending the CSRF token each time would definitely mitigate the attack (or sending it in a header). The problem is that this will break non-XHR form posts, which include the CSRF token as the authenticity_token. A hybrid option would be to send the CSRF token unmasked in an HTTP header for XHRs to use, and to send a masked token in any <form> tags that require it.

Contributor

bradleybuda commented Aug 3, 2013

@waynerobinson Nothing in the HTTP headers is at risk because they're not in the same compression context as the body (you're right; they're not compressed at all).

@homakov Not sending the CSRF token each time would definitely mitigate the attack (or sending it in a header). The problem is that this will break non-XHR form posts, which include the CSRF token as the authenticity_token. A hybrid option would be to send the CSRF token unmasked in an HTTP header for XHRs to use, and to send a masked token in any <form> tags that require it.

@bradleybuda

This comment has been minimized.

Show comment
Hide comment
@bradleybuda

bradleybuda Aug 3, 2013

Contributor

@egilburg Thanks for the comments; I'll make these changes. I'll also do some benchmarking; we definitely need to figure out if this is expensive or not. If the masking is cheap, I'd just as soon do it on every request and not make it a configuration option (or at least make it opt-out) - users should not want to opt out of masking unless there's a performance hit, or they're doing something really exotic / custom with CSRF tokens.

Contributor

bradleybuda commented Aug 3, 2013

@egilburg Thanks for the comments; I'll make these changes. I'll also do some benchmarking; we definitely need to figure out if this is expensive or not. If the masking is cheap, I'd just as soon do it on every request and not make it a configuration option (or at least make it opt-out) - users should not want to opt out of masking unless there's a performance hit, or they're doing something really exotic / custom with CSRF tokens.

@bradleybuda

This comment has been minimized.

Show comment
Hide comment
@bradleybuda

bradleybuda Aug 3, 2013

Contributor

One other option to improve performance / reduce complexity is to use a different technique to obfuscate the authenticity token. There's nothing special about the OTP / XOR masking algorithm suggested in the paper; we just need a way to randomly obfuscate the data on each request in a way that the server can check. So something like salt + MD5(salt+secret) would work, or even bcrypt with the minimum cost. This way I'm not adding random amateur crypto code to Rails and we get an algorithm with predictable performance.

Contributor

bradleybuda commented Aug 3, 2013

One other option to improve performance / reduce complexity is to use a different technique to obfuscate the authenticity token. There's nothing special about the OTP / XOR masking algorithm suggested in the paper; we just need a way to randomly obfuscate the data on each request in a way that the server can check. So something like salt + MD5(salt+secret) would work, or even bcrypt with the minimum cost. This way I'm not adding random amateur crypto code to Rails and we get an algorithm with predictable performance.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Aug 3, 2013

Contributor

Please have a look, this should be a faster and better protection https://gist.github.com/homakov/6147227

Contributor

homakov commented Aug 3, 2013

Please have a look, this should be a faster and better protection https://gist.github.com/homakov/6147227

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Aug 3, 2013

Contributor

non-XHR form posts, which include the CSRF token as the authenticity_token

yes, if JS is off we need something different

Contributor

homakov commented Aug 3, 2013

non-XHR form posts, which include the CSRF token as the authenticity_token

yes, if JS is off we need something different

@jcoglan

This comment has been minimized.

Show comment
Hide comment
@jcoglan

jcoglan Aug 3, 2013

Contributor

You can't implement CSRF tokens as a cookie; the weaknesses of cookies is what CSRF tokens are supposed to prevent. If you send the token as a cookie, the browser will attach it to all requests to your server regardless of which origin they come from, defeating the point of CSRF protection.

Plus, any security solution that relies on JavaScript should be considered a weak solution.

Contributor

jcoglan commented Aug 3, 2013

You can't implement CSRF tokens as a cookie; the weaknesses of cookies is what CSRF tokens are supposed to prevent. If you send the token as a cookie, the browser will attach it to all requests to your server regardless of which origin they come from, defeating the point of CSRF protection.

Plus, any security solution that relies on JavaScript should be considered a weak solution.

@jcoglan

This comment has been minimized.

Show comment
Hide comment
@jcoglan

jcoglan Aug 3, 2013

Contributor

If you're concerned about the performance of XOR (which might be a reasonable concern but someone should benchmark it and find out), then write it in C.

Contributor

jcoglan commented Aug 3, 2013

If you're concerned about the performance of XOR (which might be a reasonable concern but someone should benchmark it and find out), then write it in C.

@louismullie louismullie referenced this pull request in sinatra/rack-protection Aug 3, 2013

Closed

Mask CSRF tokens to mitigate BREACH attack #64

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Aug 3, 2013

Contributor

@jcoglan i know how CSRF works at my fingertips :) You probably misunderstood what I proposed: instead of plain tag we put it into Set-Cookie and only after page load (in runtime) we add CSRF tokens and other important information.

Plus, any security solution that relies on JavaScript should be considered a weak solution.

Very foggy argument, what exactly is wrong about this one?

Besides XORing we need a way to hide ANY secret tokens (api_key in my demo). How are you going to hide it? un XORing with javascript? Set-Cookie is a simplest solution, with only one weakness - it requires JS on.

Contributor

homakov commented Aug 3, 2013

@jcoglan i know how CSRF works at my fingertips :) You probably misunderstood what I proposed: instead of plain tag we put it into Set-Cookie and only after page load (in runtime) we add CSRF tokens and other important information.

Plus, any security solution that relies on JavaScript should be considered a weak solution.

Very foggy argument, what exactly is wrong about this one?

Besides XORing we need a way to hide ANY secret tokens (api_key in my demo). How are you going to hide it? un XORing with javascript? Set-Cookie is a simplest solution, with only one weakness - it requires JS on.

@jcoglan

This comment has been minimized.

Show comment
Hide comment
@jcoglan

jcoglan Aug 3, 2013

Contributor

Any solution that assumes the user agent will run JavaScript as part of the security process cannot be general-purpose, since not all user-agents run JavaScript. Invoking the user agent's JS runtime also means there's another component we need to place our trust in.

XOR-masking seems like a totally reasonable approach. It's easily understood and easy to implement, and does not rely on client-side behaviour in order to protect the server. It's the same technique used by WebSocket on all data to prevent the client from constructing arbitrary byte sequences.

The argument that XOR is slow has not been tested, and we should not make arguments from performance when we have no numbers to talk about.

The JS solution may seem to require less code, but it's most complex, since it invokes more parts of the stack in order to work.

We could protect any token by providing two server-side functions, mask(string) and unmask(string). You do not need to unmask these values on the client side; your JS code often does care what their values actually are, it just passes them back to your server and you can unmask the values on the server side.

All I'm saying is we should not lean on JavaScript when a workable solution can be done entirely server-side.

Contributor

jcoglan commented Aug 3, 2013

Any solution that assumes the user agent will run JavaScript as part of the security process cannot be general-purpose, since not all user-agents run JavaScript. Invoking the user agent's JS runtime also means there's another component we need to place our trust in.

XOR-masking seems like a totally reasonable approach. It's easily understood and easy to implement, and does not rely on client-side behaviour in order to protect the server. It's the same technique used by WebSocket on all data to prevent the client from constructing arbitrary byte sequences.

The argument that XOR is slow has not been tested, and we should not make arguments from performance when we have no numbers to talk about.

The JS solution may seem to require less code, but it's most complex, since it invokes more parts of the stack in order to work.

We could protect any token by providing two server-side functions, mask(string) and unmask(string). You do not need to unmask these values on the client side; your JS code often does care what their values actually are, it just passes them back to your server and you can unmask the values on the server side.

All I'm saying is we should not lean on JavaScript when a workable solution can be done entirely server-side.

@bradleybuda

This comment has been minimized.

Show comment
Hide comment
@bradleybuda

bradleybuda Aug 3, 2013

Contributor

Here's a benchmark (let me know if you see a way to improve any of those snippets). Results:

                  user     system      total        real
xor           0.000000   0.000000   0.000000 (  0.000090)
salt + sha1   0.000000   0.000000   0.000000 (  0.000049)
salt + md5    0.000000   0.000000   0.000000 (  0.000038)
bcrypt        0.000000   0.000000   0.000000 (  0.001115)

In absolute terms, everything but Bcrypt is quite fast.

Contributor

bradleybuda commented Aug 3, 2013

Here's a benchmark (let me know if you see a way to improve any of those snippets). Results:

                  user     system      total        real
xor           0.000000   0.000000   0.000000 (  0.000090)
salt + sha1   0.000000   0.000000   0.000000 (  0.000049)
salt + md5    0.000000   0.000000   0.000000 (  0.000038)
bcrypt        0.000000   0.000000   0.000000 (  0.001115)

In absolute terms, everything but Bcrypt is quite fast.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Aug 3, 2013

Contributor

All I'm saying is we should not lean on JavaScript when a workable solution can be done entirely server-side.

OK, I agree that XOR suits better for production because my idea requires JS.

You do not need to unmask these values on the client side; your JS code often does care what their values actually are

CSRF token is only an example, to show a developer his api_key or credit card number you will need unmask in JS. (and you will have to lean on JavaScript).

Both solutions are not perfect but XOR covers more cases

Contributor

homakov commented Aug 3, 2013

All I'm saying is we should not lean on JavaScript when a workable solution can be done entirely server-side.

OK, I agree that XOR suits better for production because my idea requires JS.

You do not need to unmask these values on the client side; your JS code often does care what their values actually are

CSRF token is only an example, to show a developer his api_key or credit card number you will need unmask in JS. (and you will have to lean on JavaScript).

Both solutions are not perfect but XOR covers more cases

@ghost ghost assigned NZKoz Aug 3, 2013

@Alamoz

This comment has been minimized.

Show comment
Hide comment
@Alamoz

Alamoz Aug 4, 2013

@bradleybuda I've run one of my rails apps in 4.1.0-beta, both with and without your patch, and see no noticeable difference in performance when rendering a form (rendered 22 times for each scenario.) Your benchmark results substantiate this, the time differences are negligible. Here are gists of each test (the times are slow overall because they were run on a laptop, my production server takes 12-20ms to render that page):

Without patch: https://gist.github.com/Alamoz/6148578

With patch: https://gist.github.com/Alamoz/6148540

Alamoz commented Aug 4, 2013

@bradleybuda I've run one of my rails apps in 4.1.0-beta, both with and without your patch, and see no noticeable difference in performance when rendering a form (rendered 22 times for each scenario.) Your benchmark results substantiate this, the time differences are negligible. Here are gists of each test (the times are slow overall because they were run on a laptop, my production server takes 12-20ms to render that page):

Without patch: https://gist.github.com/Alamoz/6148578

With patch: https://gist.github.com/Alamoz/6148540

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Aug 4, 2013

Member

I'll be honest and say that most of this complexity strikes me as unnecessary. We're all already disabling TLS compression due to CRIME right? To me this is just another reason to do so?

But most importantly, the BREACH attack was only revealed recently and has barely been reviewed outside of the original researchers and their immediate colleagues. The proposed mitigations may themselves have other implications and may either introduce other issues or not actually fix the underlying problem. Let's let this stew for a while with security researchers doing their analysis on various approaches and wait and see what the security community as a whole recommends.

There's certainly no reason for us to rush to have a fix in before a week has even passed, the odds of us doing it right are basically zero.

One thing we should do though is make sure any SSL/TLS documentation we have strongly recommends disabling compression.

Member

NZKoz commented Aug 4, 2013

I'll be honest and say that most of this complexity strikes me as unnecessary. We're all already disabling TLS compression due to CRIME right? To me this is just another reason to do so?

But most importantly, the BREACH attack was only revealed recently and has barely been reviewed outside of the original researchers and their immediate colleagues. The proposed mitigations may themselves have other implications and may either introduce other issues or not actually fix the underlying problem. Let's let this stew for a while with security researchers doing their analysis on various approaches and wait and see what the security community as a whole recommends.

There's certainly no reason for us to rush to have a fix in before a week has even passed, the odds of us doing it right are basically zero.

One thing we should do though is make sure any SSL/TLS documentation we have strongly recommends disabling compression.

@waynerobinson

This comment has been minimized.

Show comment
Hide comment
@waynerobinson

waynerobinson Aug 4, 2013

@NZKoz disabling TLS compression mitigates against CRIME.

BREACH uses a similar method to attack compressed HTTP bodies within the TLS stream.

Not compressing HTTP bodies will have a significant performance impact on a majority of applications as both HTML & JSON have a very large amount of repetition.

The attack relies completely on the data you are trying to locate not changing from one request to the next. If any of this sensitive data varies from one request to the next, guessing the correct value becomes impossible because you never make any headway on your brute force attack.

If we every want to be able to compress anything in our TLS stream, information that a hacker may find useful must be obfuscated uniquely in every request. This doesn't have to be cryptographic, it just needs to add enough extra entropy to the value to make it impossible to know what it will be from request-to-request.

@NZKoz disabling TLS compression mitigates against CRIME.

BREACH uses a similar method to attack compressed HTTP bodies within the TLS stream.

Not compressing HTTP bodies will have a significant performance impact on a majority of applications as both HTML & JSON have a very large amount of repetition.

The attack relies completely on the data you are trying to locate not changing from one request to the next. If any of this sensitive data varies from one request to the next, guessing the correct value becomes impossible because you never make any headway on your brute force attack.

If we every want to be able to compress anything in our TLS stream, information that a hacker may find useful must be obfuscated uniquely in every request. This doesn't have to be cryptographic, it just needs to add enough extra entropy to the value to make it impossible to know what it will be from request-to-request.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Aug 4, 2013

Contributor
  1. it must be a separate helper, not just AuthenticityToken
  2. for https websites only, no reason to mess with http
  3. @NZKoz

has barely been reviewed outside of the original researchers and their immediate colleagues

the idea of the hack sounds pretty clear and feasible, I see no reason to wait for something. CRIME is a real thing, BREACH is just a version of CRIME.

Contributor

homakov commented Aug 4, 2013

  1. it must be a separate helper, not just AuthenticityToken
  2. for https websites only, no reason to mess with http
  3. @NZKoz

has barely been reviewed outside of the original researchers and their immediate colleagues

the idea of the hack sounds pretty clear and feasible, I see no reason to wait for something. CRIME is a real thing, BREACH is just a version of CRIME.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Aug 4, 2013

Member

Yeah, while I may sound like I'm pouring cold water on the work here, I'm really just suggesting that we wait a few months before doing anything as ideally there'll be a clear mitigation strategy available to us in that timeframe. And I do hope that's before the 4.1 release goes out, yes.

The other option may be simply randomizing the order of these tags and shuffling the order of their attributes:

          [
            tag('meta', :name => 'csrf-param', :content => request_forgery_protection_token),
            tag('meta', :name => 'csrf-token', :content => form_authenticity_token)
          ].join("\n").html_safe

That would cause the target to bounce around a bit in the request, however I'm sure that that could be overcome with additional requests.

Member

NZKoz commented Aug 4, 2013

Yeah, while I may sound like I'm pouring cold water on the work here, I'm really just suggesting that we wait a few months before doing anything as ideally there'll be a clear mitigation strategy available to us in that timeframe. And I do hope that's before the 4.1 release goes out, yes.

The other option may be simply randomizing the order of these tags and shuffling the order of their attributes:

          [
            tag('meta', :name => 'csrf-param', :content => request_forgery_protection_token),
            tag('meta', :name => 'csrf-token', :content => form_authenticity_token)
          ].join("\n").html_safe

That would cause the target to bounce around a bit in the request, however I'm sure that that could be overcome with additional requests.

@waynerobinson

This comment has been minimized.

Show comment
Hide comment
@waynerobinson

waynerobinson Aug 4, 2013

@NZKoz adding one bit of entropy is hardly worth the effort, especially if the attacker knows they are attacking a Rails site with this addition.

The mitigation options are quite well known as they are understood from CRIME and it is effectively the same attack.

This is a security update and should ideally be implemented into the 4.0 and 3.2 (and earlier) branches.

@NZKoz adding one bit of entropy is hardly worth the effort, especially if the attacker knows they are attacking a Rails site with this addition.

The mitigation options are quite well known as they are understood from CRIME and it is effectively the same attack.

This is a security update and should ideally be implemented into the 4.0 and 3.2 (and earlier) branches.

Add a ctor to AuthenticityToken and change class methods to instance
Do all the session manipulation in the constructor, and just operate
on a copy of the CSRF token in the instance methods. Slightly
simplifies the controller mixin code and makes the API read a bit
better.
@waynerobinson

This comment has been minimized.

Show comment
Hide comment
@waynerobinson

waynerobinson Aug 5, 2013

I have not exploited Rails with their proof of concept however, the discussion paper demonstrates that Rails is directly exposed.

The head contains:

<meta content="{authetoken}" name="csrf-token" />

Rails forms contains:

<input name="authenticity_token" type="hidden" value="{authetoken}">

This seems like a pretty well-known prefixes/postfixes to me. The exploit works either forwards or backwards from either name="csrf-token" or name="authenticity_token".

Most browsers don't compress requests (I can't think of any), so the requests aren't exposed to this threat. But the authenticity_token could be

I have not exploited Rails with their proof of concept however, the discussion paper demonstrates that Rails is directly exposed.

The head contains:

<meta content="{authetoken}" name="csrf-token" />

Rails forms contains:

<input name="authenticity_token" type="hidden" value="{authetoken}">

This seems like a pretty well-known prefixes/postfixes to me. The exploit works either forwards or backwards from either name="csrf-token" or name="authenticity_token".

Most browsers don't compress requests (I can't think of any), so the requests aren't exposed to this threat. But the authenticity_token could be

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Aug 5, 2013

Member

I'm not sure if I'm misunderstanding the attack, or you are, but one of us is. In order to expedite things I've pinged the researchers on their contact address, hopefully we can get the final word from them and act accordingly. I can see from the paper that @nealharris is one of the researchers, and works for a company using rails, I'm not sure if he'll get this notification though.

The paper describes how you need to reflect a value the attacker can change in the response body, and contain a known prefix with both the attacker's data and the data you seek to recover. So it's not enough that the csrf token is embedded in every response, you also have to embed the attacker's guess in the response data, and that embedding needs to contain a common prefix.

So if you take a typical rails application, we can rule out responses generated by POST requests as they will require correct CSRF tokens to proceed and otherwise the response will either be an error page (:exception) contain a value which is not the user's csrf_token (:null_session) or cause the session to reset (:reset_session).

Developers are incredibly unlikely to allow attacker-provided data to be put into <meta content=' tags, so in order to be vulnerable I'm assuming we'll need:

  1. A GET request or some other request which the developer has disabled CSRF protection on
  2. The response to that request must to generate a form which contains one of the attacker-provided parameters as the value of an input tag with no modification.
  3. That response must also generate a form with a POST action ensuring that the CSRF token is embedded as the value of an input tag with no modification.
  4. The size of that response must not vary based on the attacker-provided parameters

I suppose this could happen if the application has all of the following:

  • a search form which accepts GET requests
  • the attacker can generate a query for that form which they know will never return any results (assuming the 'no results' page is static)
  • the search form is rendered with all the attacker's relevant search params
  • It also contains an additional param which the attacker uses as his guessing parameter
  • the response also contains a non-ajax POST form. e.g. a login form

This is certainly plausible, especially applications with admin interfaces which allow faceted searching. But it hardly describes every application under the sun? When you add in the requirement for an MITM situation it gets harder still.

I agree with need to ship a mitigation for this, but the first rule of fixing security issues is you fix them correctly and carefully, not in a rush using some idea you found on the internet.

Finally, let's not lecture one another else on the correct way of handling bugs or issues, we're all on the same team here and want the same result. A secure framework. Let's focus on the issues and the fix and not histrionics.

Member

NZKoz commented Aug 5, 2013

I'm not sure if I'm misunderstanding the attack, or you are, but one of us is. In order to expedite things I've pinged the researchers on their contact address, hopefully we can get the final word from them and act accordingly. I can see from the paper that @nealharris is one of the researchers, and works for a company using rails, I'm not sure if he'll get this notification though.

The paper describes how you need to reflect a value the attacker can change in the response body, and contain a known prefix with both the attacker's data and the data you seek to recover. So it's not enough that the csrf token is embedded in every response, you also have to embed the attacker's guess in the response data, and that embedding needs to contain a common prefix.

So if you take a typical rails application, we can rule out responses generated by POST requests as they will require correct CSRF tokens to proceed and otherwise the response will either be an error page (:exception) contain a value which is not the user's csrf_token (:null_session) or cause the session to reset (:reset_session).

Developers are incredibly unlikely to allow attacker-provided data to be put into <meta content=' tags, so in order to be vulnerable I'm assuming we'll need:

  1. A GET request or some other request which the developer has disabled CSRF protection on
  2. The response to that request must to generate a form which contains one of the attacker-provided parameters as the value of an input tag with no modification.
  3. That response must also generate a form with a POST action ensuring that the CSRF token is embedded as the value of an input tag with no modification.
  4. The size of that response must not vary based on the attacker-provided parameters

I suppose this could happen if the application has all of the following:

  • a search form which accepts GET requests
  • the attacker can generate a query for that form which they know will never return any results (assuming the 'no results' page is static)
  • the search form is rendered with all the attacker's relevant search params
  • It also contains an additional param which the attacker uses as his guessing parameter
  • the response also contains a non-ajax POST form. e.g. a login form

This is certainly plausible, especially applications with admin interfaces which allow faceted searching. But it hardly describes every application under the sun? When you add in the requirement for an MITM situation it gets harder still.

I agree with need to ship a mitigation for this, but the first rule of fixing security issues is you fix them correctly and carefully, not in a rush using some idea you found on the internet.

Finally, let's not lecture one another else on the correct way of handling bugs or issues, we're all on the same team here and want the same result. A secure framework. Let's focus on the issues and the fix and not histrionics.

@waynerobinson

This comment has been minimized.

Show comment
Hide comment
@waynerobinson

waynerobinson Aug 5, 2013

I'm definitely not suggesting we implement a crappy solution. But from your initial comments it seemed like you were advocating for a a fix that would get added to Rails 4.1 when that is released, rather than something that is important to back-port to as many previous Rails versions as practical.

I may have misunderstood the requirement to allow injection of the value the attacker is attempting to guess. If this is the case, I'm not sure how this would be exploited in the majority of situations and I apologise for my histrionics. :-)

To mitigate the GET-based attack vector, should we not consider never allowing the CSRF to be overridden (and is it even possible to do so)? I can't think of any reasons why this should be generated any other way than directly from the server.

So if this is the case, then the CSRF protection with Rails isn't at risk and only the actual value of parameters passed into a GET request is ever at risk of CRIME.

If this is the case, I'm not sure how any Rails developer (following REST-standards) could be concerned. I can't think of much that would be passed into a GET request that would be confidential. We have to be missing something right?

I'm definitely not suggesting we implement a crappy solution. But from your initial comments it seemed like you were advocating for a a fix that would get added to Rails 4.1 when that is released, rather than something that is important to back-port to as many previous Rails versions as practical.

I may have misunderstood the requirement to allow injection of the value the attacker is attempting to guess. If this is the case, I'm not sure how this would be exploited in the majority of situations and I apologise for my histrionics. :-)

To mitigate the GET-based attack vector, should we not consider never allowing the CSRF to be overridden (and is it even possible to do so)? I can't think of any reasons why this should be generated any other way than directly from the server.

So if this is the case, then the CSRF protection with Rails isn't at risk and only the actual value of parameters passed into a GET request is ever at risk of CRIME.

If this is the case, I'm not sure how any Rails developer (following REST-standards) could be concerned. I can't think of much that would be passed into a GET request that would be confidential. We have to be missing something right?

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Aug 5, 2013

Member

It's not that the attacker can control the csrf token, but that he can control another parameter that ends up in the form input tags. By doing that, and knowing that the response will also contain the csrf_token the attacker is comparing the effects of compression on:

  <input type='hidden' name='who_cares_but_not_the_token' value='asdfHACKERGUESS' />
  <input type='hidden' name='form_authenticity_token' value='asdfACTUALSECRET' />

That gives them a common prefix value=' with which to bootstrap the guessing. Without both of those being in the response, and there being no CSRF protection on the request itself, then there's no way that his guesses will cause the DEFLATE compression to change the size of the response in a dependable way. There's also the other option that the DEFLATE compression doesn't actually use value=' as it finds other, longer, common strings.

We'll definitely fix this in 4.1, we'll possibly backport that fix to earlier releases, we might publish an advisory. It's all a question of how exploitable this is, the severity, the risk of the fix etc.

Normally these discussions happen behind closed doors and a patch and advisory lands in people's inboxes, however as this is a general issue and disclosed to the public already, we can be a little more open than usual in discussing it. I'm sure our friends at NSA have already been using this for years ;)

Member

NZKoz commented Aug 5, 2013

It's not that the attacker can control the csrf token, but that he can control another parameter that ends up in the form input tags. By doing that, and knowing that the response will also contain the csrf_token the attacker is comparing the effects of compression on:

  <input type='hidden' name='who_cares_but_not_the_token' value='asdfHACKERGUESS' />
  <input type='hidden' name='form_authenticity_token' value='asdfACTUALSECRET' />

That gives them a common prefix value=' with which to bootstrap the guessing. Without both of those being in the response, and there being no CSRF protection on the request itself, then there's no way that his guesses will cause the DEFLATE compression to change the size of the response in a dependable way. There's also the other option that the DEFLATE compression doesn't actually use value=' as it finds other, longer, common strings.

We'll definitely fix this in 4.1, we'll possibly backport that fix to earlier releases, we might publish an advisory. It's all a question of how exploitable this is, the severity, the risk of the fix etc.

Normally these discussions happen behind closed doors and a patch and advisory lands in people's inboxes, however as this is a general issue and disclosed to the public already, we can be a little more open than usual in discussing it. I'm sure our friends at NSA have already been using this for years ;)

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Aug 5, 2013

Contributor

From what I have read about the attack, it doesn't seem like it should require the attacker controlled string to be reflected in an input tag. Couldn't the attacker go after the meta tag directly by starting with " name="csrf-token" /> and working backwards, guessing the last character of the token first?

Contributor

mastahyeti commented Aug 5, 2013

From what I have read about the attack, it doesn't seem like it should require the attacker controlled string to be reflected in an input tag. Couldn't the attacker go after the meta tag directly by starting with " name="csrf-token" /> and working backwards, guessing the last character of the token first?

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Aug 5, 2013

Member

@mastahyeti I think that would require the attacker to be able to embed " and > in the responses, which are escaped due to XSS concerns.

So if the app has an XSS vulnerability in response to a single get request then it could work backwards like that, but an app with an XSS vulnerability doesn't need a CSRF token to be recovered as the attacker could just trick the user into clicking that link and steal what they want from there.

Member

NZKoz commented Aug 5, 2013

@mastahyeti I think that would require the attacker to be able to embed " and > in the responses, which are escaped due to XSS concerns.

So if the app has an XSS vulnerability in response to a single get request then it could work backwards like that, but an app with an XSS vulnerability doesn't need a CSRF token to be recovered as the attacker could just trick the user into clicking that link and steal what they want from there.

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Aug 5, 2013

Contributor

Good point. They would at least need ".

Contributor

mastahyeti commented Aug 5, 2013

Good point. They would at least need ".

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Aug 5, 2013

Member

probably more than " as two characters are unlikely to have a measurable impact on the DEFLATE processing.

Member

NZKoz commented Aug 5, 2013

probably more than " as two characters are unlikely to have a measurable impact on the DEFLATE processing.

@waynerobinson

This comment has been minimized.

Show comment
Hide comment
@waynerobinson

waynerobinson Aug 5, 2013

Could probably squeeze =" name="csrf-token" / in somewhere in the output though.

Could probably squeeze =" name="csrf-token" / in somewhere in the output though.

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Aug 5, 2013

Contributor

I just meant that they wouldn't need the > character. If there was some context where quotes weren't escaped for XSS, they could start with " name="csrf-token". html_escape and escape_javascript will both munge quotes though.

With that in mind though, another mitigating factor is that there are likely to be many value= and content= strings on any given page, making it very difficult for the attacker to isolate the tag containing the CSRF token if we are assuming they only control the value of the value attribute of some tag.

Contributor

mastahyeti commented Aug 5, 2013

I just meant that they wouldn't need the > character. If there was some context where quotes weren't escaped for XSS, they could start with " name="csrf-token". html_escape and escape_javascript will both munge quotes though.

With that in mind though, another mitigating factor is that there are likely to be many value= and content= strings on any given page, making it very difficult for the attacker to isolate the tag containing the CSRF token if we are assuming they only control the value of the value attribute of some tag.

+ def initialize(session, logger = nil)
+ session[:_csrf_token] ||= SecureRandom.base64(LENGTH)
+ @master_csrf_token = Base64.strict_decode64(session[:_csrf_token])
+ @logger = logger

This comment has been minimized.

@egilburg

egilburg Aug 5, 2013

Contributor

Would be friendly to make logger an attr_accessor so other code/tests can get/set it post-initialize.

@egilburg

egilburg Aug 5, 2013

Contributor

Would be friendly to make logger an attr_accessor so other code/tests can get/set it post-initialize.

This comment has been minimized.

@jmazzi

jmazzi Aug 5, 2013

Contributor

Unless it's being used outside this class right now, I don't think that is needed, personally :)

@jmazzi

jmazzi Aug 5, 2013

Contributor

Unless it's being used outside this class right now, I don't think that is needed, personally :)

@@ -203,5 +203,9 @@ def form_authenticity_param
def protect_against_forgery?
allow_forgery_protection
end
+
+ def authenticity_token
+ AuthenticityToken.new(session, logger)

This comment has been minimized.

@egilburg

egilburg Aug 5, 2013

Contributor

You don't need to generate this over and over in a single request, right? Please memoize this e.g. @authenticity_token ||= AuthenticityToken.new(session, logger), so this doesn't generate several times between calls in the same request.

@egilburg

egilburg Aug 5, 2013

Contributor

You don't need to generate this over and over in a single request, right? Please memoize this e.g. @authenticity_token ||= AuthenticityToken.new(session, logger), so this doesn't generate several times between calls in the same request.

+
+ masked_token == @master_csrf_token
+
+ elsif masked_token.length == LENGTH * 2

This comment has been minimized.

@egilburg

egilburg Aug 5, 2013

Contributor

Might seem like a micro-optimization, but also for slightly better readability, I'd rename the constant LEGNTH to UNMASKED_LENGTH (or RAW_LENGTH), and right below it define MASKED_LENGTH = UNMASKED_LENGTH * 2. Then I'd just be checking against the two constants here. Also, make it a case statement and put the more commonly expected case further up:

case masked_token.length
when MASKED_LENGTH
  # ...
when UNMASKED_LENGTH
  # ...
else
  false
end
@egilburg

egilburg Aug 5, 2013

Contributor

Might seem like a micro-optimization, but also for slightly better readability, I'd rename the constant LEGNTH to UNMASKED_LENGTH (or RAW_LENGTH), and right below it define MASKED_LENGTH = UNMASKED_LENGTH * 2. Then I'd just be checking against the two constants here. Also, make it a case statement and put the more commonly expected case further up:

case masked_token.length
when MASKED_LENGTH
  # ...
when UNMASKED_LENGTH
  # ...
else
  false
end

This comment has been minimized.

@steveklabnik

steveklabnik Aug 5, 2013

Member

case statements return nil which is falsy if none of the whens happen so you wouldn't need the else clause.

@steveklabnik

steveklabnik Aug 5, 2013

Member

case statements return nil which is falsy if none of the whens happen so you wouldn't need the else clause.

This comment has been minimized.

@egilburg

egilburg Aug 5, 2013

Contributor

Do you think it's good practice for Boolean methods to return explicit true or false, not just truthy or falsy?

@egilburg

egilburg Aug 5, 2013

Contributor

Do you think it's good practice for Boolean methods to return explicit true or false, not just truthy or falsy?

This comment has been minimized.

@steveklabnik

steveklabnik Aug 6, 2013

Member

I think it's actively bad practice.

@steveklabnik

steveklabnik Aug 6, 2013

Member

I think it's actively bad practice.

@nealharris

This comment has been minimized.

Show comment
Hide comment
@nealharris

nealharris Aug 5, 2013

One thing to note regarding bootstrapping: only three characters are necessary to get it going. Consider

<meta content="csrf_token_which_is_base64_encoded=" name="csrf-token" />

Note that if an attacker's query param is reflected in an attribute value anywhere in the page as in

<input type="hidden" name="who_cares_but_not_the_token" value="asdfHACKERGUESS" />

then the ="<FIRST_CHAR_OF_HACKER_GUESS> in value="asdfHACKERGUESS" will match against the ="<FIRST_CHAR_OF_ACTUAL_SECRET> in content="csrf_token_which_is_base64_encoded=" when the guess is correct. The actual name of the attribute doesn't have to match. This is likely to be noisy for the attacker, given how many other places in the page they might be colliding with. Worth mentioning though.

Also, I emphasize b64 encoding here, since that gives the attacker some idea of how the secret will end, and allows them to use the same idea to bootstrap from the end, instead of the beginning.

One thing to note regarding bootstrapping: only three characters are necessary to get it going. Consider

<meta content="csrf_token_which_is_base64_encoded=" name="csrf-token" />

Note that if an attacker's query param is reflected in an attribute value anywhere in the page as in

<input type="hidden" name="who_cares_but_not_the_token" value="asdfHACKERGUESS" />

then the ="<FIRST_CHAR_OF_HACKER_GUESS> in value="asdfHACKERGUESS" will match against the ="<FIRST_CHAR_OF_ACTUAL_SECRET> in content="csrf_token_which_is_base64_encoded=" when the guess is correct. The actual name of the attribute doesn't have to match. This is likely to be noisy for the attacker, given how many other places in the page they might be colliding with. Worth mentioning though.

Also, I emphasize b64 encoding here, since that gives the attacker some idea of how the secret will end, and allows them to use the same idea to bootstrap from the end, instead of the beginning.

+ if @logger
+ @logger.warn "The client is using an unmasked CSRF token. This " +
+ "should only happen immediately after you upgrade to masked " +
+ "tokens; if this persists, something is wrong."

This comment has been minimized.

@NZKoz

NZKoz Aug 5, 2013

Member

This doesn't actually matter, if an attacker has the raw token, they can generate a valid masked token, I think we can safely ignore it rather than log.

@NZKoz

NZKoz Aug 5, 2013

Member

This doesn't actually matter, if an attacker has the raw token, they can generate a valid masked token, I think we can safely ignore it rather than log.

This comment has been minimized.

@homakov

homakov Aug 5, 2013

Contributor

yes, such a notification is very rare. only for those who opened page just before deployment

@homakov

homakov Aug 5, 2013

Contributor

yes, such a notification is very rare. only for those who opened page just before deployment

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Aug 5, 2013

Contributor

discussion above is pretty long but @NZKoz is right, w/o reflection it doesn't work! you need it to put guesses.
Although it's very easy to find a reflection. Search etc

Contributor

homakov commented Aug 5, 2013

discussion above is pretty long but @NZKoz is right, w/o reflection it doesn't work! you need it to put guesses.
Although it's very easy to find a reflection. Search etc

@nealharris

This comment has been minimized.

Show comment
Hide comment
@nealharris

nealharris Aug 6, 2013

One more comment: when we were doing our research, we definitely took a look at Rails to see how easy/difficult the attack would be there. It didn't make life easy for us (for essentially the reasons discussed above), and wouldn't have been a good candidate for a demo. That being said, as I think everyone here realizes: Rails is vulnerable in in principle.

Anyway, the fact that @angeloprado, @ygluck, and I didn't find a good way to attack Rails shouldn't provide too much comfort. There are lots of people out there that are way more industrious and clever than we are.

One more comment: when we were doing our research, we definitely took a look at Rails to see how easy/difficult the attack would be there. It didn't make life easy for us (for essentially the reasons discussed above), and wouldn't have been a good candidate for a demo. That being said, as I think everyone here realizes: Rails is vulnerable in in principle.

Anyway, the fact that @angeloprado, @ygluck, and I didn't find a good way to attack Rails shouldn't provide too much comfort. There are lots of people out there that are way more industrious and clever than we are.

@Gawdl3y Gawdl3y referenced this pull request in laravel/framework Aug 6, 2013

Closed

Mask CSRF tokens to mitigate BREACH attack #2046

@acorncom acorncom referenced this pull request in yiisoft/yii Aug 26, 2013

Closed

Mitigate BREACH attack possibilities #2819

+ "tokens; if this persists, something is wrong."
+ end
+
+ masked_token == @master_csrf_token

This comment has been minimized.

@tarcieri

tarcieri Oct 2, 2013

Contributor

Is this vulnerable to a timing attack?

@tarcieri

tarcieri Oct 2, 2013

Contributor

Is this vulnerable to a timing attack?

+ encrypted_csrf_token = masked_token.last(LENGTH)
+ csrf_token = self.class.xor_byte_strings(one_time_pad, encrypted_csrf_token)
+
+ csrf_token == @master_csrf_token

This comment has been minimized.

@tarcieri

tarcieri Oct 2, 2013

Contributor

Likewise, is this also vulnerable to a timing attack?

@tarcieri

tarcieri Oct 2, 2013

Contributor

Likewise, is this also vulnerable to a timing attack?

This comment has been minimized.

@NZKoz

NZKoz Oct 2, 2013

Member

timing attacks are unlikely to be an issue here, CSRF tokens are particular to the session, not server side. The CSRF attacker doesn't have the ability to see the requests and time their responses unless you're talking about a fairly specific set of scenarios like an active MITM attacker. I'd love to seem someone demo that as it'd be quite a neat trick, but seems an unlikely vector at present.

We could change it, but it'd be belt-and-suspenders style stuff not a critical fix.

@NZKoz

NZKoz Oct 2, 2013

Member

timing attacks are unlikely to be an issue here, CSRF tokens are particular to the session, not server side. The CSRF attacker doesn't have the ability to see the requests and time their responses unless you're talking about a fairly specific set of scenarios like an active MITM attacker. I'd love to seem someone demo that as it'd be quite a neat trick, but seems an unlikely vector at present.

We could change it, but it'd be belt-and-suspenders style stuff not a critical fix.

This comment has been minimized.

@tarcieri

tarcieri Oct 2, 2013

Contributor

Well, you're trying to mitigate BREACH here, which already requires the attacker MITM with a TCP proxy and can drive the victim's browser. A timing attack on CSRF would have the exact same setup, but rather than using the TCP proxy to measure the length of the response, they'd use it to measure timing information (which, granted, is harder than measuring the number of bytes in the response ;)

@tarcieri

tarcieri Oct 2, 2013

Contributor

Well, you're trying to mitigate BREACH here, which already requires the attacker MITM with a TCP proxy and can drive the victim's browser. A timing attack on CSRF would have the exact same setup, but rather than using the TCP proxy to measure the length of the response, they'd use it to measure timing information (which, granted, is harder than measuring the number of bytes in the response ;)

This comment has been minimized.

@NZKoz

NZKoz Oct 2, 2013

Member

Sure, I'm not denying it's theoretically possible, nor rejecting the idea of changing the code. Just not FREAKING OUT at every use of String#== ;).

@NZKoz

NZKoz Oct 2, 2013

Member

Sure, I'm not denying it's theoretically possible, nor rejecting the idea of changing the code. Just not FREAKING OUT at every use of String#== ;).

This comment has been minimized.

@tarcieri

tarcieri Oct 2, 2013

Contributor

Note that I am not one to FREAK OUT at every use of String#== either [1] :P But given the attack this patch is intending to mitigate, I think it's warranted

@tarcieri

tarcieri Oct 2, 2013

Contributor

Note that I am not one to FREAK OUT at every use of String#== either [1] :P But given the attack this patch is intending to mitigate, I think it's warranted

This comment has been minimized.

@mastahyeti

mastahyeti Oct 2, 2013

Contributor

Just playing around a bit, you can get millisecond level timing info on a cross-domain POST, but that obviously isn't enough for a timing attack against string comparisons.

@mastahyeti

mastahyeti Oct 2, 2013

Contributor

Just playing around a bit, you can get millisecond level timing info on a cross-domain POST, but that obviously isn't enough for a timing attack against string comparisons.

This comment has been minimized.

@tarcieri

tarcieri Oct 2, 2013

Contributor

What if the attacker is MitMing you with a TCP proxy (like they would have to do for BREACH anyway?)

@tarcieri

tarcieri Oct 2, 2013

Contributor

What if the attacker is MitMing you with a TCP proxy (like they would have to do for BREACH anyway?)

+ masked_token = SecureRandom.random_bytes(LENGTH)
+
+ # XOR the random bits with the real token and concatenate them
+ encrypted_csrf_token = self.class.xor_byte_strings(masked_token, @master_csrf_token)

This comment has been minimized.

@tarcieri

tarcieri Oct 2, 2013

Contributor

"encrypted?"

@tarcieri

tarcieri Oct 2, 2013

Contributor

"encrypted?"

This comment has been minimized.

@NZKoz

NZKoz Oct 2, 2013

Member

yeah, masked is a better name for it

@NZKoz

NZKoz Oct 2, 2013

Member

yeah, masked is a better name for it

This comment has been minimized.

@elithrar

elithrar Nov 11, 2013

It may make more sense to rename masked_token to one_time_pad as per #L45.

masked_token is likely a better name for the XOR of the real CSRF token and our one-time pad.

@elithrar

elithrar Nov 11, 2013

It may make more sense to rename masked_token to one_time_pad as per #L45.

masked_token is likely a better name for the XOR of the real CSRF token and our one-time pad.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Nov 10, 2013

Contributor

Hey @bradleybuda have you had time to react to the feedback from @NZKoz and @tarcieri? Seems like this is something important to get fixed...

Contributor

JonRowe commented Nov 10, 2013

Hey @bradleybuda have you had time to react to the feedback from @NZKoz and @tarcieri? Seems like this is something important to get fixed...

nealharris pushed a commit to square/rails that referenced this pull request Feb 9, 2014

nealharris pushed a commit to square/rails that referenced this pull request Mar 7, 2014

nealharris pushed a commit to square/rails that referenced this pull request Mar 7, 2014

nealharris pushed a commit to square/rails that referenced this pull request Mar 21, 2014

@nealharris nealharris referenced this pull request in square/rails Mar 21, 2014

Merged

patch breach for csrf tokens. #18

@dosire

This comment has been minimized.

Show comment
Hide comment
@dosire

dosire Mar 25, 2014

Contributor

For people waiting for this to get merged consider using https://github.com/meldium/breach-mitigation-rails in the meantime

Contributor

dosire commented Mar 25, 2014

For people waiting for this to get merged consider using https://github.com/meldium/breach-mitigation-rails in the meantime

@robin850 robin850 added this to the 4.2.0 milestone Apr 6, 2014

@robin850 robin850 added the security label Apr 12, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Oct 31, 2014

Member

See #16570 for the simple masking merged from breach-mitigation-rails. This PR nicely encapsulates the authenticity token, would be welcome to rebase and continue with its abstraction.

Member

jeremy commented Oct 31, 2014

See #16570 for the simple masking merged from breach-mitigation-rails. This PR nicely encapsulates the authenticity token, would be welcome to rebase and continue with its abstraction.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 31, 2015

Member

Closing due inactivity and since an alternative solution was already merged.

Member

rafaelfranca commented Dec 31, 2015

Closing due inactivity and since an alternative solution was already merged.

@rafaelfranca rafaelfranca removed this from the 5.0.0 [temp] milestone Dec 31, 2015

@jkowens jkowens referenced this pull request in sinatra/rack-protection Aug 15, 2016

Closed

Mitigate BREACH attack #115

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