Rails Ruby on Rails

Comments and changes to this ticket

• 

  CancelProfileIsBroken September 8th, 2009 @ 06:51 PM

  • Assigned user set to “Michael Koziarski”

• You flagged this item as spam.
  

  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 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.

• You flagged this item as spam.
  

  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.