From 292e8d98453cd5a2b2cd1a9395e585e2a123e3a2 Mon Sep 17 00:00:00 2001 From: Michael McCabe Date: Mon, 9 Sep 2013 21:45:00 -0400 Subject: [PATCH 01/10] adding execjs and therubyracer to fix js issue on ubuntu --- Gemfile | 4 ++++ Gemfile.lock | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/Gemfile b/Gemfile index aec2b0a..a225617 100755 --- a/Gemfile +++ b/Gemfile @@ -63,3 +63,7 @@ gem 'minitest', '~> 4.0', :require=> "minitest/autorun" # To use debugger # gem 'debugger' + +gem 'execjs' +gem 'therubyracer' + diff --git a/Gemfile.lock b/Gemfile.lock index d0e774c..861ff3b 100755 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -115,6 +115,7 @@ GEM thor (>= 0.14, < 2.0) json (1.7.7) kgio (2.8.0) + libv8 (3.16.14.3) listen (0.7.3) lumberjack (1.0.3) mail (2.5.3) @@ -162,6 +163,7 @@ GEM rb-fsevent (0.9.3) rdoc (3.12.2) json (~> 1.4) + ref (1.0.5) rspec (2.14.1) rspec-core (~> 2.14.0) rspec-expectations (~> 2.14.0) @@ -200,6 +202,9 @@ GEM sqlite3 (1.3.7) temple (0.6.3) terminal-table (1.4.5) + therubyracer (0.12.0) + libv8 (~> 3.16.14.0) + ref thor (0.18.1) tilt (1.3.7) treetop (1.4.12) @@ -224,6 +229,7 @@ DEPENDENCIES brakeman bundler-audit coffee-rails (~> 3.2.1) + execjs foreman gauntlt guard-brakeman @@ -240,5 +246,6 @@ DEPENDENCIES rspec-rails sass-rails (~> 3.2.3) sqlite3 + therubyracer uglifier (>= 1.0.3) unicorn From 987b6d8844c5713c681baaeeda7390315c701f2c Mon Sep 17 00:00:00 2001 From: Michael McCabe Date: Tue, 10 Sep 2013 09:17:40 -0400 Subject: [PATCH 02/10] setting up travis ci env --- .travis.yml | 4 ++++ Gemfile | 1 + Gemfile.lock | 4 ++++ 3 files changed, 9 insertions(+) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..8c734ac --- /dev/null +++ b/.travis.yml @@ -0,0 +1,4 @@ +language: ruby +rvm: + - "1.9.3" +before_script: rake db:migrate diff --git a/Gemfile b/Gemfile index a225617..a242843 100755 --- a/Gemfile +++ b/Gemfile @@ -18,6 +18,7 @@ group :development do gem 'bundler-audit' gem 'guard-livereload' gem 'rack-livereload' + gem 'travis-lint' end gem 'gauntlt' diff --git a/Gemfile.lock b/Gemfile.lock index 861ff3b..47b8772 100755 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -102,6 +102,7 @@ GEM guard (>= 1.1.0) haml (4.0.2) tilt + hashr (0.0.22) highline (1.6.16) hike (1.2.2) http_parser.rb (0.5.3) @@ -207,6 +208,8 @@ GEM ref thor (0.18.1) tilt (1.3.7) + travis-lint (1.7.0) + hashr (~> 0.0.22) treetop (1.4.12) polyglot polyglot (>= 0.3.1) @@ -247,5 +250,6 @@ DEPENDENCIES sass-rails (~> 3.2.3) sqlite3 therubyracer + travis-lint uglifier (>= 1.0.3) unicorn From aab489ef4069162219c7e3643002aa5b947698bc Mon Sep 17 00:00:00 2001 From: cktricky Date: Tue, 10 Sep 2013 21:58:29 -0400 Subject: [PATCH 03/10] fix for performance bug --- app/models/performance.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/performance.rb b/app/models/performance.rb index eba7cf5..f6785b1 100644 --- a/app/models/performance.rb +++ b/app/models/performance.rb @@ -3,6 +3,7 @@ class Performance < ActiveRecord::Base belongs_to :user def reviewer_name - User.find_by_id(self.reviewer).full_name + u = User.find_by_id(self.reviewer) + u.full_name if u.respond_to?('fullname') end end From 20420be1a670d4d3c33bfd0c0689d8fa4b5d35e2 Mon Sep 17 00:00:00 2001 From: Chris Morris Date: Wed, 25 Sep 2013 16:56:34 -0500 Subject: [PATCH 04/10] 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. --- .gitignore | 18 ++---------------- Gemfile | 9 +++++---- Gemfile.lock | 1 + app/controllers/users_controller.rb | 2 +- 4 files changed, 9 insertions(+), 21 deletions(-) diff --git a/.gitignore b/.gitignore index 6e10082..c87e5f5 100755 --- a/.gitignore +++ b/.gitignore @@ -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 \ No newline at end of file +/public/data diff --git a/Gemfile b/Gemfile index 72031de..1f1f2d9 100755 --- a/Gemfile +++ b/Gemfile @@ -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 diff --git a/Gemfile.lock b/Gemfile.lock index e82bd83..27bae65 100755 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -237,6 +237,7 @@ DEPENDENCIES jquery-rails minitest (~> 4.0) powder + pry rack-livereload rails (= 3.2.13) rb-fsevent diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 09701a3..b52a43b 100755 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -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! From 8793ca8a883175d9d2e2d26829e35ef52b08a001 Mon Sep 17 00:00:00 2001 From: chrismo Date: Thu, 26 Sep 2013 10:31:11 -0500 Subject: [PATCH 05/10] Add .gitkeep on data folder so uploads work --- public/data/.gitkeep | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 public/data/.gitkeep diff --git a/public/data/.gitkeep b/public/data/.gitkeep new file mode 100644 index 0000000..e69de29 From df9efa915b453f13764104c3e8733e16f2cacce1 Mon Sep 17 00:00:00 2001 From: chrismo Date: Thu, 26 Sep 2013 17:50:30 -0500 Subject: [PATCH 06/10] Capybara added to demonstrate vulnerabilities. Adding Capybara to verify replay-ability of hacking vulnerabilities. I imagine these may want to be kept on a different branch for QA and educational purposes, but not distributed with master when forked. This commit also includes demonstrating the SQL Injection vulnerability. --- .gitignore | 1 + Gemfile | 5 +- Gemfile.lock | 21 ++- Gemfile.lock.orig | 255 ---------------------------- spec/features/sql_injection_spec.rb | 37 ++++ spec/spec_helper.rb | 17 +- 6 files changed, 77 insertions(+), 259 deletions(-) delete mode 100755 Gemfile.lock.orig create mode 100644 spec/features/sql_injection_spec.rb diff --git a/.gitignore b/.gitignore index c87e5f5..fef3086 100755 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,4 @@ .elasticbeanstalk/ .DS_Store /public/data +*.png \ No newline at end of file diff --git a/Gemfile b/Gemfile index ca2954a..b175ad7 100755 --- a/Gemfile +++ b/Gemfile @@ -25,6 +25,9 @@ end gem 'gauntlt' group :development, :test do + gem 'capybara' + gem 'database_cleaner' + gem 'poltergeist' gem 'rspec-rails' end @@ -56,7 +59,7 @@ gem 'jquery-rails' gem 'powder' gem 'aruba' -gem 'minitest', '~> 4.0', :require=> "minitest/autorun" +#gem 'minitest', '~> 4.0', :require=> "minitest/autorun" #gem 'minitest' diff --git a/Gemfile.lock b/Gemfile.lock index 7d1c71e..dc27f3d 100755 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -48,8 +48,15 @@ GEM builder (3.0.4) bundler-audit (0.1.2) bundler (~> 1.2) + capybara (2.1.0) + mime-types (>= 1.16) + nokogiri (>= 1.3.3) + rack (>= 1.0.0) + rack-test (>= 0.5.4) + xpath (~> 2.0) childprocess (0.3.9) ffi (~> 1.0, >= 1.0.11) + cliver (0.2.2) coderay (1.0.9) coffee-rails (3.2.2) coffee-script (>= 2.2.0) @@ -63,6 +70,7 @@ GEM diff-lcs (>= 1.1.3) gherkin (~> 2.12.0) multi_json (~> 1.3) + database_cleaner (1.1.1) diff-lcs (1.2.4) em-websocket (0.5.0) eventmachine (>= 0.12.9) @@ -125,9 +133,13 @@ GEM treetop (~> 1.4.8) method_source (0.8.1) mime-types (1.22) - minitest (4.7.5) multi_json (1.7.2) nokogiri (1.5.10) + poltergeist (1.4.1) + capybara (~> 2.1.0) + cliver (~> 0.2.1) + multi_json (~> 1.0) + websocket-driver (>= 0.2.0) polyglot (0.3.3) powder (0.2.0) thor (>= 0.11.5) @@ -222,6 +234,9 @@ GEM kgio (~> 2.6) rack raindrops (~> 0.7) + websocket-driver (0.3.0) + xpath (2.0.0) + nokogiri (~> 1.3) PLATFORMS ruby @@ -231,7 +246,9 @@ DEPENDENCIES bcrypt-ruby brakeman bundler-audit + capybara coffee-rails (~> 3.2.1) + database_cleaner execjs foreman gauntlt @@ -241,7 +258,7 @@ DEPENDENCIES guard-shell jquery-fileupload-rails jquery-rails - minitest (~> 4.0) + poltergeist powder pry rack-livereload diff --git a/Gemfile.lock.orig b/Gemfile.lock.orig deleted file mode 100755 index 47b8772..0000000 --- a/Gemfile.lock.orig +++ /dev/null @@ -1,255 +0,0 @@ -GEM - remote: https://rubygems.org/ - specs: - actionmailer (3.2.13) - actionpack (= 3.2.13) - mail (~> 2.5.3) - actionpack (3.2.13) - activemodel (= 3.2.13) - activesupport (= 3.2.13) - builder (~> 3.0.0) - erubis (~> 2.7.0) - journey (~> 1.0.4) - rack (~> 1.4.5) - rack-cache (~> 1.2) - rack-test (~> 0.6.1) - sprockets (~> 2.2.1) - activemodel (3.2.13) - activesupport (= 3.2.13) - builder (~> 3.0.0) - activerecord (3.2.13) - activemodel (= 3.2.13) - activesupport (= 3.2.13) - arel (~> 3.0.2) - tzinfo (~> 0.3.29) - activeresource (3.2.13) - activemodel (= 3.2.13) - activesupport (= 3.2.13) - activesupport (3.2.13) - i18n (= 0.6.1) - multi_json (~> 1.0) - arel (3.0.2) - aruba (0.5.3) - childprocess (>= 0.3.6) - cucumber (>= 1.1.1) - rspec-expectations (>= 2.7.0) - bcrypt-ruby (3.0.1) - brakeman (1.9.5) - erubis (~> 2.6) - fastercsv (~> 1.5) - haml (>= 3.0, < 5.0) - highline (~> 1.6) - multi_json (~> 1.2) - ruby2ruby (= 2.0.3) - ruby_parser (~> 3.1.1) - sass (~> 3.0) - slim (~> 1.3.6) - terminal-table (~> 1.4) - builder (3.0.4) - bundler-audit (0.1.2) - bundler (~> 1.2) - childprocess (0.3.9) - ffi (~> 1.0, >= 1.0.11) - coderay (1.0.9) - coffee-rails (3.2.2) - coffee-script (>= 2.2.0) - railties (~> 3.2.0) - coffee-script (2.2.0) - coffee-script-source - execjs - coffee-script-source (1.6.2) - cucumber (1.3.2) - builder (>= 2.1.2) - diff-lcs (>= 1.1.3) - gherkin (~> 2.12.0) - multi_json (~> 1.3) - diff-lcs (1.2.4) - em-websocket (0.5.0) - eventmachine (>= 0.12.9) - http_parser.rb (~> 0.5.3) - erubis (2.7.0) - eventmachine (1.0.3) - execjs (1.4.0) - multi_json (~> 1.0) - fastercsv (1.5.5) - ffi (1.9.0) - foreman (0.62.0) - thor (>= 0.13.6) - formatador (0.2.4) - gauntlt (1.0.5) - cucumber - nokogiri (~> 1.5.0) - trollop - gherkin (2.12.0) - multi_json (~> 1.3) - guard (1.7.0) - formatador (>= 0.2.4) - listen (>= 0.6.0) - lumberjack (>= 1.0.2) - pry (>= 0.9.10) - thor (>= 0.14.6) - guard-brakeman (0.6.3) - brakeman (>= 1.8.2) - guard (>= 1.1.0) - guard-livereload (1.3.0) - em-websocket (>= 0.2.0) - guard (>= 1.5.0) - multi_json (~> 1.0) - guard-rspec (2.5.4) - guard (>= 1.1) - rspec (~> 2.11) - guard-shell (0.5.1) - guard (>= 1.1.0) - haml (4.0.2) - tilt - hashr (0.0.22) - highline (1.6.16) - hike (1.2.2) - http_parser.rb (0.5.3) - i18n (0.6.1) - journey (1.0.4) - jquery-fileupload-rails (0.4.1) - actionpack (>= 3.1) - railties (>= 3.1) - jquery-rails (3.0.1) - railties (>= 3.0, < 5.0) - thor (>= 0.14, < 2.0) - json (1.7.7) - kgio (2.8.0) - libv8 (3.16.14.3) - listen (0.7.3) - lumberjack (1.0.3) - mail (2.5.3) - i18n (>= 0.4.0) - mime-types (~> 1.16) - treetop (~> 1.4.8) - method_source (0.8.1) - mime-types (1.22) - minitest (4.7.5) - multi_json (1.7.2) - nokogiri (1.5.10) - polyglot (0.3.3) - powder (0.2.0) - thor (>= 0.11.5) - pry (0.9.12) - coderay (~> 1.0.5) - method_source (~> 0.8) - slop (~> 3.4) - rack (1.4.5) - rack-cache (1.2) - rack (>= 0.4) - rack-livereload (0.3.15) - rack - rack-ssl (1.3.3) - rack - rack-test (0.6.2) - rack (>= 1.0) - rails (3.2.13) - actionmailer (= 3.2.13) - actionpack (= 3.2.13) - activerecord (= 3.2.13) - activeresource (= 3.2.13) - activesupport (= 3.2.13) - bundler (~> 1.0) - railties (= 3.2.13) - railties (3.2.13) - actionpack (= 3.2.13) - activesupport (= 3.2.13) - rack-ssl (~> 1.3.2) - rake (>= 0.8.7) - rdoc (~> 3.4) - thor (>= 0.14.6, < 2.0) - raindrops (0.10.0) - rake (10.0.4) - rb-fsevent (0.9.3) - rdoc (3.12.2) - json (~> 1.4) - ref (1.0.5) - rspec (2.14.1) - rspec-core (~> 2.14.0) - rspec-expectations (~> 2.14.0) - rspec-mocks (~> 2.14.0) - rspec-core (2.14.2) - rspec-expectations (2.14.0) - diff-lcs (>= 1.1.3, < 2.0) - rspec-mocks (2.14.1) - rspec-rails (2.14.0) - actionpack (>= 3.0) - activesupport (>= 3.0) - railties (>= 3.0) - rspec-core (~> 2.14.0) - rspec-expectations (~> 2.14.0) - rspec-mocks (~> 2.14.0) - ruby2ruby (2.0.3) - ruby_parser (~> 3.1) - sexp_processor (~> 4.0) - ruby_parser (3.1.3) - sexp_processor (~> 4.1) - sass (3.2.7) - sass-rails (3.2.6) - railties (~> 3.2.0) - sass (>= 3.1.10) - tilt (~> 1.3) - sexp_processor (4.2.1) - slim (1.3.8) - temple (~> 0.6.3) - tilt (~> 1.3.3) - slop (3.4.4) - sprockets (2.2.2) - hike (~> 1.2) - multi_json (~> 1.0) - rack (~> 1.0) - tilt (~> 1.1, != 1.3.0) - sqlite3 (1.3.7) - temple (0.6.3) - terminal-table (1.4.5) - therubyracer (0.12.0) - libv8 (~> 3.16.14.0) - ref - thor (0.18.1) - tilt (1.3.7) - travis-lint (1.7.0) - hashr (~> 0.0.22) - treetop (1.4.12) - polyglot - polyglot (>= 0.3.1) - trollop (2.0) - tzinfo (0.3.37) - uglifier (2.0.1) - execjs (>= 0.3.0) - multi_json (~> 1.0, >= 1.0.2) - unicorn (4.6.2) - kgio (~> 2.6) - rack - raindrops (~> 0.7) - -PLATFORMS - ruby - -DEPENDENCIES - aruba - bcrypt-ruby - brakeman - bundler-audit - coffee-rails (~> 3.2.1) - execjs - foreman - gauntlt - guard-brakeman - guard-livereload - guard-rspec - guard-shell - jquery-fileupload-rails - jquery-rails - minitest (~> 4.0) - powder - rack-livereload - rails (= 3.2.13) - rb-fsevent - rspec-rails - sass-rails (~> 3.2.3) - sqlite3 - therubyracer - travis-lint - uglifier (>= 1.0.3) - unicorn diff --git a/spec/features/sql_injection_spec.rb b/spec/features/sql_injection_spec.rb new file mode 100644 index 0000000..d689b12 --- /dev/null +++ b/spec/features/sql_injection_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +feature 'sql injection' do + before do + User.delete_all + Rails.application.load_seed + @normal_user = User.create!(:first_name => 'Joe', :last_name => 'Schmoe', + :email => 'joe@schmoe.com', :password => 'aoeuaoeu', :password_confirmation => 'aoeuaoeu') + @admin_user = User.where("admin='t'").first + end + + scenario 'injection attack on account_settings' do + @admin_user.admin.should be_true + + visit '/' + within('.signup') do + fill_in 'email', :with => 'joe@schmoe.com' + fill_in 'password', :with => 'aoeuaoeu' + end + click_on 'Login' + + visit "/users/#{@normal_user.user_id}/account_settings" + within('#account_edit') do + fill_in 'Email', :with => 'joe.admin@schmoe.com' + fill_in 'user_password', :with => 'hacketyhack' + fill_in 'user_password_confirmation', :with => 'hacketyhack' + + # this is a hidden field, so cannot use fill_in to access it. + find(:xpath, "//input[@id='user_user_id']", :visible => false).set "8' OR admin='t') --" + end + click_on 'Submit' + + @admin_user = User.where("admin='t'").first + @admin_user.email.should == 'joe.admin@schmoe.com' + @admin_user.admin.should == true + end +end \ No newline at end of file diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index d2cbea7..417153f 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -3,6 +3,9 @@ ENV["RAILS_ENV"] ||= 'test' require File.expand_path("../../config/environment", __FILE__) require 'rspec/rails' require 'rspec/autorun' +require 'capybara/rails' +require 'capybara/poltergeist' +require 'database_cleaner' # Requires supporting ruby files with custom matchers and macros, etc, # in spec/support/ and its subdirectories. @@ -23,7 +26,7 @@ RSpec.configure do |config| # If you're not using ActiveRecord, or you'd prefer not to run each of your # examples within a transaction, remove the following line or assign false # instead of true. - config.use_transactional_fixtures = true + config.use_transactional_fixtures = false # Capybara Poltergeist driver requires this # If true, the base class of anonymous controllers will be inferred # automatically. This will be the default behavior in future versions of @@ -35,4 +38,16 @@ RSpec.configure do |config| # the seed, which is printed after each run. # --seed 1234 config.order = "random" + + config.before(:each) do + DatabaseCleaner.start + end + + config.after(:each) do + DatabaseCleaner.clean + end end + +Capybara.javascript_driver = :poltergeist + +DatabaseCleaner.strategy = :truncation \ No newline at end of file From e0bca0139ef9e7a791600b13b3ea8cbb07f73c89 Mon Sep 17 00:00:00 2001 From: chrismo Date: Fri, 27 Sep 2013 14:59:30 -0500 Subject: [PATCH 07/10] Added command injection Capybara spec. --- app/controllers/users_controller.rb | 5 +--- app/models/user.rb | 11 +++++-- spec/features/command_injection_spec.rb | 39 +++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 6 deletions(-) create mode 100644 spec/features/command_injection_spec.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b52a43b..d5ae600 100755 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -9,10 +9,7 @@ class UsersController < ApplicationController def create user = User.new(params[:user]) - user.build_retirement(POPULATE_RETIREMENTS.shuffle.first) - user.build_paid_time_off(POPULATE_PAID_TIME_OFF.shuffle.first).schedule.build(POPULATE_SCHEDULE.shuffle.first) - user.build_work_info(POPULATE_WORK_INFO.shuffle.first) - user.performance.build(POPULATE_PERFORMANCE.shuffle.first) + user.build_benefits_data if user.save session[:user_id] = user.user_id redirect_to home_dashboard_index_path diff --git a/app/models/user.rb b/app/models/user.rb index a9ecca8..c4b7f64 100755 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -16,8 +16,15 @@ class User < ActiveRecord::Base has_one :paid_time_off, :foreign_key => :user_id, :primary_key => :user_id, :dependent => :destroy has_one :work_info, :foreign_key => :user_id, :primary_key => :user_id, :dependent => :destroy has_many :performance, :foreign_key => :user_id, :primary_key => :user_id, :dependent => :destroy - - + + + def build_benefits_data + build_retirement(POPULATE_RETIREMENTS.shuffle.first) + build_paid_time_off(POPULATE_PAID_TIME_OFF.shuffle.first).schedule.build(POPULATE_SCHEDULE.shuffle.first) + build_work_info(POPULATE_WORK_INFO.shuffle.first) + performance.build(POPULATE_PERFORMANCE.shuffle.first) + end + private def full_name diff --git a/spec/features/command_injection_spec.rb b/spec/features/command_injection_spec.rb new file mode 100644 index 0000000..186524a --- /dev/null +++ b/spec/features/command_injection_spec.rb @@ -0,0 +1,39 @@ +require 'spec_helper' +require 'tmpdir' + +feature 'command injection' do + before do + User.delete_all + Rails.application.load_seed + @normal_user = User.new(:first_name => 'Joe', :last_name => 'Schmoe', + :email => 'joe@schmoe.com', :password => 'aoeuaoeu', :password_confirmation => 'aoeuaoeu') + @normal_user.build_benefits_data + @normal_user.save! + end + + scenario 'injection attack on file upload', :js => true do + visit '/' + within('.signup') do + fill_in 'email', :with => 'joe@schmoe.com' + fill_in 'password', :with => 'aoeuaoeu' + end + click_on 'Login' + + legit_file = File.join(Rails.root, 'public', 'data', 'legit.txt') + File.open(legit_file, 'w') { |f| f.puts 'totes legit' } + + visit "/users/#{@normal_user.user_id}/benefit_forms" + Dir.mktmpdir do |dir| + hackety_file = File.join(dir, '; cd public && cd data && rm -f * ;') + File.open(hackety_file, 'w') { |f| f.print 'mwahaha' } + within('.new_benefits') do + attach_file 'benefits_upload', hackety_file + find(:xpath, "//input[@id='benefits_backup']", :visible => false).set 'true' + end + save_screenshot('screenshot.before.upload.png') + click_on 'Start Upload' + end + save_screenshot('screenshot.after.upload.png') + File.exists?(legit_file).should be_false + end +end \ No newline at end of file From 269d5a0075670ca356525647d859039e949f8b53 Mon Sep 17 00:00:00 2001 From: chrismo Date: Fri, 27 Sep 2013 16:58:33 -0500 Subject: [PATCH 08/10] XSS Capybara spec added. --- spec/features/command_injection_spec.rb | 15 +++---------- spec/features/sql_injection_spec.rb | 13 +++-------- spec/features/xss_spec.rb | 30 +++++++++++++++++++++++++ spec/support/capybara_shared.rb | 8 +++++++ spec/support/user_fixture.rb | 18 +++++++++++++++ 5 files changed, 62 insertions(+), 22 deletions(-) create mode 100644 spec/features/xss_spec.rb create mode 100644 spec/support/capybara_shared.rb create mode 100644 spec/support/user_fixture.rb diff --git a/spec/features/command_injection_spec.rb b/spec/features/command_injection_spec.rb index 186524a..e1ef311 100644 --- a/spec/features/command_injection_spec.rb +++ b/spec/features/command_injection_spec.rb @@ -3,21 +3,12 @@ require 'tmpdir' feature 'command injection' do before do - User.delete_all - Rails.application.load_seed - @normal_user = User.new(:first_name => 'Joe', :last_name => 'Schmoe', - :email => 'joe@schmoe.com', :password => 'aoeuaoeu', :password_confirmation => 'aoeuaoeu') - @normal_user.build_benefits_data - @normal_user.save! + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user end scenario 'injection attack on file upload', :js => true do - visit '/' - within('.signup') do - fill_in 'email', :with => 'joe@schmoe.com' - fill_in 'password', :with => 'aoeuaoeu' - end - click_on 'Login' + login(@normal_user) legit_file = File.join(Rails.root, 'public', 'data', 'legit.txt') File.open(legit_file, 'w') { |f| f.puts 'totes legit' } diff --git a/spec/features/sql_injection_spec.rb b/spec/features/sql_injection_spec.rb index d689b12..9553fc4 100644 --- a/spec/features/sql_injection_spec.rb +++ b/spec/features/sql_injection_spec.rb @@ -2,22 +2,15 @@ require 'spec_helper' feature 'sql injection' do before do - User.delete_all - Rails.application.load_seed - @normal_user = User.create!(:first_name => 'Joe', :last_name => 'Schmoe', - :email => 'joe@schmoe.com', :password => 'aoeuaoeu', :password_confirmation => 'aoeuaoeu') + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user @admin_user = User.where("admin='t'").first end scenario 'injection attack on account_settings' do @admin_user.admin.should be_true - visit '/' - within('.signup') do - fill_in 'email', :with => 'joe@schmoe.com' - fill_in 'password', :with => 'aoeuaoeu' - end - click_on 'Login' + login(@normal_user) visit "/users/#{@normal_user.user_id}/account_settings" within('#account_edit') do diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb new file mode 100644 index 0000000..39735f1 --- /dev/null +++ b/spec/features/xss_spec.rb @@ -0,0 +1,30 @@ +require 'spec_helper' + +feature 'xss' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'injection attack on account_settings', :js => true do + login @normal_user + + visit "/users/#{@normal_user.user_id}/account_settings" + within('#account_edit') do + fill_in 'First name', :with => "B" + + # password gets screwed up if you don't re-submit - need to fix + fill_in 'user_password', :with => @normal_user.clear_password + fill_in 'user_password_confirmation', :with => @normal_user.clear_password + end + click_on 'Submit' + save_screenshot('screenshot.post.submit.png') + + visit '/' + + find('form.button_to input.btn.btn-primary').value.should == 'RailsGoat h4x0r3d' + + # might be nice to demonstrate posting cookie contents or somesuch, but + # this at least shows the vulnerability still exists. + end +end \ No newline at end of file diff --git a/spec/support/capybara_shared.rb b/spec/support/capybara_shared.rb new file mode 100644 index 0000000..aeeb960 --- /dev/null +++ b/spec/support/capybara_shared.rb @@ -0,0 +1,8 @@ +def login(user) + visit '/' + within('.signup') do + fill_in 'email', :with => user.email + fill_in 'password', :with => user.clear_password + end + click_on 'Login' +end diff --git a/spec/support/user_fixture.rb b/spec/support/user_fixture.rb new file mode 100644 index 0000000..8a5f182 --- /dev/null +++ b/spec/support/user_fixture.rb @@ -0,0 +1,18 @@ +class UserFixture + def self.reset_all_users + User.delete_all + Rails.application.load_seed + end + + def self.normal_user + password = 'aoeuaoeu' + user = User.new(:first_name => 'Joe', :last_name => 'Schmoe', + :email => 'joe@schmoe.com', :password => password, :password_confirmation => password) + def user.clear_password + 'aoeuaoeu' + end + user.build_benefits_data + user.save! + user + end +end \ No newline at end of file From 1c8b6e9e1718c4a01b4e9aa31dcb0108cf6b95f5 Mon Sep 17 00:00:00 2001 From: chrismo Date: Fri, 27 Sep 2013 17:30:57 -0500 Subject: [PATCH 09/10] Broken Authorization specs added. --- spec/features/broken_auth_spec.rb | 25 +++++++++++++++++++++++++ spec/features/xss_spec.rb | 2 +- 2 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 spec/features/broken_auth_spec.rb diff --git a/spec/features/broken_auth_spec.rb b/spec/features/broken_auth_spec.rb new file mode 100644 index 0000000..d1f7e6e --- /dev/null +++ b/spec/features/broken_auth_spec.rb @@ -0,0 +1,25 @@ +require 'spec_helper' + +feature 'broken_auth' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'TMI during login', :js => true do + visit '/' + within('.signup') do + fill_in 'email', :with => @normal_user.email + 'not' + fill_in 'password', :with => @normal_user.clear_password + end + click_on 'Login' + find('div#flash_notice').text.should == "#{@normal_user.email}not doesn't exist!" + + within('.signup') do + fill_in 'email', :with => @normal_user.email + fill_in 'password', :with => @normal_user.clear_password + 'not' + end + click_on 'Login' + find('div#flash_notice').text.should == 'Incorrect Password!' + end +end \ No newline at end of file diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb index 39735f1..a5bea9f 100644 --- a/spec/features/xss_spec.rb +++ b/spec/features/xss_spec.rb @@ -6,7 +6,7 @@ feature 'xss' do @normal_user = UserFixture.normal_user end - scenario 'injection attack on account_settings', :js => true do + scenario 'xss attack on account_settings', :js => true do login @normal_user visit "/users/#{@normal_user.user_id}/account_settings" From 8e238e1d81224d1bca21899778ac133075e662f1 Mon Sep 17 00:00:00 2001 From: chrismo Date: Fri, 27 Sep 2013 18:05:45 -0500 Subject: [PATCH 10/10] Insecure Direct Object Reference spec added. This includes two scenarios - the work_info one mentioned in the tutorials, but also one allowing downloading of source code, which may belong somewhere else as I haven't worked through all the tutorials yet. --- spec/features/insecure_dor_spec.rb | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 spec/features/insecure_dor_spec.rb diff --git a/spec/features/insecure_dor_spec.rb b/spec/features/insecure_dor_spec.rb new file mode 100644 index 0000000..b0ac570 --- /dev/null +++ b/spec/features/insecure_dor_spec.rb @@ -0,0 +1,29 @@ +require 'spec_helper' + +feature 'insecure direct object reference' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'download production configuration' do + login(@normal_user) + + visit "/users/#{@normal_user.user_id}/benefit_forms" + download_url = first('.widget-body a')[:href] + visit download_url.sub(/name=(.*?)&/, 'name=../../config/database.yml&') + + page.status_code.should == 200 + page.response_headers['Content-Disposition'].should include('database.yml') + page.response_headers['Content-Length'].should == '576' + end + + scenario 'view any user work_info' do + login(@normal_user) + + @normal_user.user_id.should_not == 2 + visit '/users/2/work_info' + + first('td').text.should == 'Jack Mannino' + end +end \ No newline at end of file