
Multiple security vulnerabilities in Rails - alinajaf
https://groups.google.com/forum/#!forum/rubyonrails-security
======
andersonmvd
I've grouped the patches for 4.1 and 4.2 here:
[https://drive.google.com/file/d/0BwnrE2iUdypUMkpqWVVPTXNzNVU...](https://drive.google.com/file/d/0BwnrE2iUdypUMkpqWVVPTXNzNVU/view)
\-- because download one by one is boring. Don't trust me, verify each file
before patching. Some comments:

[CVE-2015-7581] Object leak vulnerability for wildcard controller routes in
Action Pack: Look for routes that contain ":controller" and change it to
something else. Hopefully you didn't have this weird name in your routes.

[CVE-2015-7578/79] Possible XSS vulnerability in rails-html-sanitizer: You're
safe if you use a single page application that properly encode for you.
Stripping tags isn't the best way anyway to filter XSS, so if you're encoding,
you're good.

[CVE-2016-0753] Possible Input Validation Circumvention in Active Model:
params.permit! is negligence, you should not be doing that anyway

[CVE-2016-0752] Possible Information Leak Vulnerability in Action View: render
params[:id] is not defensive programming, so you should not be doing that too

[CVE-2015-7577] Nested attributes rejection proc bypass in Active Record: Only
if using nested_attributes and rejection proc. Wasn't my case. Just patch.

[CVE-2016-0751] Possible Object Leak and Denial of Service attack in Action
Pack: DoS is bad, just patch.

[CVE-2015-7576] Timing attack vulnerability in basic authentication in Action
Controller: Just patch.

\-- Doesn't look THAT bad, but need to be patched fast.

~~~
Someone1234
Wow you have a terrible attitude about security. "None of these are an issue,
just program in this [very specific way that requires pre-knowledge of these
vulnerabilities] and you're safe, anything else is basically negligence."

Your opinions about rails-html-sanitizer are particularly troubling as even if
you use the sanitizer as suggested in the docs you're vulnerable and your
retort is "well you should encode AND sanitise, not just rely on the sanitiser
doing what the documentation says it should do!" Why?

I have no issue with the wording in the official CVEs. But this attempt at
whitewashing the, frankly, pretty serious issues is deplorable.

~~~
eropple
I have a much bigger problem with your post than his--and calling him out for
a "terrible attitude" regarding security is a little bit funny seeing as he
runs Gauntlet.io, which while not my favorite scanner out there is a legit
tool that deserves more respect than you have afforded him. He has his bona
fides; where are yours?

And, more concretely, I have no problem with his pointing out that some of--
not all, but some--these issues are ones mitigated by decent, security-aware
software development practices, like, oh, "don't spit something into a
template out of your input parameters". Because tools break. Your libraries
break. They will always break. Program defensively in all situations where
hostile input is possible and never take for granted any attempts to defuse
attacks; always favor braces-and-belt wherever possible and you'll generally
do okay. It's entirely unnecessary to be a jerk towards him for saying this.

~~~
Someone1234
> He has his bona fides; where are yours?

I called him out for his specific replies in this specific thread. I didn't
call out his reputation or character.

Maybe you should stick to what they and I actually posted here today, and not
try to draw the conversation off track into reputation wars.

> It's entirely unnecessary to be a jerk towards him for saying this.

I don't appreciate being called a "jerk."

I stand by what I said, and what I said was that I felt (and feel) that they
have a bad attitude to security. They're trying to shift blame from the rails
developers to every rails user, and their excuses are weak.

If you think that is "jerky," that's fine, but I feel like name calling and
playing the reputation card for no reason is only going to take a conversation
down a bad and unconstructive path.

~~~
enraged_camel
>>I don't appreciate being called a "jerk

Then try not to sound like one? The OP went out of his way to compile and
document the important bugs and post them here. You, on the other hand,
completely ignored the value of his contribution and instead rudely called him
out on a very minor (and frankly, subjective) tonal issue, and went so far as
to call it "whitewashing," which implies that he has an agenda.

~~~
ryanlol
Did you not notice how OP had absolutely nothing to say about the more serious
bugs and instead entirely focused on the less serious ones? (Except for the
rails-html-sanitizer bug, which is fairly serious)

He does make multiple valid points though.

~~~
bpchaps
I don't understand comments like these.. He's pointing out a legitimate
attitude problem which I, and many others, also agree with. There's absolutely
no need for the flippant attitude.

If the users of your framework are consistently causing major security
problems and the framework is built in a way that it can't be fixed without
compromise.. I dunno.. document it? Maybe? Your documentation is basically the
API to learning your framework, so if the API is broken to the extent of
causing security problems, then it's not god damn production ready!

Remember, if every student is failing your class, the student probably isn't
the one to blame.

~~~
enraged_camel
>>Remember, if every student is failing your class, the student probably isn't
the one to blame.

Except not every student is failing the class. Only those not following best
practices are.

For example, if you're using params.permit!, you're simply being lazy. This is
clearly documented[1]:

" _Extreme care should be taken when using permit! as it will allow all
current and future model attributes to be mass-assigned._ "

[1][https://github.com/rails/strong_parameters#permitted-
scalar-...](https://github.com/rails/strong_parameters#permitted-scalar-
values)

------
nfm
Doesn't look _too_ bad, although there are a lot of CVEs to go through:

\- A timing attack if you're using HTTP basic auth

\- A couple of GC related DoS attacks

\- An issue with `accepts_nested_attributes_for` if you're using both the
`allow_destroy` and `reject_if` options

\- A validation bypass exploit if you're calling
`SomeModel.new(params[:some_model])` instead of using StrongParams

\- An information leak exploit if you're calling `render params[:something]`
with raw user input

\- A bunch of potential XSS exploits

The `render` issue looks like it could cause the most harm, but hopefully
shouldn't be too prevalent. The XSS issues should be a quick fix as you only
have to update `rails-html-sanitizer`, not Rails itself.

~~~
ryanlol
>\- A timing attack if you're using HTTP basic auth

I'd say that qualifies as pretty bad.

How the hell does that even happen? Using time constant string comparison is
authentication 101. That's really not something you can mess up by mistake,
it's something you mess up by not understanding what you're doing. And that's
is all ignoring the fact that there's no reason to not use hashing here.

~~~
semiquaver
The vast majority of rails applications do not use HTTP basic authentication,
and I would guess that most of the ones that do use nginx or apache to provide
it. This was probably not caught until now because hardly anyone uses it.

~~~
ryanlol
I've seen a bunch of companies use rails HTTP basic auth internally.

And it's not that it wasn't caught until now, it's that it wasn't caught
before the commit was accepted.

~~~
eropple
I expect (though I have not looked) that it's old code that wasn't eyeballed
for security so much as for consistency and correctness. Once in, it doesn't
leave.

------
deanclatworthy
I see a timing attack in the list. It's fairly trivial to mitigate against
this in the majority of languages nowadays [1] [2] [3] etc..

I presume this can also be mitigated by implementing rate limiting on your
authentication endpoints, although that should also be implemented for other
reasons.

[1]
[https://golang.org/pkg/crypto/subtle/#ConstantTimeCompare](https://golang.org/pkg/crypto/subtle/#ConstantTimeCompare)

[2] [http://php.net/manual/en/function.hash-
equals.php](http://php.net/manual/en/function.hash-equals.php)

[3] [http://www.levigross.com/2014/02/07/constant-time-
comparison...](http://www.levigross.com/2014/02/07/constant-time-comparison-
functions-in...-python-haskell-clojure-and-java/)

~~~
thibaut_barrere
The current implementation is here:

[https://github.com/rails/rails/commit/859ca4474e1608b83d6194...](https://github.com/rails/rails/commit/859ca4474e1608b83d61941724574aba491be7f1)

~~~
koolba
That's still broken. They've just pushed the problem deeper. Now instead of
having a timing attack on the number of operations in the compare, the timing
attack is pushed to the number of bytes that is hashed by sha256. Also, this
opens up a new avenue in that now hash collisions (as unlikely as they may be)
would be considered equal.

~~~
zaroth
This is considered best practice for languages where you can't trust your
"constant time" comparison won't be optimized out from under you.

Performing a timing attack requires control of the bytes being compared. If
you can control the bytes of the _output_ of a SHA256 then there are some
Bitcoin miners who will pay you a lot of money.

If you want to be over-the-top about it you can get some secure randomness and
add it to the values being compared before hashing, and then attacker would
have even less control over the bytes being compared.

~~~
ryanlol
I suppose you could theoretically deduce a large enough part of the hash to
perform a bruteforce attack though.

You don't need that much of the hash to perform wordlist attacks and find
likely candidates.

~~~
zaroth
Yes, this is true if you are talking about unsalted passwords, or if the
attacker knows the salts. If there's an unknown salt, having the salted hash
of a given plaintext input match the first few bytes of the target salted hash
does not help you narrow the word list at all. If there is no salt, or the
salt is public, then that's a case where you could append some ephemeral CS-
PRNG output to both sides before hash-comparing... but probably better to fix
the underlying issue.

I mean, it's funny to be far enough down the rabbit hole to be doing hash-
compare. Then to say we wanted randomly keyed hash-compare is the final step.
The nice thing is adding some random bytes imposes a fairly miniscule
performance hit and it's purely computation, no additional storage. Probably
still too slow to use for _every_ comparison, but for constant-time critical
comparison, it literally can't hurt.

------
matdrewin
On one hand, I find that Rails often has security issues. On the other hand,
perhaps it is just indicative of its popularity and interest. When a framework
has no security issues, is it because there are none or is it just that no one
can be bothered to look for some?

~~~
elliotec
Excellent point. Also the fact is there will be people working on these
immediately.

------
dain
Aah Aaron. Thanks. Everywhere he codes he refactors, fixes performance issues,
finds bugs, he's so my hero.

~~~
VeejayRampay
tenderlove is the best. Not only does he work on both Ruby and Ruby on Rails,
he's also very active in the community (conferences, workshops, talks) and
working hard on promoting a spirit of humility and respect (see the humorous
Adequate HQ, the Friday Hug, the overwhelmingly positive and generous attitude
he's always had towards anyone, famous or not, in the Ruby community). I have
nothing but respect for that man and I'm glad he's working hard everyday on
the language I so love.

Thank you Aaron.

~~~
dain
Really really well said. So grateful to have him in our lovely community.

------
igravious
A quick `bundle update` appears to be just the ticket:

    
    
        …
        Installing rails-html-sanitizer 1.0.3 (was 1.0.2)
        Installing actionmailer 4.2.5.1 (was 4.2.5)
        Installing activemodel 4.2.5.1 (was 4.2.5)
        Installing activerecord 4.2.5.1 (was 4.2.5)
        Installing railties 4.2.5.1 (was 4.2.5)
        Installing rails 4.2.5.1 (was 4.2.5)
        …
        Bundle updated!

------
forced-request
CVE-2016-0752 is explained in more detail here:
[https://nvisium.com/blog/2016/01/26/rails-dynamic-render-
to-...](https://nvisium.com/blog/2016/01/26/rails-dynamic-render-to-rce-
cve-2016-0752/)

~~~
secretmike
Great overview, thanks!

------
tetraverse
What was the name of that still-in-development OS that is going to mitigate
against most forms of conventional attacks.

random quote: 'I used to consume cannabis on a daily basis, I suffer no short
term memory loss, as far as I can remember....'

