From da061c79b6e19e20119a04461f964005d16d0895 Mon Sep 17 00:00:00 2001 From: cktricky Date: Mon, 30 Sep 2013 13:03:03 -0400 Subject: [PATCH 01/30] intended to remove some of the weirdness when updating a users account. A blank password basically ends up causing the previously existing password to be hashed twice. Probably move to has_secure_password at some point although that may end up screwing up the intent of the particular tutorial item --- app/controllers/users_controller.rb | 7 +++++-- app/models/user.rb | 16 ++++++++++------ db/seeds.rb | 7 ++++++- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index d5ae600..ce51404 100755 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -33,9 +33,12 @@ class UsersController < ApplicationController user = User.find(:first, :conditions => "user_id = '#{params[:user][:user_id]}'") user.skip_user_id_assign = true + user.skip_hash_password = true 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?) + if !(params[:user][:password].empty?) && (params[:user][:password] == params[:user][:password_confirmation]) + user.skip_hash_password = false + user.password = params[:user][:password] + end message = true if user.save! respond_to do |format| format.html { redirect_to user_account_settings_path(:user_id => current_user.user_id) } diff --git a/app/models/user.rb b/app/models/user.rb index c4b7f64..0e982e2 100755 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,15 +1,16 @@ class User < ActiveRecord::Base - attr_accessible :email, :password, :admin, :password_confirmation, :first_name, :last_name - validates_confirmation_of :password, :password_confirmation, :on => :create + attr_accessible :email, :admin, :first_name, :last_name, :user_id, :password, :password_confirmation validates :password, :presence => true, :confirmation => true, :length => {:within => 6..40}, - :on => :create#, + :on => :create, + :if => :password#, #:format => {:with => /\A.*(?=.{10,})(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[\@\#\$\%\^\&\+\=]).*\z/} validates_presence_of :email validates_uniqueness_of :email validates_format_of :email, :with => /.+@.+\..+/i attr_accessor :skip_user_id_assign + attr_accessor :skip_hash_password before_save :assign_user_id, :on => :create before_save :hash_password has_one :retirement, :foreign_key => :user_id, :primary_key => :user_id, :dependent => :destroy @@ -18,6 +19,7 @@ class User < ActiveRecord::Base 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) @@ -44,7 +46,7 @@ class User < ActiveRecord::Base raise "#{email} doesn't exist!" end return auth - end + end def assign_user_id unless @skip_user_id_assign.present? || self.user_id.present? @@ -55,8 +57,10 @@ class User < ActiveRecord::Base end def hash_password - if self.password.present? - self.password = Digest::MD5.hexdigest(password) + unless @skip_hash_password == true + if password.present? + self.password = Digest::MD5.hexdigest(password) + end end end diff --git a/db/seeds.rb b/db/seeds.rb index 9750d82..13cd4ae 100755 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -7,6 +7,7 @@ users = [ :email => "admin@metacorp.com", :admin => true, :password => "admin1234", + :password_confirmation => "admin1234", :first_name => "Admin", :last_name => "", :user_id =>1 @@ -15,6 +16,7 @@ users = [ :email => "jack@metacorp.com", :admin => false, :password => "yankeessuck", + :password_confirmation => "yankeessuck", :first_name => "Jack", :last_name => "Mannino", :user_id => 2 @@ -23,6 +25,7 @@ users = [ :email => "jim@metacorp.com", :admin => false, :password => "alohaowasp", + :password_confirmation => "alohaowasp", :first_name => "Jim", :last_name => "Manico", :user_id =>3 @@ -31,6 +34,7 @@ users = [ :email => "mike@metacorp.com", :admin => false, :password => "motorcross1445", + :password_confirmation => "motorcross1445", :first_name => "Mike", :last_name => "McCabe", :user_id =>4 @@ -39,6 +43,7 @@ users = [ :email => "ken@metacorp.com", :admin => false, :password => "citrusblend", + :password_confirmation => "citrusblend", :first_name => "Ken", :last_name => "Johnson", :user_id =>5 @@ -233,7 +238,7 @@ paid_time_off = [ users.each do |user_info| - user = User.new(user_info.reject {|k| k == :user_id}) + user = User.new(user_info.reject {|k| k == :user_id }) user.user_id = user_info[:user_id] user.save end From 0df6735b534b7ff9b43c74f1e758b67e02473be0 Mon Sep 17 00:00:00 2001 From: chrismo Date: Mon, 30 Sep 2013 15:29:36 -0500 Subject: [PATCH 02/30] Added example of CSRF vulnerability in csrf_spec. --- spec/features/command_injection_spec.rb | 2 -- spec/features/csrf_spec.rb | 44 +++++++++++++++++++++++++ spec/features/xss_spec.rb | 1 - 3 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 spec/features/csrf_spec.rb diff --git a/spec/features/command_injection_spec.rb b/spec/features/command_injection_spec.rb index e1ef311..d2b8a55 100644 --- a/spec/features/command_injection_spec.rb +++ b/spec/features/command_injection_spec.rb @@ -21,10 +21,8 @@ feature 'command injection' 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 diff --git a/spec/features/csrf_spec.rb b/spec/features/csrf_spec.rb new file mode 100644 index 0000000..b088291 --- /dev/null +++ b/spec/features/csrf_spec.rb @@ -0,0 +1,44 @@ +require 'spec_helper' +require 'tmpdir' + +feature 'csrf' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'csrf attack to pto', :js => true do + visit '/' + # TODO: is there a way to get this without visiting root first? + base_url = current_url + + login @normal_user + + Dir.mktmpdir do |dir| + hackety_file = File.join(dir, 'form.on.bad.guy.site.html') + post_url = "#{base_url}schedule.json" + File.open(hackety_file, 'w') do |f| + f.print <<-HTML + + +
+ + + + + +
+ + + HTML + end + + page.driver.visit "file://#{hackety_file}" + within('#submit_me') do + click_on 'Submit request' + end + end + + @normal_user.reload.paid_time_off.schedule.last.event_name.should == 'Bad Guy' + end +end \ No newline at end of file diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb index a5bea9f..f7c4694 100644 --- a/spec/features/xss_spec.rb +++ b/spec/features/xss_spec.rb @@ -18,7 +18,6 @@ feature 'xss' do fill_in 'user_password_confirmation', :with => @normal_user.clear_password end click_on 'Submit' - save_screenshot('screenshot.post.submit.png') visit '/' From 4f1526e0210f217615fe20002442bd9d1301da2a Mon Sep 17 00:00:00 2001 From: chrismo Date: Tue, 1 Oct 2013 16:06:21 -0500 Subject: [PATCH 03/30] URL access spec added --- spec/features/url_access_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 spec/features/url_access_spec.rb diff --git a/spec/features/url_access_spec.rb b/spec/features/url_access_spec.rb new file mode 100644 index 0000000..7e2992d --- /dev/null +++ b/spec/features/url_access_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' + +feature 'url access' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'admin route not protected', :js => true do + login @normal_user + + visit '/admin/1/dashboard' + current_path.should == '/admin/1/dashboard' + end +end \ No newline at end of file From 0021ddd0368b78ae43ed95694acc5e5e88ea9701 Mon Sep 17 00:00:00 2001 From: chrismo Date: Tue, 1 Oct 2013 16:20:15 -0500 Subject: [PATCH 04/30] Unvalidated redirect spec added --- spec/features/unvalidated_redirects_spec.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 spec/features/unvalidated_redirects_spec.rb diff --git a/spec/features/unvalidated_redirects_spec.rb b/spec/features/unvalidated_redirects_spec.rb new file mode 100644 index 0000000..8b52f7e --- /dev/null +++ b/spec/features/unvalidated_redirects_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +feature 'unvalidated redirect' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'login redirects to anywhere', :js => true do + visit '/?url=http://example.com/do/evil/things' + within('.signup') do + fill_in 'email', :with => @normal_user.email + fill_in 'password', :with => @normal_user.clear_password + end + click_on 'Login' + + current_url.should == 'http://example.com/do/evil/things' + end +end \ No newline at end of file From 85b0c7608b2dad6ce5c38a4eda9505a208f8a88c Mon Sep 17 00:00:00 2001 From: chrismo Date: Tue, 1 Oct 2013 16:47:06 -0500 Subject: [PATCH 05/30] Info disclosure spec added --- spec/features/info_disclosure_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 spec/features/info_disclosure_spec.rb diff --git a/spec/features/info_disclosure_spec.rb b/spec/features/info_disclosure_spec.rb new file mode 100644 index 0000000..7ecee45 --- /dev/null +++ b/spec/features/info_disclosure_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +feature 'sensitive information disclosure' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + @normal_user.work_info.update_attribute(:SSN, '999-99-9999') + end + + # this won't work with javascript_driver, as it'll apply the javascript + # function to mask this value and the source will be overwritten. + scenario 'full ssn returned to view' do + login @normal_user + + visit "/users/#{@normal_user.user_id}/work_info" + page.source.should include '999-99-9999' + end +end \ No newline at end of file From b1a388249641acd2f72e4425b45c5adacddefb86 Mon Sep 17 00:00:00 2001 From: chrismo Date: Tue, 1 Oct 2013 17:14:21 -0500 Subject: [PATCH 06/30] Mass assignment spec added --- .rspec | 1 + spec/features/mass_assignment_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 spec/features/mass_assignment_spec.rb diff --git a/.rspec b/.rspec index 4e1e0d2..9fc14ad 100644 --- a/.rspec +++ b/.rspec @@ -1 +1,2 @@ --color +--backtrace \ No newline at end of file diff --git a/spec/features/mass_assignment_spec.rb b/spec/features/mass_assignment_spec.rb new file mode 100644 index 0000000..a818656 --- /dev/null +++ b/spec/features/mass_assignment_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' + +feature 'sql injection' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'mass assignment attack on account_settings' do + @normal_user.admin.should be_false + + login(@normal_user) + + params = {:user => {:admin => 't', + :user_id => @normal_user.user_id, + :password => @normal_user.clear_password, + :password_confirmation => @normal_user.clear_password}} + page.driver.put "/users/#{@normal_user.user_id}.json", params + + @normal_user.reload.admin.should be_true + end +end \ No newline at end of file From 911a52ee83e104222f43d1059921935ab224f3c4 Mon Sep 17 00:00:00 2001 From: chrismo Date: Tue, 1 Oct 2013 23:26:28 -0500 Subject: [PATCH 07/30] Add pending code to flip-flop results of specs. This isn't the cleanest approach, but should be good for now. Obviously, there are two contexts for these specs: one is from the maintainer's standpoint, the other is from the trainee who is using RailsGoat for training. The maintainer wants all of these specs to pass, to ensure the vulnerabilities are still functional as vulnerabilities. The trainee could potentially use these specs (though reading the specs contains spoilers) to track and verify their fixes. I've wired in a pending block around each assertion that checks a method to see what the result of the pending call would be. You can see examples of how this works with conditions here: https://www.relishapp.com/rspec/rspec-core/v/2-14/docs/pending/pending-examples This means these specs will all fail now by default (the trainee context), but will pass, when vulnerable, if the RAILSGOAT_MAINTAINER env var is set. The only flaw at the moment is that in the trainee context, fixing the vulnerabilities will result in the specs going from failing to _pending_, not passing (which makes sense, given how we're using RSpec's pending functionality). Maybe it'd be simpler/better to have a boolean toggle of our own somehow wrap the assertions in blocks to do explicitly what we want (flip-flop the result based on the context). --- spec/features/broken_auth_spec.rb | 9 ++++++--- spec/features/command_injection_spec.rb | 4 ++-- spec/features/csrf_spec.rb | 2 +- spec/features/info_disclosure_spec.rb | 2 +- spec/features/insecure_dor_spec.rb | 10 ++++++---- spec/features/mass_assignment_spec.rb | 21 ++++++++++++++++++--- spec/features/sql_injection_spec.rb | 8 +++++--- spec/features/unvalidated_redirects_spec.rb | 2 +- spec/features/url_access_spec.rb | 2 +- spec/features/xss_spec.rb | 2 +- spec/support/capybara_shared.rb | 11 +++++++++++ 11 files changed, 53 insertions(+), 20 deletions(-) diff --git a/spec/features/broken_auth_spec.rb b/spec/features/broken_auth_spec.rb index d1f7e6e..4c83de0 100644 --- a/spec/features/broken_auth_spec.rb +++ b/spec/features/broken_auth_spec.rb @@ -6,20 +6,23 @@ feature 'broken_auth' do @normal_user = UserFixture.normal_user end - scenario 'TMI during login', :js => true do + scenario 'TMI during login - username' 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!" + pending(:if => verifying_fixed?) { find('div#flash_notice').text.should == "#{@normal_user.email}not doesn't exist!" } + end + scenario 'TMI during login - password' do + visit '/' 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!' + pending(:if => verifying_fixed?) { find('div#flash_notice').text.should == 'Incorrect Password!' } end end \ No newline at end of file diff --git a/spec/features/command_injection_spec.rb b/spec/features/command_injection_spec.rb index d2b8a55..468e92d 100644 --- a/spec/features/command_injection_spec.rb +++ b/spec/features/command_injection_spec.rb @@ -8,7 +8,7 @@ feature 'command injection' do end scenario 'injection attack on file upload', :js => true do - login(@normal_user) + login @normal_user legit_file = File.join(Rails.root, 'public', 'data', 'legit.txt') File.open(legit_file, 'w') { |f| f.puts 'totes legit' } @@ -23,6 +23,6 @@ feature 'command injection' do end click_on 'Start Upload' end - File.exists?(legit_file).should be_false + pending(:if => verifying_fixed?) { File.exists?(legit_file).should be_false } end end \ No newline at end of file diff --git a/spec/features/csrf_spec.rb b/spec/features/csrf_spec.rb index b088291..b3e56fb 100644 --- a/spec/features/csrf_spec.rb +++ b/spec/features/csrf_spec.rb @@ -39,6 +39,6 @@ feature 'csrf' do end end - @normal_user.reload.paid_time_off.schedule.last.event_name.should == 'Bad Guy' + pending(:if => verifying_fixed?) { @normal_user.reload.paid_time_off.schedule.last.event_name.should == 'Bad Guy' } end end \ No newline at end of file diff --git a/spec/features/info_disclosure_spec.rb b/spec/features/info_disclosure_spec.rb index 7ecee45..cc93282 100644 --- a/spec/features/info_disclosure_spec.rb +++ b/spec/features/info_disclosure_spec.rb @@ -13,6 +13,6 @@ feature 'sensitive information disclosure' do login @normal_user visit "/users/#{@normal_user.user_id}/work_info" - page.source.should include '999-99-9999' + pending(:if => verifying_fixed?) { page.source.should include '999-99-9999' } end end \ No newline at end of file diff --git a/spec/features/insecure_dor_spec.rb b/spec/features/insecure_dor_spec.rb index b0ac570..ce089e2 100644 --- a/spec/features/insecure_dor_spec.rb +++ b/spec/features/insecure_dor_spec.rb @@ -13,9 +13,11 @@ feature 'insecure direct object reference' do 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' + pending(:if => verifying_fixed?) { + 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 @@ -24,6 +26,6 @@ feature 'insecure direct object reference' do @normal_user.user_id.should_not == 2 visit '/users/2/work_info' - first('td').text.should == 'Jack Mannino' + pending(:if => verifying_fixed?) { first('td').text.should == 'Jack Mannino' } end end \ No newline at end of file diff --git a/spec/features/mass_assignment_spec.rb b/spec/features/mass_assignment_spec.rb index a818656..0e89b65 100644 --- a/spec/features/mass_assignment_spec.rb +++ b/spec/features/mass_assignment_spec.rb @@ -6,7 +6,7 @@ feature 'sql injection' do @normal_user = UserFixture.normal_user end - scenario 'mass assignment attack on account_settings' do + scenario 'mass assignment attack update account_settings' do @normal_user.admin.should be_false login(@normal_user) @@ -17,6 +17,21 @@ feature 'sql injection' do :password_confirmation => @normal_user.clear_password}} page.driver.put "/users/#{@normal_user.user_id}.json", params - @normal_user.reload.admin.should be_true + pending(:if => verifying_fixed?) { @normal_user.reload.admin.should be_true } end -end \ No newline at end of file + + scenario 'mass assignment attack create new account' do + params = {:user => {:admin => 't', + :email => 'hackety@h4x0rs.c0m', + :first_name => 'hackety', + :last_name => 'hax', + :password => 'foobarewe', + :password_confirmation => 'foobarewe'}} + page.driver.post '/users', params + + pending(:if => verifying_fixed?) { + User.last.email.should == 'hackety@h4x0rs.c0m' + User.last.admin.should be_true + } + end +end diff --git a/spec/features/sql_injection_spec.rb b/spec/features/sql_injection_spec.rb index 9553fc4..45a2800 100644 --- a/spec/features/sql_injection_spec.rb +++ b/spec/features/sql_injection_spec.rb @@ -23,8 +23,10 @@ feature 'sql injection' do end click_on 'Submit' - @admin_user = User.where("admin='t'").first - @admin_user.email.should == 'joe.admin@schmoe.com' - @admin_user.admin.should == true + pending(:if => verifying_fixed?) { + @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/features/unvalidated_redirects_spec.rb b/spec/features/unvalidated_redirects_spec.rb index 8b52f7e..8db28e8 100644 --- a/spec/features/unvalidated_redirects_spec.rb +++ b/spec/features/unvalidated_redirects_spec.rb @@ -14,6 +14,6 @@ feature 'unvalidated redirect' do end click_on 'Login' - current_url.should == 'http://example.com/do/evil/things' + pending(:if => verifying_fixed?) { current_url.should == 'http://example.com/do/evil/things' } end end \ No newline at end of file diff --git a/spec/features/url_access_spec.rb b/spec/features/url_access_spec.rb index 7e2992d..f33f858 100644 --- a/spec/features/url_access_spec.rb +++ b/spec/features/url_access_spec.rb @@ -10,6 +10,6 @@ feature 'url access' do login @normal_user visit '/admin/1/dashboard' - current_path.should == '/admin/1/dashboard' + pending(:if => verifying_fixed?) { current_path.should == '/admin/1/dashboard' } end end \ No newline at end of file diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb index f7c4694..264eaaf 100644 --- a/spec/features/xss_spec.rb +++ b/spec/features/xss_spec.rb @@ -21,7 +21,7 @@ feature 'xss' do visit '/' - find('form.button_to input.btn.btn-primary').value.should == 'RailsGoat h4x0r3d' + pending(:if => verifying_fixed?) { 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. diff --git a/spec/support/capybara_shared.rb b/spec/support/capybara_shared.rb index aeeb960..55cd9d3 100644 --- a/spec/support/capybara_shared.rb +++ b/spec/support/capybara_shared.rb @@ -1,3 +1,14 @@ +# By default this will return true, and thus all of the Capybara specs will +# fail until a developer using the site for training has patched up all of +# the vulnerabilities. +# +# However, RailsGoat maintainers need the Capybara features to pass to indicate +# changes to the site have not inadvertently removed or fixed any vulnerabilities +# since the whole point is to provide a site for a developer to fix. +def verifying_fixed? + !ENV['RAILSGOAT_MAINTAINER'] +end + def login(user) visit '/' within('.signup') do From 4ccdca89840fe85be5993c2e4d92d5d49fcb8f6e Mon Sep 17 00:00:00 2001 From: chrismo Date: Wed, 2 Oct 2013 17:53:12 -0500 Subject: [PATCH 08/30] Fixed model specs, some of which I broke. There's a fight here between DatabaseCleaner strategies - simpler to use the default :transaction for model specs, but Capybara lives in a different world where different connections are in play and :transactions don't work. So, while introducing the more cumbersome (though with more control) DatabaseCleaner gem and its truncation strategy, I forgot to make sure the model specs had the fixtures present that they depend on. This is fixed up now. The user spec for invalid email was also failing - the regex there is not savvy enough to handle rejecting two @ signs, so I made the invalid value something still invalid to get it passing -- real regex validation of email is ... impossible, so we'll roll with this and move on. --- spec/models/benefits_spec.rb | 1 - spec/models/paid_time_off_spec.rb | 14 -------------- spec/models/user_spec.rb | 11 ++++++++++- 3 files changed, 10 insertions(+), 16 deletions(-) delete mode 100644 spec/models/benefits_spec.rb delete mode 100644 spec/models/paid_time_off_spec.rb diff --git a/spec/models/benefits_spec.rb b/spec/models/benefits_spec.rb deleted file mode 100644 index f8ec369..0000000 --- a/spec/models/benefits_spec.rb +++ /dev/null @@ -1 +0,0 @@ -require 'spec_helper' diff --git a/spec/models/paid_time_off_spec.rb b/spec/models/paid_time_off_spec.rb deleted file mode 100644 index 2dba717..0000000 --- a/spec/models/paid_time_off_spec.rb +++ /dev/null @@ -1,14 +0,0 @@ -require 'spec_helper.rb' -=begin -describe "PaidTimeOff" do - user = User.new( - first_name: 'Tester', - last_name: 'MGee', - email: 'tester.mgee@gmail.com', - password: 'password', - password_confirmation: 'password' - ) - expect(user).to be_valid -end - -=end \ No newline at end of file diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 759d850..d521142 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1,6 +1,15 @@ require 'spec_helper.rb' describe User do + before(:all) do + UserFixture.reset_all_users + DatabaseCleaner.strategy = :transaction + end + + after(:all) do + DatabaseCleaner.strategy = :truncation + end + it "can be instantiated" do User.new.should be_an_instance_of(User) end @@ -10,7 +19,7 @@ describe User do end it "should require valid email" do - User.new(:email => "tester@gmail.com@gmail.com").should_not be_valid + User.new(:email => "@gmail.com").should_not be_valid end it "should require unique email" do From 525dfa1717e951c72bf540878e67b0487cf7be33 Mon Sep 17 00:00:00 2001 From: chrismo Date: Thu, 3 Oct 2013 11:00:43 -0500 Subject: [PATCH 09/30] Added notice and removed spoilers from spec names. --- spec/features/broken_auth_spec.rb | 4 +-- spec/features/command_injection_spec.rb | 2 +- spec/features/csrf_spec.rb | 2 +- spec/features/info_disclosure_spec.rb | 2 +- spec/features/insecure_dor_spec.rb | 4 +-- spec/features/mass_assignment_spec.rb | 6 ++--- spec/features/sql_injection_spec.rb | 2 +- spec/features/unvalidated_redirects_spec.rb | 2 +- spec/features/url_access_spec.rb | 2 +- spec/features/xss_spec.rb | 2 +- spec/support/capybara_shared.rb | 28 ++++++++++++++++++++- 11 files changed, 41 insertions(+), 15 deletions(-) diff --git a/spec/features/broken_auth_spec.rb b/spec/features/broken_auth_spec.rb index 4c83de0..e3548a6 100644 --- a/spec/features/broken_auth_spec.rb +++ b/spec/features/broken_auth_spec.rb @@ -6,7 +6,7 @@ feature 'broken_auth' do @normal_user = UserFixture.normal_user end - scenario 'TMI during login - username' do + scenario 'one' do visit '/' within('.signup') do fill_in 'email', :with => @normal_user.email + 'not' @@ -16,7 +16,7 @@ feature 'broken_auth' do pending(:if => verifying_fixed?) { find('div#flash_notice').text.should == "#{@normal_user.email}not doesn't exist!" } end - scenario 'TMI during login - password' do + scenario 'two' do visit '/' within('.signup') do fill_in 'email', :with => @normal_user.email diff --git a/spec/features/command_injection_spec.rb b/spec/features/command_injection_spec.rb index 468e92d..9b4ad85 100644 --- a/spec/features/command_injection_spec.rb +++ b/spec/features/command_injection_spec.rb @@ -7,7 +7,7 @@ feature 'command injection' do @normal_user = UserFixture.normal_user end - scenario 'injection attack on file upload', :js => true do + scenario 'attack', :js => true do login @normal_user legit_file = File.join(Rails.root, 'public', 'data', 'legit.txt') diff --git a/spec/features/csrf_spec.rb b/spec/features/csrf_spec.rb index b3e56fb..8301a48 100644 --- a/spec/features/csrf_spec.rb +++ b/spec/features/csrf_spec.rb @@ -7,7 +7,7 @@ feature 'csrf' do @normal_user = UserFixture.normal_user end - scenario 'csrf attack to pto', :js => true do + scenario 'attack', :js => true do visit '/' # TODO: is there a way to get this without visiting root first? base_url = current_url diff --git a/spec/features/info_disclosure_spec.rb b/spec/features/info_disclosure_spec.rb index cc93282..ce0bd2a 100644 --- a/spec/features/info_disclosure_spec.rb +++ b/spec/features/info_disclosure_spec.rb @@ -9,7 +9,7 @@ feature 'sensitive information disclosure' do # this won't work with javascript_driver, as it'll apply the javascript # function to mask this value and the source will be overwritten. - scenario 'full ssn returned to view' do + scenario 'attack' do login @normal_user visit "/users/#{@normal_user.user_id}/work_info" diff --git a/spec/features/insecure_dor_spec.rb b/spec/features/insecure_dor_spec.rb index ce089e2..aada5eb 100644 --- a/spec/features/insecure_dor_spec.rb +++ b/spec/features/insecure_dor_spec.rb @@ -6,7 +6,7 @@ feature 'insecure direct object reference' do @normal_user = UserFixture.normal_user end - scenario 'download production configuration' do + scenario 'attack one' do login(@normal_user) visit "/users/#{@normal_user.user_id}/benefit_forms" @@ -20,7 +20,7 @@ feature 'insecure direct object reference' do } end - scenario 'view any user work_info' do + scenario 'attack two' do login(@normal_user) @normal_user.user_id.should_not == 2 diff --git a/spec/features/mass_assignment_spec.rb b/spec/features/mass_assignment_spec.rb index 0e89b65..51dbc44 100644 --- a/spec/features/mass_assignment_spec.rb +++ b/spec/features/mass_assignment_spec.rb @@ -1,12 +1,12 @@ require 'spec_helper' -feature 'sql injection' do +feature 'mass assignment' do before do UserFixture.reset_all_users @normal_user = UserFixture.normal_user end - scenario 'mass assignment attack update account_settings' do + scenario 'attack one' do @normal_user.admin.should be_false login(@normal_user) @@ -20,7 +20,7 @@ feature 'sql injection' do pending(:if => verifying_fixed?) { @normal_user.reload.admin.should be_true } end - scenario 'mass assignment attack create new account' do + scenario 'attack two' do params = {:user => {:admin => 't', :email => 'hackety@h4x0rs.c0m', :first_name => 'hackety', diff --git a/spec/features/sql_injection_spec.rb b/spec/features/sql_injection_spec.rb index 45a2800..15ebdfd 100644 --- a/spec/features/sql_injection_spec.rb +++ b/spec/features/sql_injection_spec.rb @@ -7,7 +7,7 @@ feature 'sql injection' do @admin_user = User.where("admin='t'").first end - scenario 'injection attack on account_settings' do + scenario 'attack' do @admin_user.admin.should be_true login(@normal_user) diff --git a/spec/features/unvalidated_redirects_spec.rb b/spec/features/unvalidated_redirects_spec.rb index 8db28e8..82cdc47 100644 --- a/spec/features/unvalidated_redirects_spec.rb +++ b/spec/features/unvalidated_redirects_spec.rb @@ -6,7 +6,7 @@ feature 'unvalidated redirect' do @normal_user = UserFixture.normal_user end - scenario 'login redirects to anywhere', :js => true do + scenario 'attack', :js => true do visit '/?url=http://example.com/do/evil/things' within('.signup') do fill_in 'email', :with => @normal_user.email diff --git a/spec/features/url_access_spec.rb b/spec/features/url_access_spec.rb index f33f858..6d71ebe 100644 --- a/spec/features/url_access_spec.rb +++ b/spec/features/url_access_spec.rb @@ -6,7 +6,7 @@ feature 'url access' do @normal_user = UserFixture.normal_user end - scenario 'admin route not protected', :js => true do + scenario 'attack', :js => true do login @normal_user visit '/admin/1/dashboard' diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb index 264eaaf..f96148d 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 'xss attack on account_settings', :js => true do + scenario 'attack', :js => true do login @normal_user visit "/users/#{@normal_user.user_id}/account_settings" diff --git a/spec/support/capybara_shared.rb b/spec/support/capybara_shared.rb index 55cd9d3..6e3657d 100644 --- a/spec/support/capybara_shared.rb +++ b/spec/support/capybara_shared.rb @@ -5,8 +5,34 @@ # However, RailsGoat maintainers need the Capybara features to pass to indicate # changes to the site have not inadvertently removed or fixed any vulnerabilities # since the whole point is to provide a site for a developer to fix. +@@displayed_spec_notice = false + def verifying_fixed? - !ENV['RAILSGOAT_MAINTAINER'] + maintainer_env_name = 'RAILSGOAT_MAINTAINER' + result = !ENV[maintainer_env_name] + if !@@displayed_spec_notice && result + puts <<-NOTICE + +****************************************************************************** + You are running the RailsGoat Capybara Specs in Training mode. These specs + are supposed to fail, indicating vulnerabilities exist. They contain + spoilers, so do not read the code in spec/features if your goal is to + learn more about patching the vulnerabilities. You should fix the + vulnerabilities in the application in order to get these specs to pass**. + You can use them to measure your progress. + + These same specs will pass if you set the #{maintainer_env_name} ENV + variable. + + **NOTE: The RSpec pending feature is used to toggle the outcome of these + specs between Training mode and RailsGoat Maintainer mode, so when the + vulnerabilities are removed, these specs actually won't 'pass' but go into + a 'pending' state. +****************************************************************************** + NOTICE + @@displayed_spec_notice = true + end + result end def login(user) From e71834b830c3e8553f5c2b6d850938b56d934080 Mon Sep 17 00:00:00 2001 From: chrismo Date: Mon, 7 Oct 2013 10:21:33 -0500 Subject: [PATCH 10/30] Additions to README --- README.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/README.md b/README.md index 0740350..2f87ac6 100755 --- a/README.md +++ b/README.md @@ -19,7 +19,24 @@ Start hacking!!! +### Running Capybara Tests ### + +RailsGoat now includes a set of _failing_ Capybara RSpecs, each one indicating a separate vulnerability exists +in the application. + +To run them, though, you'll first need to [install PhantomJS](https://github.com/jonleighton/poltergeist#installing-phantomjs), +which is required by the Poltergeist Capybara driver. Then just rake: + + rake + +NOTE: As vulnerabilities are fixed in the application, these specs won't change from to passing but to _pending_. + ### Developer Note ### + +As changes are made to the application, the Capybara RSpecs can be used to verify the vulnerabilities +in the application are still intact. To use them in this way, and have them _pass_ instead of fail, +set the `RAILSGOAT_MAINTAINER` environment variable. +

Conversion to the OWASP Top 10, 2013 is under way. From 0d15dd0a6c196245075242e495181d7371014688 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Mon, 7 Oct 2013 13:35:39 -0400 Subject: [PATCH 11/30] pinning dbcleaner to lower version due to https://github.com/bmabey/database_cleaner/issues/224 --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index b175ad7..2d11b1e 100755 --- a/Gemfile +++ b/Gemfile @@ -26,7 +26,7 @@ gem 'gauntlt' group :development, :test do gem 'capybara' - gem 'database_cleaner' + gem 'database_cleaner', '< 1.1.0' gem 'poltergeist' gem 'rspec-rails' end diff --git a/Gemfile.lock b/Gemfile.lock index dc27f3d..7630c7b 100755 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -70,7 +70,7 @@ GEM diff-lcs (>= 1.1.3) gherkin (~> 2.12.0) multi_json (~> 1.3) - database_cleaner (1.1.1) + database_cleaner (1.0.1) diff-lcs (1.2.4) em-websocket (0.5.0) eventmachine (>= 0.12.9) @@ -248,7 +248,7 @@ DEPENDENCIES bundler-audit capybara coffee-rails (~> 3.2.1) - database_cleaner + database_cleaner (< 1.1.0) execjs foreman gauntlt From d0d5165c6cb774e0fd6fcb3abf05a2a1fa4a7b88 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Mon, 7 Oct 2013 13:46:55 -0400 Subject: [PATCH 12/30] adding env variable to run vulnerability tests --- .travis.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 8c734ac..4ae7691 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,4 +1,5 @@ language: ruby rvm: - "1.9.3" -before_script: rake db:migrate +before_script: rake db:setup +env: RAILSGOAT_MAINTAINER=true \ No newline at end of file From d9eadddfe3234012f8ee40743046ee3c1b0fbfff Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Mon, 7 Oct 2013 13:47:33 -0400 Subject: [PATCH 13/30] adding flash message with validation errors, and redirect to sign_up --- app/controllers/users_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index ce51404..535045e 100755 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -15,7 +15,8 @@ class UsersController < ApplicationController redirect_to home_dashboard_index_path else @user = user - render :new + flash[:error] = user.errors.full_messages.to_sentence + redirect_to :sign_up end end From 82e40fe581a4045c33950656a57073fab3aee452 Mon Sep 17 00:00:00 2001 From: mccabe615 Date: Mon, 7 Oct 2013 14:05:27 -0400 Subject: [PATCH 14/30] Update README.md --- README.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 0740350..805a782 100755 --- a/README.md +++ b/README.md @@ -5,13 +5,11 @@ cd railsgoat - rvm use 1.9.3@railsgoat --create + rvm use 1.9.3@railsgoat --create # https://rvm.io/ bundle - rake db:create - - rake db:migrate + rake db:setup rails s @@ -33,6 +31,7 @@ Then proceed with browsing the site as normal :thumbsup: ### Build Info ### [![Code Climate](https://codeclimate.com/github/OWASP/railsgoat.png)](https://codeclimate.com/github/OWASP/railsgoat) +[![Build Status](https://travis-ci.org/mccabe615/railsgoat.png?branch=master)](https://travis-ci.org/mccabe615/railsgoat) ### License Stuff ### From 0b5be6d55e02f6ccf8f04e9660a0aa1793a63095 Mon Sep 17 00:00:00 2001 From: mccabe615 Date: Mon, 7 Oct 2013 14:05:50 -0400 Subject: [PATCH 15/30] Update README.md --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 805a782..0bf210f 100755 --- a/README.md +++ b/README.md @@ -31,6 +31,7 @@ Then proceed with browsing the site as normal :thumbsup: ### Build Info ### [![Code Climate](https://codeclimate.com/github/OWASP/railsgoat.png)](https://codeclimate.com/github/OWASP/railsgoat) + [![Build Status](https://travis-ci.org/mccabe615/railsgoat.png?branch=master)](https://travis-ci.org/mccabe615/railsgoat) ### License Stuff ### From 398c1bbe8394949138b1536672b064c5773abfd6 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Mon, 7 Oct 2013 14:18:17 -0400 Subject: [PATCH 16/30] moving vulnerability tests and adding password complexity test --- .../{ => vulnerabilities}/broken_auth_spec.rb | 0 .../command_injection_spec.rb | 0 .../{ => vulnerabilities}/csrf_spec.rb | 0 .../info_disclosure_spec.rb | 0 .../insecure_dor_spec.rb | 0 .../mass_assignment_spec.rb | 0 .../password_complexity_spec.rb | 21 +++++++++++++++++++ .../sql_injection_spec.rb | 0 .../unvalidated_redirects_spec.rb | 0 .../{ => vulnerabilities}/url_access_spec.rb | 0 .../{ => vulnerabilities}/xss_spec.rb | 0 11 files changed, 21 insertions(+) rename spec/features/{ => vulnerabilities}/broken_auth_spec.rb (100%) rename spec/features/{ => vulnerabilities}/command_injection_spec.rb (100%) rename spec/features/{ => vulnerabilities}/csrf_spec.rb (100%) rename spec/features/{ => vulnerabilities}/info_disclosure_spec.rb (100%) rename spec/features/{ => vulnerabilities}/insecure_dor_spec.rb (100%) rename spec/features/{ => vulnerabilities}/mass_assignment_spec.rb (100%) create mode 100644 spec/features/vulnerabilities/password_complexity_spec.rb rename spec/features/{ => vulnerabilities}/sql_injection_spec.rb (100%) rename spec/features/{ => vulnerabilities}/unvalidated_redirects_spec.rb (100%) rename spec/features/{ => vulnerabilities}/url_access_spec.rb (100%) rename spec/features/{ => vulnerabilities}/xss_spec.rb (100%) diff --git a/spec/features/broken_auth_spec.rb b/spec/features/vulnerabilities/broken_auth_spec.rb similarity index 100% rename from spec/features/broken_auth_spec.rb rename to spec/features/vulnerabilities/broken_auth_spec.rb diff --git a/spec/features/command_injection_spec.rb b/spec/features/vulnerabilities/command_injection_spec.rb similarity index 100% rename from spec/features/command_injection_spec.rb rename to spec/features/vulnerabilities/command_injection_spec.rb diff --git a/spec/features/csrf_spec.rb b/spec/features/vulnerabilities/csrf_spec.rb similarity index 100% rename from spec/features/csrf_spec.rb rename to spec/features/vulnerabilities/csrf_spec.rb diff --git a/spec/features/info_disclosure_spec.rb b/spec/features/vulnerabilities/info_disclosure_spec.rb similarity index 100% rename from spec/features/info_disclosure_spec.rb rename to spec/features/vulnerabilities/info_disclosure_spec.rb diff --git a/spec/features/insecure_dor_spec.rb b/spec/features/vulnerabilities/insecure_dor_spec.rb similarity index 100% rename from spec/features/insecure_dor_spec.rb rename to spec/features/vulnerabilities/insecure_dor_spec.rb diff --git a/spec/features/mass_assignment_spec.rb b/spec/features/vulnerabilities/mass_assignment_spec.rb similarity index 100% rename from spec/features/mass_assignment_spec.rb rename to spec/features/vulnerabilities/mass_assignment_spec.rb diff --git a/spec/features/vulnerabilities/password_complexity_spec.rb b/spec/features/vulnerabilities/password_complexity_spec.rb new file mode 100644 index 0000000..a92bcbd --- /dev/null +++ b/spec/features/vulnerabilities/password_complexity_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +feature 'password complexity' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'one' do + visit '/signup' + within('.signup') do + fill_in 'user_email', :with => @normal_user.email + 'not' + fill_in 'user_first_name', :with => @normal_user.first_name + fill_in 'user_last_name', :with => @normal_user.last_name + 'not' + fill_in 'user_password', :with => 'password' + fill_in 'user_password_confirmation', :with => 'password' + end + click_on 'Submit' + pending(:if => verifying_fixed?) {current_path.should == '/dashboard/home'} + end +end \ No newline at end of file diff --git a/spec/features/sql_injection_spec.rb b/spec/features/vulnerabilities/sql_injection_spec.rb similarity index 100% rename from spec/features/sql_injection_spec.rb rename to spec/features/vulnerabilities/sql_injection_spec.rb diff --git a/spec/features/unvalidated_redirects_spec.rb b/spec/features/vulnerabilities/unvalidated_redirects_spec.rb similarity index 100% rename from spec/features/unvalidated_redirects_spec.rb rename to spec/features/vulnerabilities/unvalidated_redirects_spec.rb diff --git a/spec/features/url_access_spec.rb b/spec/features/vulnerabilities/url_access_spec.rb similarity index 100% rename from spec/features/url_access_spec.rb rename to spec/features/vulnerabilities/url_access_spec.rb diff --git a/spec/features/xss_spec.rb b/spec/features/vulnerabilities/xss_spec.rb similarity index 100% rename from spec/features/xss_spec.rb rename to spec/features/vulnerabilities/xss_spec.rb From 19ee423d8dc179364ebd0b38a877ee6b5d154edb Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Mon, 7 Oct 2013 13:35:39 -0400 Subject: [PATCH 17/30] pinning dbcleaner to lower version due to https://github.com/bmabey/database_cleaner/issues/224 --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index b175ad7..2d11b1e 100755 --- a/Gemfile +++ b/Gemfile @@ -26,7 +26,7 @@ gem 'gauntlt' group :development, :test do gem 'capybara' - gem 'database_cleaner' + gem 'database_cleaner', '< 1.1.0' gem 'poltergeist' gem 'rspec-rails' end diff --git a/Gemfile.lock b/Gemfile.lock index dc27f3d..7630c7b 100755 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -70,7 +70,7 @@ GEM diff-lcs (>= 1.1.3) gherkin (~> 2.12.0) multi_json (~> 1.3) - database_cleaner (1.1.1) + database_cleaner (1.0.1) diff-lcs (1.2.4) em-websocket (0.5.0) eventmachine (>= 0.12.9) @@ -248,7 +248,7 @@ DEPENDENCIES bundler-audit capybara coffee-rails (~> 3.2.1) - database_cleaner + database_cleaner (< 1.1.0) execjs foreman gauntlt From cc7535af307913f6dd2a2baa63544b99bc6fe1ad Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Mon, 7 Oct 2013 13:46:55 -0400 Subject: [PATCH 18/30] adding env variable to run vulnerability tests --- .travis.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 8c734ac..4ae7691 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,4 +1,5 @@ language: ruby rvm: - "1.9.3" -before_script: rake db:migrate +before_script: rake db:setup +env: RAILSGOAT_MAINTAINER=true \ No newline at end of file From 73f3272aa128a0c03c8cf7d76314bc7d3024f1d7 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Mon, 7 Oct 2013 13:47:33 -0400 Subject: [PATCH 19/30] adding flash message with validation errors, and redirect to sign_up --- app/controllers/users_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index ce51404..535045e 100755 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -15,7 +15,8 @@ class UsersController < ApplicationController redirect_to home_dashboard_index_path else @user = user - render :new + flash[:error] = user.errors.full_messages.to_sentence + redirect_to :sign_up end end From 30f432e8a07241b7cde4a63f46635bb2e7a76671 Mon Sep 17 00:00:00 2001 From: mccabe615 Date: Mon, 7 Oct 2013 14:05:27 -0400 Subject: [PATCH 20/30] Update README.md --- README.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 2f87ac6..8fb03b4 100755 --- a/README.md +++ b/README.md @@ -5,13 +5,11 @@ cd railsgoat - rvm use 1.9.3@railsgoat --create + rvm use 1.9.3@railsgoat --create # https://rvm.io/ bundle - rake db:create - - rake db:migrate + rake db:setup rails s @@ -50,6 +48,7 @@ Then proceed with browsing the site as normal :thumbsup: ### Build Info ### [![Code Climate](https://codeclimate.com/github/OWASP/railsgoat.png)](https://codeclimate.com/github/OWASP/railsgoat) +[![Build Status](https://travis-ci.org/mccabe615/railsgoat.png?branch=master)](https://travis-ci.org/mccabe615/railsgoat) ### License Stuff ### From 829b566c297c1d2767ad41a5f7c51e20c545155f Mon Sep 17 00:00:00 2001 From: mccabe615 Date: Mon, 7 Oct 2013 14:05:50 -0400 Subject: [PATCH 21/30] Update README.md --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 8fb03b4..b3d2908 100755 --- a/README.md +++ b/README.md @@ -48,6 +48,7 @@ Then proceed with browsing the site as normal :thumbsup: ### Build Info ### [![Code Climate](https://codeclimate.com/github/OWASP/railsgoat.png)](https://codeclimate.com/github/OWASP/railsgoat) + [![Build Status](https://travis-ci.org/mccabe615/railsgoat.png?branch=master)](https://travis-ci.org/mccabe615/railsgoat) ### License Stuff ### From 9b3181eef945df143ad1e306f4c63fbb05e5e2a6 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Mon, 7 Oct 2013 14:18:17 -0400 Subject: [PATCH 22/30] moving vulnerability tests and adding password complexity test --- .../{ => vulnerabilities}/broken_auth_spec.rb | 0 .../command_injection_spec.rb | 0 .../{ => vulnerabilities}/csrf_spec.rb | 0 .../info_disclosure_spec.rb | 0 .../insecure_dor_spec.rb | 0 .../mass_assignment_spec.rb | 0 .../password_complexity_spec.rb | 21 +++++++++++++++++++ .../sql_injection_spec.rb | 0 .../unvalidated_redirects_spec.rb | 0 .../{ => vulnerabilities}/url_access_spec.rb | 0 .../{ => vulnerabilities}/xss_spec.rb | 0 11 files changed, 21 insertions(+) rename spec/features/{ => vulnerabilities}/broken_auth_spec.rb (100%) rename spec/features/{ => vulnerabilities}/command_injection_spec.rb (100%) rename spec/features/{ => vulnerabilities}/csrf_spec.rb (100%) rename spec/features/{ => vulnerabilities}/info_disclosure_spec.rb (100%) rename spec/features/{ => vulnerabilities}/insecure_dor_spec.rb (100%) rename spec/features/{ => vulnerabilities}/mass_assignment_spec.rb (100%) create mode 100644 spec/features/vulnerabilities/password_complexity_spec.rb rename spec/features/{ => vulnerabilities}/sql_injection_spec.rb (100%) rename spec/features/{ => vulnerabilities}/unvalidated_redirects_spec.rb (100%) rename spec/features/{ => vulnerabilities}/url_access_spec.rb (100%) rename spec/features/{ => vulnerabilities}/xss_spec.rb (100%) diff --git a/spec/features/broken_auth_spec.rb b/spec/features/vulnerabilities/broken_auth_spec.rb similarity index 100% rename from spec/features/broken_auth_spec.rb rename to spec/features/vulnerabilities/broken_auth_spec.rb diff --git a/spec/features/command_injection_spec.rb b/spec/features/vulnerabilities/command_injection_spec.rb similarity index 100% rename from spec/features/command_injection_spec.rb rename to spec/features/vulnerabilities/command_injection_spec.rb diff --git a/spec/features/csrf_spec.rb b/spec/features/vulnerabilities/csrf_spec.rb similarity index 100% rename from spec/features/csrf_spec.rb rename to spec/features/vulnerabilities/csrf_spec.rb diff --git a/spec/features/info_disclosure_spec.rb b/spec/features/vulnerabilities/info_disclosure_spec.rb similarity index 100% rename from spec/features/info_disclosure_spec.rb rename to spec/features/vulnerabilities/info_disclosure_spec.rb diff --git a/spec/features/insecure_dor_spec.rb b/spec/features/vulnerabilities/insecure_dor_spec.rb similarity index 100% rename from spec/features/insecure_dor_spec.rb rename to spec/features/vulnerabilities/insecure_dor_spec.rb diff --git a/spec/features/mass_assignment_spec.rb b/spec/features/vulnerabilities/mass_assignment_spec.rb similarity index 100% rename from spec/features/mass_assignment_spec.rb rename to spec/features/vulnerabilities/mass_assignment_spec.rb diff --git a/spec/features/vulnerabilities/password_complexity_spec.rb b/spec/features/vulnerabilities/password_complexity_spec.rb new file mode 100644 index 0000000..a92bcbd --- /dev/null +++ b/spec/features/vulnerabilities/password_complexity_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +feature 'password complexity' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'one' do + visit '/signup' + within('.signup') do + fill_in 'user_email', :with => @normal_user.email + 'not' + fill_in 'user_first_name', :with => @normal_user.first_name + fill_in 'user_last_name', :with => @normal_user.last_name + 'not' + fill_in 'user_password', :with => 'password' + fill_in 'user_password_confirmation', :with => 'password' + end + click_on 'Submit' + pending(:if => verifying_fixed?) {current_path.should == '/dashboard/home'} + end +end \ No newline at end of file diff --git a/spec/features/sql_injection_spec.rb b/spec/features/vulnerabilities/sql_injection_spec.rb similarity index 100% rename from spec/features/sql_injection_spec.rb rename to spec/features/vulnerabilities/sql_injection_spec.rb diff --git a/spec/features/unvalidated_redirects_spec.rb b/spec/features/vulnerabilities/unvalidated_redirects_spec.rb similarity index 100% rename from spec/features/unvalidated_redirects_spec.rb rename to spec/features/vulnerabilities/unvalidated_redirects_spec.rb diff --git a/spec/features/url_access_spec.rb b/spec/features/vulnerabilities/url_access_spec.rb similarity index 100% rename from spec/features/url_access_spec.rb rename to spec/features/vulnerabilities/url_access_spec.rb diff --git a/spec/features/xss_spec.rb b/spec/features/vulnerabilities/xss_spec.rb similarity index 100% rename from spec/features/xss_spec.rb rename to spec/features/vulnerabilities/xss_spec.rb From a93159c9f23f18866af3221fae750172faf65420 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Wed, 9 Oct 2013 11:07:13 -0400 Subject: [PATCH 23/30] adding launchy --- Gemfile | 1 + Gemfile.lock | 6 +++++- spec/{features => }/vulnerabilities/broken_auth_spec.rb | 0 .../vulnerabilities/command_injection_spec.rb | 0 spec/{features => }/vulnerabilities/csrf_spec.rb | 0 spec/{features => }/vulnerabilities/info_disclosure_spec.rb | 0 spec/{features => }/vulnerabilities/insecure_dor_spec.rb | 0 spec/{features => }/vulnerabilities/mass_assignment_spec.rb | 0 .../vulnerabilities/password_complexity_spec.rb | 0 spec/{features => }/vulnerabilities/sql_injection_spec.rb | 0 .../vulnerabilities/unvalidated_redirects_spec.rb | 0 spec/{features => }/vulnerabilities/url_access_spec.rb | 0 spec/{features => }/vulnerabilities/xss_spec.rb | 0 13 files changed, 6 insertions(+), 1 deletion(-) rename spec/{features => }/vulnerabilities/broken_auth_spec.rb (100%) rename spec/{features => }/vulnerabilities/command_injection_spec.rb (100%) rename spec/{features => }/vulnerabilities/csrf_spec.rb (100%) rename spec/{features => }/vulnerabilities/info_disclosure_spec.rb (100%) rename spec/{features => }/vulnerabilities/insecure_dor_spec.rb (100%) rename spec/{features => }/vulnerabilities/mass_assignment_spec.rb (100%) rename spec/{features => }/vulnerabilities/password_complexity_spec.rb (100%) rename spec/{features => }/vulnerabilities/sql_injection_spec.rb (100%) rename spec/{features => }/vulnerabilities/unvalidated_redirects_spec.rb (100%) rename spec/{features => }/vulnerabilities/url_access_spec.rb (100%) rename spec/{features => }/vulnerabilities/xss_spec.rb (100%) diff --git a/Gemfile b/Gemfile index 2d11b1e..253dec3 100755 --- a/Gemfile +++ b/Gemfile @@ -25,6 +25,7 @@ end gem 'gauntlt' group :development, :test do + gem 'launchy' gem 'capybara' gem 'database_cleaner', '< 1.1.0' gem 'poltergeist' diff --git a/Gemfile.lock b/Gemfile.lock index 7630c7b..8bf624c 100755 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -28,6 +28,7 @@ GEM activesupport (3.2.13) i18n (= 0.6.1) multi_json (~> 1.0) + addressable (2.3.5) arel (3.0.2) aruba (0.5.3) childprocess (>= 0.3.6) @@ -124,6 +125,8 @@ GEM thor (>= 0.14, < 2.0) json (1.7.7) kgio (2.8.0) + launchy (2.3.0) + addressable (~> 2.3) libv8 (3.16.14.3) listen (0.7.3) lumberjack (1.0.3) @@ -172,7 +175,7 @@ GEM rdoc (~> 3.4) thor (>= 0.14.6, < 2.0) raindrops (0.10.0) - rake (10.0.4) + rake (10.1.0) rb-fsevent (0.9.3) rdoc (3.12.2) json (~> 1.4) @@ -258,6 +261,7 @@ DEPENDENCIES guard-shell jquery-fileupload-rails jquery-rails + launchy poltergeist powder pry diff --git a/spec/features/vulnerabilities/broken_auth_spec.rb b/spec/vulnerabilities/broken_auth_spec.rb similarity index 100% rename from spec/features/vulnerabilities/broken_auth_spec.rb rename to spec/vulnerabilities/broken_auth_spec.rb diff --git a/spec/features/vulnerabilities/command_injection_spec.rb b/spec/vulnerabilities/command_injection_spec.rb similarity index 100% rename from spec/features/vulnerabilities/command_injection_spec.rb rename to spec/vulnerabilities/command_injection_spec.rb diff --git a/spec/features/vulnerabilities/csrf_spec.rb b/spec/vulnerabilities/csrf_spec.rb similarity index 100% rename from spec/features/vulnerabilities/csrf_spec.rb rename to spec/vulnerabilities/csrf_spec.rb diff --git a/spec/features/vulnerabilities/info_disclosure_spec.rb b/spec/vulnerabilities/info_disclosure_spec.rb similarity index 100% rename from spec/features/vulnerabilities/info_disclosure_spec.rb rename to spec/vulnerabilities/info_disclosure_spec.rb diff --git a/spec/features/vulnerabilities/insecure_dor_spec.rb b/spec/vulnerabilities/insecure_dor_spec.rb similarity index 100% rename from spec/features/vulnerabilities/insecure_dor_spec.rb rename to spec/vulnerabilities/insecure_dor_spec.rb diff --git a/spec/features/vulnerabilities/mass_assignment_spec.rb b/spec/vulnerabilities/mass_assignment_spec.rb similarity index 100% rename from spec/features/vulnerabilities/mass_assignment_spec.rb rename to spec/vulnerabilities/mass_assignment_spec.rb diff --git a/spec/features/vulnerabilities/password_complexity_spec.rb b/spec/vulnerabilities/password_complexity_spec.rb similarity index 100% rename from spec/features/vulnerabilities/password_complexity_spec.rb rename to spec/vulnerabilities/password_complexity_spec.rb diff --git a/spec/features/vulnerabilities/sql_injection_spec.rb b/spec/vulnerabilities/sql_injection_spec.rb similarity index 100% rename from spec/features/vulnerabilities/sql_injection_spec.rb rename to spec/vulnerabilities/sql_injection_spec.rb diff --git a/spec/features/vulnerabilities/unvalidated_redirects_spec.rb b/spec/vulnerabilities/unvalidated_redirects_spec.rb similarity index 100% rename from spec/features/vulnerabilities/unvalidated_redirects_spec.rb rename to spec/vulnerabilities/unvalidated_redirects_spec.rb diff --git a/spec/features/vulnerabilities/url_access_spec.rb b/spec/vulnerabilities/url_access_spec.rb similarity index 100% rename from spec/features/vulnerabilities/url_access_spec.rb rename to spec/vulnerabilities/url_access_spec.rb diff --git a/spec/features/vulnerabilities/xss_spec.rb b/spec/vulnerabilities/xss_spec.rb similarity index 100% rename from spec/features/vulnerabilities/xss_spec.rb rename to spec/vulnerabilities/xss_spec.rb From bbed455178564072bf19716c2cf3e375079e6e60 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Wed, 9 Oct 2013 11:08:39 -0400 Subject: [PATCH 24/30] verifying user exists before trying to update --- app/controllers/users_controller.rb | 45 ++++++++++++++++------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 535045e..e7f1684 100755 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,12 +1,12 @@ class UsersController < ApplicationController - + skip_before_filter :has_info skip_before_filter :authenticated, :only => [:new, :create] - + def new @user = User.new end - + def create user = User.new(params[:user]) user.build_benefits_data @@ -19,32 +19,37 @@ class UsersController < ApplicationController redirect_to :sign_up end end - + def account_settings @user = current_user end - + def update message = false #Safest # user = current_user - + # Still an Insecure DoR vulnerability #user = User.find(:first, :conditions => ["user_id = ?", "#{params[:user][:user_id]}"]) - + user = User.find(:first, :conditions => "user_id = '#{params[:user][:user_id]}'") - user.skip_user_id_assign = true - user.skip_hash_password = true - user.update_attributes(params[:user].reject { |k| %w(password password_confirmation user_id).include? k }) - if !(params[:user][:password].empty?) && (params[:user][:password] == params[:user][:password_confirmation]) - user.skip_hash_password = false - user.password = params[:user][:password] - end - message = true if user.save! - respond_to do |format| - format.html { redirect_to user_account_settings_path(:user_id => current_user.user_id) } - format.json { render :json => {:msg => message ? "success" : "false "} } + if user + user.skip_user_id_assign = true + user.skip_hash_password = true + user.update_attributes(params[:user].reject { |k| %w(password password_confirmation user_id).include? k }) + if !(params[:user][:password].empty?) && (params[:user][:password] == params[:user][:password_confirmation]) + user.skip_hash_password = false + user.password = params[:user][:password] + end + message = true if user.save! + respond_to do |format| + format.html { redirect_to user_account_settings_path(:user_id => current_user.user_id) } + format.json { render :json => {:msg => message ? "success" : "false "} } + end + else + flash[:error] = "Could not update user!" + redirect_to user_account_settings_path(:user_id => current_user.user_id) end end - -end + +end \ No newline at end of file From c9a64b9e82eea5470169cc7dd6e72dd371da31d7 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Wed, 9 Oct 2013 11:09:15 -0400 Subject: [PATCH 25/30] adding simple sqlmap gauntlt script, WIP --- gauntlt_scripts/sqlmap.attack | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 gauntlt_scripts/sqlmap.attack diff --git a/gauntlt_scripts/sqlmap.attack b/gauntlt_scripts/sqlmap.attack new file mode 100644 index 0000000..f766c61 --- /dev/null +++ b/gauntlt_scripts/sqlmap.attack @@ -0,0 +1,17 @@ +#sqlmap.attack +Feature: Run sqlmap against a target + # See: + # https://github.com/sqlmapproject/sqlmap/wiki/Usage + + Scenario: Identify SQL injection vulnerabilities + Given "sqlmap" is installed + And the following profile: + | target_url | http://localhost:300/| + When I launch a "sqlmap" attack with: + """ + /usr/bin/python -u --dbms sqlite + """ + Then the output should contain: + """ + sqlmap identified the following injection points + """ \ No newline at end of file From e999c0250624b18a96b689e79113f5efdaffaa7b Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Wed, 9 Oct 2013 12:55:00 -0400 Subject: [PATCH 26/30] adding password hashing spec --- spec/support/capybara_shared.rb | 2 +- spec/vulnerabilities/password_hashing_spec.rb | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 spec/vulnerabilities/password_hashing_spec.rb diff --git a/spec/support/capybara_shared.rb b/spec/support/capybara_shared.rb index 6e3657d..2f982f9 100644 --- a/spec/support/capybara_shared.rb +++ b/spec/support/capybara_shared.rb @@ -16,7 +16,7 @@ def verifying_fixed? ****************************************************************************** You are running the RailsGoat Capybara Specs in Training mode. These specs are supposed to fail, indicating vulnerabilities exist. They contain - spoilers, so do not read the code in spec/features if your goal is to + spoilers, so do not read the code in spec/vulnerabilities if your goal is to learn more about patching the vulnerabilities. You should fix the vulnerabilities in the application in order to get these specs to pass**. You can use them to measure your progress. diff --git a/spec/vulnerabilities/password_hashing_spec.rb b/spec/vulnerabilities/password_hashing_spec.rb new file mode 100644 index 0000000..2c2e7a6 --- /dev/null +++ b/spec/vulnerabilities/password_hashing_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +feature 'improper password hashing' do + before do + UserFixture.reset_all_users + @normal_user = UserFixture.normal_user + end + + scenario 'with just md5' do + new_pass = 'testpassword' + @normal_user.password = new_pass + @normal_user.password_confirmation = new_pass + @normal_user.save + pending(:if => verifying_fixed?) {Digest::MD5.hexdigest(new_pass).should == @normal_user.password} + end + + scenario 'with md5 and salt' do + if @normal_user.has_attribute?('salt') + new_pass = 'testpassword' + @normal_user.password = new_pass + @normal_user.password_confirmation = new_pass + @normal_user.save + pending(:if => verifying_fixed?) {Digest::MD5.hexdigest(@normal_user.salt + new_pass).should == @normal_user.password} + end + end +end \ No newline at end of file From 82387a1f92b50b5a08153c30fd4da7b757e91256 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Wed, 9 Oct 2013 13:18:32 -0400 Subject: [PATCH 27/30] updating spec to fail if salt is not defined --- spec/vulnerabilities/password_hashing_spec.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/vulnerabilities/password_hashing_spec.rb b/spec/vulnerabilities/password_hashing_spec.rb index 2c2e7a6..2d9ddb0 100644 --- a/spec/vulnerabilities/password_hashing_spec.rb +++ b/spec/vulnerabilities/password_hashing_spec.rb @@ -21,6 +21,9 @@ feature 'improper password hashing' do @normal_user.password_confirmation = new_pass @normal_user.save pending(:if => verifying_fixed?) {Digest::MD5.hexdigest(@normal_user.salt + new_pass).should == @normal_user.password} + else + #fail test if salt attribute not defined + true.should == false end end end \ No newline at end of file From 77a3940530fe854898d26bc329e16fa58d508ce9 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Wed, 9 Oct 2013 13:20:30 -0400 Subject: [PATCH 28/30] adding training rake task to ease running training specs --- lib/tasks/traning.rake | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 lib/tasks/traning.rake diff --git a/lib/tasks/traning.rake b/lib/tasks/traning.rake new file mode 100644 index 0000000..2a40c39 --- /dev/null +++ b/lib/tasks/traning.rake @@ -0,0 +1,4 @@ +desc 'run training tests' +task :training do + Rake::Task["spec:vulnerabilities"].invoke +end \ No newline at end of file From 79915519b11a023f40ee9a09ada2eb1b3000f924 Mon Sep 17 00:00:00 2001 From: mccabe615 Date: Wed, 9 Oct 2013 13:25:54 -0400 Subject: [PATCH 29/30] Update README.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b3d2908..850287c 100755 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ in the application. To run them, though, you'll first need to [install PhantomJS](https://github.com/jonleighton/poltergeist#installing-phantomjs), which is required by the Poltergeist Capybara driver. Then just rake: - rake + rake training NOTE: As vulnerabilities are fixed in the application, these specs won't change from to passing but to _pending_. From c9231233e5207d0c6a99d0ddf999276cea9e0c28 Mon Sep 17 00:00:00 2001 From: Mike McCabe Date: Wed, 9 Oct 2013 14:23:46 -0400 Subject: [PATCH 30/30] make test go into pending unless salt attribute defined for travis --- spec/vulnerabilities/password_hashing_spec.rb | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/spec/vulnerabilities/password_hashing_spec.rb b/spec/vulnerabilities/password_hashing_spec.rb index 2d9ddb0..077a352 100644 --- a/spec/vulnerabilities/password_hashing_spec.rb +++ b/spec/vulnerabilities/password_hashing_spec.rb @@ -15,15 +15,11 @@ feature 'improper password hashing' do end scenario 'with md5 and salt' do - if @normal_user.has_attribute?('salt') - new_pass = 'testpassword' - @normal_user.password = new_pass - @normal_user.password_confirmation = new_pass - @normal_user.save - pending(:if => verifying_fixed?) {Digest::MD5.hexdigest(@normal_user.salt + new_pass).should == @normal_user.password} - else - #fail test if salt attribute not defined - true.should == false - end + pending unless @normal_user.has_attribute?('salt') + new_pass = 'testpassword' + @normal_user.password = new_pass + @normal_user.password_confirmation = new_pass + @normal_user.save + pending(:if => verifying_fixed?) {Digest::MD5.hexdigest(@normal_user.salt + new_pass).should == @normal_user.password} end end \ No newline at end of file