This looks good, although it seems like there should be a config setting as I could imagine some use cases where the application expects the token not to change in this way. I'm not sure how common this and whether or not such a setting should be enabled by default, but I think it should be considered.
-- Michael Mior On Sunday, August 3, 2014 9:15:15 PM UTC-4, Adam Brenecki wrote: > > Hi all, > > So, a while ago, BREACH happened, and Django's CSRF implementation was > vulnerable, as was Rails'. The paper that discussed it described a > mitigation (and a Rails patch had already been made), so I implemented that > same mitigation in a Django patch. Discussion on the Trac ticket has > stalled, and I've been told this is the place to go. > > Disclaimer: although I understand enough about it to write the patch, *I'm > not a security person* - the reason I'm posting here is that I'm hoping > to get an answer from someone that *is* a security person as to whether > I'm on the right track. > > To jog your memories (and also in the hope that if I'm misunderstanding > the problem, someone will correct me), the short version of BREACH is: an > attacker forcing a user to visit a HTTPS page that a) is gzipped, and b) > contains *in the response body* a secret (in our case, the CSRF token in > the form), as well as user input (e.g. reflected URL parameters), then > observing the size of the HTTPS responses. When the user input partially or > completely matches the secret, the compressed length is slightly shorter, > and the attacker can use this to guess the secret byte-by-byte. > > In section 3 of the paper, the authors describe a variety of mitigations. > One of them is to, on each request where a secret S should appear, > generate a new one-time pad P and instead write P + xor(P, S) in the > response. (The paper also describes other mitigations, but this is the most > feasible¹.) > > My understanding is (and I'll reiterate here I'm not a security person) > that this should make S impossible to deduce via BREACH, as it doesn't > appear directly anywhere in the response. As P takes on a new value with > every request, it simply can't be deduced by any method that involves > making many requests; therefore the same thing will happen to xor(P, S). > (I think the fact that P changes every request might have been a point of > confusion in the Trac discussion; there's a lot of comments there talking > about trying to deduce P). > > The patch I've written implements this mitigation, with one difference: > instead of using xor, it uses a Vigenère cipher (as suggested by FunkyBob), > as xor was creating non-printable characters which caused problems. I think > this should be OK as Vigenère is commonly used for one-time pads and does > more or less the same thing to characters that xor does to bits. > > This is pretty much what django-debreach does, except that *it* uses AES > instead of xor (i.e. the output is K + AES(K, S).). However, this adds > processing load to every request and a dependency on PyCrypto, and as far > as I can tell this doesn't actually add any benefit over xor/Vigenère. > > So, in summary, I think I'm doing nearly exactly what the paper says, and > I think it effectively makes the attack practically impossible, but I'd > love to hear from someone who has a better idea than I as to if I'm > actually correct on all of this. > > Thanks, > Adam > > The Trac ticket: https://code.djangoproject.com/ticket/20869 > My pull request: https://github.com/django/django/pull/1477 > The original paper: > http://breachattack.com/resources/BREACH%20-%20SSL,%20gone%20in%2030%20seconds.pdf > > (the relevant bit is section 3.4 at the bottom of page 10) > The related Rails pull request: https://github.com/rails/rails/pull/11729 > > ¹ Section 3.4 describes the mitigation I implemented. Section 3.1 > describes fuzzing the lengths of each response by appending random lengths > of garbage data, then immediately dismisses this method as ineffective; > Russell Keith-Magee tells me that a discussion within Django reached the > same conclusion about this one. Apart from those two, they're all things > like rate limiting and separating secrets from user input, which don't seem > to me like things we can enforce on a Django level. > -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/eaf3df25-b49d-4419-aa48-6b5437f3c1f1%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
