From 7f4f48183cc5094d4c3fb68f05cb51ddb374fb1c Mon Sep 17 00:00:00 2001 From: Janell-Huyck Date: Thu, 7 Sep 2023 10:30:54 -0400 Subject: [PATCH] fixed format-validation vulnerability for submitting phone number --- Gemfile | 3 +- Gemfile.lock | 16 +- app/models/submitter.rb | 2 +- brakeman.html | 643 ++++++++++++++++++++++++++++++++++ spec/models/submitter_spec.rb | 24 +- 5 files changed, 674 insertions(+), 14 deletions(-) create mode 100644 brakeman.html diff --git a/Gemfile b/Gemfile index 9f6cfdb7..9ad189ac 100644 --- a/Gemfile +++ b/Gemfile @@ -65,6 +65,7 @@ group :development, :test do end group :development do + gem 'brakeman', '~> 6.0' gem 'capistrano', '~> 3.17.1', require: false gem 'capistrano-bundler', '~> 1.6', require: false gem 'capistrano-rails', '~> 1.4', require: false @@ -73,10 +74,10 @@ group :development do gem 'capistrano-rvm', require: false # Access an interactive console on exception pages or by calling 'console' anywhere in the code. gem 'listen', '>= 3.0.5', '< 3.2' - gem 'web-console', '>= 3.3.0' # Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring gem 'spring' gem 'spring-watcher-listen', '~> 2.0.0' + gem 'web-console', '>= 3.3.0' end group :test do diff --git a/Gemfile.lock b/Gemfile.lock index edc779f0..54625720 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -65,7 +65,7 @@ GEM airbrussh (1.4.2) sshkit (>= 1.6.1, != 1.7.0) ast (2.4.2) - autoprefixer-rails (10.4.13.0) + autoprefixer-rails (10.4.15.0) execjs (~> 2) base64 (0.1.1) bcrypt_pbkdf (1.1.0) @@ -76,6 +76,7 @@ GEM autoprefixer-rails (>= 9.1.0) popper_js (>= 1.14.3, < 2) sassc-rails (>= 2.0.0) + brakeman (6.0.1) builder (3.2.4) byebug (11.1.3) capistrano (3.17.3) @@ -139,8 +140,8 @@ GEM factory_bot (~> 6.2.0) railties (>= 5.0.0) ffi (1.15.5) - globalid (1.1.0) - activesupport (>= 5.0) + globalid (1.2.1) + activesupport (>= 6.1) htmlentities (4.3.4) i18n (1.14.1) concurrent-ruby (~> 1.0) @@ -168,7 +169,7 @@ GEM matrix (0.4.2) method_source (1.0.0) mini_mime (1.1.5) - minitest (5.19.0) + minitest (5.20.0) msgpack (1.7.2) mysql2 (0.5.5) net-imap (0.3.7) @@ -196,7 +197,7 @@ GEM puma (3.12.6) racc (1.7.1) rack (2.2.8) - rack-proxy (0.7.6) + rack-proxy (0.7.7) rack rack-test (2.1.0) rack (>= 1.3) @@ -294,7 +295,7 @@ GEM sprockets (> 3.0) sprockets-rails tilt - selenium-webdriver (4.11.0) + selenium-webdriver (4.12.0) rexml (~> 3.2, >= 3.2.5) rubyzip (>= 1.2.2, < 3.0) websocket (~> 1.0) @@ -339,7 +340,7 @@ GEM uglifier (4.2.0) execjs (>= 0.3.0, < 3) unicode-display_width (2.4.2) - web-console (4.2.0) + web-console (4.2.1) actionview (>= 6.0.0) activemodel (>= 6.0.0) bindex (>= 0.4.0) @@ -364,6 +365,7 @@ DEPENDENCIES bcrypt_pbkdf bootsnap (>= 1.1.0) bootstrap (~> 4.4.1) + brakeman (~> 6.0) byebug capistrano (~> 3.17.1) capistrano-bundler (~> 1.6) diff --git a/app/models/submitter.rb b/app/models/submitter.rb index 3022c306..6c5584e0 100644 --- a/app/models/submitter.rb +++ b/app/models/submitter.rb @@ -5,7 +5,7 @@ class Submitter < ApplicationRecord validates :first_name, presence: true validates :last_name, presence: true validates :mailing_address, presence: true - validates :phone_number, presence: true, format: { with: /\d{3}-\d{3}-\d{4}/, message: 'Please use the format 111-111-1111' } + validates :phone_number, presence: true, format: { with: /\A\d{3}-\d{3}-\d{4}\z/, message: 'Please use the format 111-111-1111' } validates :email_address, presence: true, format: { with: URI::MailTo::EMAIL_REGEXP, message: 'Please enter a valid email' } def self.to_csv diff --git a/brakeman.html b/brakeman.html new file mode 100644 index 00000000..a621d503 --- /dev/null +++ b/brakeman.html @@ -0,0 +1,643 @@ + + + + +Brakeman Report + + + + + + + +

Brakeman Report

+ + + + + + + + + + + + + + + + + + +
Application PathRails VersionBrakeman VersionReport TimeChecks Performed
/Users/huyckjl/Codebases/aaec6.1.7.66.0.1 + + 2023-09-07 10:18:36 -0400

+ 0.874537 seconds + 		BasicAuth, BasicAuthTimingAttack, CSRFTokenForgeryCVE, ContentTag, CookieSerialization, CreateWith, CrossSiteScripting, DefaultRoutes, Deserialize, DetailedExceptions, DigestDoS, DynamicFinders, EOLRails, EOLRuby, EscapeFunction, Evaluation, Execute, FileAccess, FileDisclosure, FilterSkipping, ForgerySetting, HeaderDoS, I18nXSS, JRubyXML, JSONEncoding, JSONEntityEscape, JSONParsing, LinkTo, LinkToHref, MailTo, MassAssignment, MimeTypeDoS, ModelAttrAccessible, ModelAttributes, ModelSerialize, NestedAttributes, NestedAttributesBypass, NumberToCurrency, PageCachingCVE, Pathname, PermitAttributes, QuoteTableName, Redirect, RegexDoS, Render, RenderDoS, RenderInline, ResponseSplitting, RouteDoS, SQL, SQLCVEs, SSLVerify, SafeBufferManipulation, SanitizeConfigCve, SanitizeMethods, SelectTag, SelectVulnerability, Send, SendFile, SessionManipulation, SessionSettings, SimpleFormat, SingleQuotes, SkipBeforeFilter, SprocketsPathTraversal, StripTags, SymbolDoSCVE, TemplateInjection, TranslateBug, UnsafeReflection, UnsafeReflectionMethods, ValidationRegex, VerbConfusion, WeakRSAKey, WithoutProtection, XMLDoS, YAMLParsing
+
+

Summary

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
Scanned/ReportedTotal
Controllers19
Models16
Templates89
Errors0
Security Warnings4 (3)
Ignored Warnings0
+
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
Warning TypeTotal
Dynamic Render Path1
File Access1
Format Validation1
Remote Code Execution1
+
+

Security Warnings

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
ConfidenceClassMethodWarning TypeCWE IDMessage
HighPagesControllervalid_page?File Access[22]
Parameter value used in file name near line 17: Pathname.new((Rails.root + "app/views/pages/#{params[:page]}.html.erb")) + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
HighAdminControllercsvRemote Code Execution[470]
Unsafe reflection method const_get called with parameter value near line 18: Object.const_get(params[:controller_name].classify) + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
MediumPagesControllershowDynamic Render Path[22]
Render path contains parameter value near line 8: render(template => "pages/#{params[:page]}", {}) + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
+

Model Warnings

+ + + + + + + + + + + + + + + + + + + + + +
ConfidenceModelWarning TypeCWE IDMessage
HighSubmitterFormat Validation[777]
Insufficient validation for phone_number using /\d{3}-\d{3}-\d{4}/. Use \A and \z as anchors near line 8 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
\ No newline at end of file diff --git a/spec/models/submitter_spec.rb b/spec/models/submitter_spec.rb index 8e577f84..12473b59 100644 --- a/spec/models/submitter_spec.rb +++ b/spec/models/submitter_spec.rb @@ -26,18 +26,32 @@ expect(subject).to_not be_valid end + it 'is valid with a properly formatted phone_number' do + subject.phone_number = '111-111-1111' + expect(subject).to be_valid + end + it 'is not valid without a phone_number' do subject.phone_number = nil expect(subject).to_not be_valid end - it 'is not valid without a email_address' do - subject.email_address = nil - expect(subject).to_not be_valid + it 'is not valid with an improperly formatted phone_number' do + [ + '1111111111', # no dashes + '111-1111-1111', # too many digits + '11-111-1111', # too few digits + '111-111-1111abc', # additional characters + 'abc111-111-1111', # additional characters + '1-111-111-1111' # too many sections and digits + ].each do |invalid_number| + subject.phone_number = invalid_number + expect(subject).to_not be_valid, "Expected #{invalid_number} to be invalid" + end end - it 'is not valid without a formatted phone_number' do - subject.phone_number = '1111111111' + it 'is not valid without a email_address' do + subject.email_address = nil expect(subject).to_not be_valid end