Hacker News Reader

Comment by sync | Hacker News Reader

This one seems pretty amateur and should have been caught as part of a pull request/code review process.

   - @regex = /^(#{Regexp.escape(@prefix)})(.+?)(#{Regexp.escape(@suffix)})$/
   + @regex = /\A(#{Regexp.escape(@prefix)})(.+?)(#{Regexp.escape(@suffix)})\z/

^ and $ only match the first line in ruby, whereas \A and \z match across all lines.

View on HN

"This one seems pretty amateur and should have been caught as part of a pull request/code review process"

I say that about 95% of the bugs I ever see...especially ones written by me.

More specifically, \A and \z are "start of string" and "end of string" respectively.

Why not \Z, you ask? \Z will match to the end of the string but ignore a trailing newline.

  irb(main):001:0> !!("hello\n" =~ /\Ahello\z/)
  => false
  irb(main):002:0> !!("hello\n" =~ /\Ahello\Z/)
  => true

This is an extremely common bug that is not specific to Rails. It would be worth reviewing your code to look at every regex to see if you have similar flaws.

I seem to remember a blog post about this regex issue here on HN a few months ago. It definitely surprised me to learn that Ruby doesn't treat $ as end-of-string by default.

I blogged in http://advogato.org/person/fxn/diary/498.html some key differences between Perl and Ruby regexp flags (which includes this gotcha).

http://homakov.blogspot.com/2012/05/saferweb-injects-in-vari... it is usual mistake in url validators

I feel a bit dumb, but I never would have thought about this one. So, is this best practice for any kind of validation regex?

Yes. Otherwise, say your username validation regex looks like /^[a-z0-9]+$/ (one which I see all the time). It's pretty simple for me to send this: "a\n☃" ("a\n<snowman>" if you can't see it) and it'll validate. I say "pretty simple" because you can do it in many browsers just by pasting text with a newline in it into a form field - it can even be done by accident, no malicious intent necessary.

In general, you may be better off avoiding regexes when you can, especially if it's security-sensitive. They're very useful, but they're very easy to get wrong, especially when they get complex. This case, for instance, looks like it would have been impossible if they checked if the attribute were in a list, instead of building a regex. It might be faster with a regex in this case, but for most people that's a (massively) premature optimization for (imperceptibly) small gain.

This is what everybody says when studying mathematical proofs of theorems, especially the good ones. Obvious, isn't it?

That depends on how long it's been in the codebase. As this case is covered in the Rails security guide _and_ the Ruby security reviewer's guide I'd expect it to be quite old and now being found because it hasn't been properly audited before or indicate that the review process needs some work.

So basically this attack relies on putting a not-blacklisted identifier on the first line?

How do you then get rails to assign it to the correct (blacklisted) identifier?

Reply on news.ycombinator.com