This project is archived and is in readonly mode.

#3168 ✓wontfix
Brian Mitchell

A better way to avoid timing attacks on the message verifier

Reported by Brian Mitchell | September 8th, 2009 @ 04:48 PM

As seen on this commit: http://github.com/rails/rails/commit/aeab739bd56c0bff6d1b5685eee35e...

It would be a better idea to avoid the timing drop on positive cases and instead provide a non-deterministic timing for the negative case only.

This patch simply provides an obvious refactor that implements this timer. It's simpler code and is more portable between ruby 1.8 and 1.9.

Comments and changes to this ticket

  • CancelProfileIsBroken

    CancelProfileIsBroken September 8th, 2009 @ 06:51 PM

    • Assigned user set to “Michael Koziarski”
  • Grant Hutchins

    Grant Hutchins September 9th, 2009 @ 12:31 AM

    I disagree. Adding noise to the signal just means that any timing attacks would just need to take a larger sample size to get to the same result.

    I'm not saying that it's probable that it would happen, but just that this change would add risk for minimal benefit.

    Also, the 200 hard-coded in the sleep line is a code smell. The same value probably would not work well for systems running at different speeds.

    If we really want to avoid timing attacks, we should eat up the slight performance hit.

  • Michael Koziarski

    Michael Koziarski September 9th, 2009 @ 06:56 AM

    • State changed from “new” to “wontfix”

    The timing difference is trivial, this approach has been vetted by security folks and is consistent with what 'they' recommend.

    Marking wontfix.

  • Brian Mitchell

    Brian Mitchell September 9th, 2009 @ 05:30 PM

    Grant, Right except... It was measured to make it statistically astronomical to attack. If you actually look at the entropy a sleep alone gives you it would probably take trillions of samples to effectively break this assuming you even have a web stack that is also very consistent which, from my measurements is not true in a typical case. (Also consider that load balancing makes it somewhat harder to detect too since many machines will vary).

    It was really a statement that this sort of fix is possibly changing something that wasn't broken... or made as a suggestion from someone who hadn't really measured very much.

    As Michael just said, the timing differences are trivial. Ironic in a way. Also, 110µs (1.8) to 140µs (1.9) might be small but these do add up given how many places code like this exists. The string comparison on the other hand is fast enough that it won't even register in ruby's relatively high resolution timers on a benchmark.

    Anyway, not a huge deal but I like 3 things about my patch: it's less code, it creates less garbage on 1.9 (100's of small strings just makes fragmentation problems worse since things aren't generational), and it does add entropy to the measurement that pretty much defeats practical timing attacks even if we were just running the comparison and not the rest of the web stack.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Pages