From 0df6735b534b7ff9b43c74f1e758b67e02473be0 Mon Sep 17 00:00:00 2001 From: chrismo Date: Mon, 30 Sep 2013 15:29:36 -0500 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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