Comment by tptacek | Hacker News Reader

Comment by tptacek | Hacker News Reader

I can probably go all night on this, but a couple things from a quick read of this (very good) post:

First, mass assignment.

The answer to mass-assignment bugs is "attr_accessible". Accessible attributes can be set via update/build/new; nothing else can. Every Rails AR model should have an "attr_accessible" line in it.

I've met smart dev teams working under the misconception that attr_accessible means "these are the attributes that can be changed based on user requests", and so virtually everything is made accessible. No! If something's not attr_accessible, you just set it manually (user.foo = params[:user][:foo]). It's not painful and the extra line expresses something important ("this is a sensitive attribute"). Attributes are inaccessible until they prove themselves mass-assignment-worthy.

Second, the string interpolation in the regex.

Real quick: don't ever let users interpolate arbitrary strings into regular expressions. Regular expression libraries are terribly complicated and not very well tested. To illustrate (but not fully explain) the danger here, run this line of code:

     ruby -e "'=XX===============================' =~
     /X(.+)+X/"

There are worse things that can happen to you with regex injection than a trivial DoS, but that should be enough motivation.

Oh, one more thing: I appreciate Patrick's take on systems failures breaking Rails apps before underlying crypto flaws will, but even if they had protected their keys, their crypto wouldn't have worked. Don't build things that require crypto. You aren't going to get it right.

Every Rails AR model should have an "attr_accessible" line in it.

I'd do you one better: use an initializer to monkeypatch ActiveRecord::Base and fire "attr_accessible nil", which will cause mass assignment to fail on any object you create from a class which doesn't make the assignment explicit.

That's clever. Want a job? =)

You two, get a (conference) room :-p

Good luck! I've been trying to hire him for months now. ;p

Is that offer good for anyone? http://news.ycombinator.com/item?id=1031126

Of course. Drop me a line. I'd love to talk to you. We love talking to HN people.

I'm keeping this in my back pocket the next time I have a conversation about why I prefer Ruby's monkey patching paradigm to Python's strictness. This is better than all my current examples. :)

The good part about Ruby: you can monkeypatch around framework defaults which do not maximize for your project's circumstances. The bad part about Ruby: your least talented coder can monkeypatch around security features which make his life more difficult ("attr_accessible? Stupid Rails coders, don't they know they have private for that shit? Well, I'll redefine it to just NOP. I am the awesome!")

I find educating the least talented coder I work with a mostly social problem that I can solve over lunch [1]. But yes, I've definitely felt the pain of monkey patching gone awry. :-p Working around a restriction enforced by your language is no picnic either though and it's not really something you can solve cleanly.

[1] Obviously large companies with massive Ruby code bases can't really do this. Not sure what to say there.

The real problem is with the coders that you don't eat lunch with — the authors of the shitty gems that get pulled in as dependencies.

For your companies' code reopening a class should be a huge flag in code review (something like gerrit should be in place at every large company), but it's not sustainable to police the dependencies of the libraries you use, especially when the default in the Rails community is spray and pray.

Note that this specific fix doesn't need you to open/monkey-patch ActiveRecord::Base, you can just do ActiveRecord::Base.send(:attr_accessible, nil) in an initializer.

In Shapado we use a safe_update methode like this so we always need to specify which attribute can be updated:

@question.safe_update(%w[title body language tags], params[:question])

I like this better than my solution, which was to specific which params were allowed for each controller action and remove any that weren't allowed.

I'd go one further and say that if you use string interpolation with anything remotely related to user input at all in a web app, you probably just wrote a security hole of some sort. I use "probably" in the frequentist sense of the term, because the odds of a given user-input string interpolation in an arbitrarily-chosen web app being in at least one of 1. an HTML string 2. a database string 3. a javascript string or 4. something else that is executable by something, without the context-relevant encoding function, are quite high. Same goes for any string concatenation involving anything from the environment or user without the conspicuous presence of an encoding function of some kind, since interpolation is just syntax sugar for concatenation.

"Don't let users interpolate, ever" is close to truth. It isn't quite truth, but it's a lot shorter than the truth.

ruby -e "'=XX===============================' =~ /X(.+)+X/"

Why does that hang in Ruby? In Perl it's fine...

Because Ruby uses PCRE, but Perl uses its own regex engine which handles cases like that a lot better. That pattern is rejected right away by Perl's engine because it sees that there isn't another X in that string:

    anchored "X" at 0 floating "X" at 2..2147483647 (checking floating) minlen 3 
    Guessing start of match in sv for REx "X(.+)+X" against "=XX==============================="
    Found floating substr "X" at offset 2...
    Contradicts anchored substr "X", trying floating at offset 3...
    Did not find floating substr "X"...
    Match rejected by optimizer

Changing the pattern to /X(.)X/ will cause Perl to do real work. But it'll complete it in 0.003s while Ruby will just hang.

I wrote an article a while back about the differences between PCRE and Perl's engine: http://use.perl.org/~avar/journal/33585

Thanks for the explanation

Well... basically, it sounds like Ruby's regex engine needs some work, hmm?

No, people should know better than to write regexes like /X(.+)+X/, with gratuitous doubly-nested "+" characters. :-) This code performs fine when written as /X(.+)X/, and it matches the same set of strings.

Regexp engines are subtle beasts, and there's a couple different ways to implement them (DFAs vs NFAs, simple engines vs lots of clever special cases, etc.). See O'Reilly's "Mastering Regular Expressions" for an exhaustive discussion.

Interesting. Any other recommendations on how to secure regex's that take in user input in Ruby/Rails?

First of all you should think hard before taking regexes from users. Even if you do it correctly you'll still (presumably) need to search over your entire dataset, instead of doing something more lightweight like rely on SQL indexes. Use it with care.

You should use a regex engine that's explicitly designed to take potentially hostile input. Like the Plan9 engine, or Google's re2 engine which powers Google Code Search.

You can also just use Ruby's dangerous PCRE engine if you do something like forking off another process with strict ulimits which executes the regex for you. Then you can just kill it if it starts running away with your resources. Look into how e.g. evaluation bots that work on the popular IRC channels on FreeNode are implemented. POE::Component::IRC::Plugin::Eval on the CPAN is a good example.

This assumes that PCRE doesn't still contain memory corruption flaws, despite not being heavily tested, and being in effect a programming language interpreter. Tavis Ormandy found a couple serious problems a few years ago.

I'd just scrub the hell out of strings before passing them to a regex engine.

Good advice, thanks you two!

Even if it does it's a pretty remote possibility that it'll be exploitable if you limit the input to say 100 bytes. Pretty hard to get a Perl or Ruby level program of that size to exploit some memory corruption at the C level.

It just seems to me that if Perl's regex engine handles this without a problem, and Ruby's implementation freaks out, something should be improved about the Ruby one.

I don't have access to a box with ruby on at the moment, what does that line do?

It just hangs ruby (like in an endless loop)

Ah fair enough, thanks!

Reply on news.ycombinator.com