OWASP/railsgoat — pull request #45 by chrismo (2013-09-25 22:11:28+00:00)
Title: Fixed logic to strip out user params.

Disclaimer: changes like these in this sort of app are tricky because
it's harder to presume the intention of the code in question.

The prior line:

```
user.update_attributes(params[:user].reject { |k| k == ("password" || "password_confirmation") || "user_id" })
```

returns an empty hash, because of the way the block evaluates:

```
irb(main):002:0> k = 'foo'
=> "foo"
irb(main):003:0> k == ("password" || "password_confirmation") || "user_id"
=> "user_id"
```

Before the last change to that line, without 'user_id' outside the
params, it didn't evaluate properly either:

```
irb(main):007:0> k = 'password_confirmation'
=> "password_confirmation"
irb(main):008:0> k == ("password" || "password_confirmation")
=> false
irb(main):009:0> ("password" || "password_confirmation")
=> "password"
```

So, in the normal use case for this form, you can't update any other
attribute of the User. To me, that's probably the best argument for
making this change, but it does simplify the SQL Injection attack
(although perhaps the prior complication was intended).

Before this change, injecting conditional SQL into the user_id param in
the account_settings update call would allow the password of whatever
account is found (e.g. the first one if injecting 'OR 1=1') to be reset,
but without additional attacks, the email address of that account is not
known.

After this change, the email address of that account now is also updated
in addition to the password, making it simpler to get in as an admin --
though you're still presuming the first account to be an admin.