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.
This commit is contained in:
Chris Morris
2013-09-25 16:56:34 -05:00
parent aab489ef40
commit 20420be1a6
4 changed files with 9 additions and 21 deletions
+2 -16
View File
@@ -1,22 +1,8 @@
# See http://help.github.com/ignore-files/ for more about ignoring files.
#
# If you find yourself ignoring temporary files generated by your text editor
# or operating system, you probably want to add a global ignore instead:
# git config --global core.excludesfile ~/.gitignore_global
# Ignore bundler config
/.bundle
# Ignore the default SQLite database.
/bin
/db/*.sqlite3
# Ignore all logfiles and tempfiles.
/log/*.log
/tmp
.elasticbeanstalk/
# Ignore Mac folder settings
.DS_Store
# Ignore data directory
/public/data
/public/data
+5 -4
View File
@@ -11,13 +11,14 @@ gem 'foreman'
group :development do
gem 'brakeman'
gem 'guard-brakeman'
gem 'guard-rspec'
gem 'rb-fsevent'
gem 'guard-shell'
gem 'bundler-audit'
gem 'guard-brakeman'
gem 'guard-livereload'
gem 'guard-rspec'
gem 'guard-shell'
gem 'pry'
gem 'rack-livereload'
gem 'rb-fsevent'
gem 'travis-lint'
end
+1
View File
@@ -237,6 +237,7 @@ DEPENDENCIES
jquery-rails
minitest (~> 4.0)
powder
pry
rack-livereload
rails (= 3.2.13)
rb-fsevent
+1 -1
View File
@@ -36,7 +36,7 @@ class UsersController < ApplicationController
user = User.find(:first, :conditions => "user_id = '#{params[:user][:user_id]}'")
user.skip_user_id_assign = true
user.update_attributes(params[:user].reject { |k| k == ("password" || "password_confirmation") || "user_id" })
user.update_attributes(params[:user].reject { |k| %w(password password_confirmation user_id).include? k })
pass = params[:user][:password]
user.password = pass if !(pass.blank?)
message = true if user.save!