Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL injection not detected for certain heredocs #1433

Closed
cbortz opened this issue Nov 21, 2019 · 1 comment · Fixed by #1434
Closed

SQL injection not detected for certain heredocs #1433

cbortz opened this issue Nov 21, 2019 · 1 comment · Fixed by #1434

Comments

@cbortz
Copy link

cbortz commented Nov 21, 2019

Background

Brakeman version: 4.7.1
Rails version: 5.2.3
Ruby version: 2.5.3

Link to Rails application code: https://github.com/cbortz/brakeman-bug/blob/master/app/models/character.rb

Issue

When using String#strip_heredoc (e.g. <<-.strip_heredoc), possible SQL injection isn't detected. This method is provided by the Rails library rather than the standard library.

Otherwise, the standard heredoc (<<-) and squiggly heredoc (<<~) work as expected.

I've created a dummy Rails project and run brakeman with the debug flag (see output below). You'll see that only two possible SQL injections were detected, rather than three.

Stack trace:

Loading scanner...
Processing application in <REMOVED>/brakeman-bug
Processing gems...
Parsing <REMOVED>/brakeman-bug/Gemfile
[Notice] Detected Rails 5 application
Processing configuration...
Parsing <REMOVED>/brakeman-bug/config/application.rb
Parsing <REMOVED>/brakeman-bug/config/environments/production.rb
[Notice] Escaping HTML by default
Parsing files...
Parsing config/initializers/application_controller_renderer.rb
Parsing config/initializers/assets.rb
Parsing config/initializers/backtrace_silencers.rb
Parsing config/initializers/content_security_policy.rb
Parsing config/initializers/cookies_serializer.rb
Parsing config/initializers/filter_parameter_logging.rb
Parsing config/initializers/inflections.rb
Parsing config/initializers/mime_types.rb
Parsing config/initializers/wrap_parameters.rb
Parsing app/controllers/application_controller.rb
Parsing app/models/application_record.rb
Parsing app/models/character.rb
Parsing app/helpers/application_helper.rb
Parsing app/jobs/application_job.rb
Parsing <REMOVED>/brakeman-bug/app/views/layouts/application.html.erb
Parsing <REMOVED>/brakeman-bug/app/views/layouts/application.html.erb
Parsing <REMOVED>/brakeman-bug/app/views/layouts/mailer.html.erb
Parsing <REMOVED>/brakeman-bug/app/views/layouts/mailer.html.erb
Processing initializers...
Processing <REMOVED>/brakeman-bug/config/initializers/assets.rb
Processing <REMOVED>/brakeman-bug/config/initializers/cookies_serializer.rb
Processing <REMOVED>/brakeman-bug/config/initializers/filter_parameter_logging.rb
Processing <REMOVED>/brakeman-bug/config/initializers/wrap_parameters.rb
Processing libs...
Processing <REMOVED>/brakeman-bug/app/helpers/application_helper.rb
Processing <REMOVED>/brakeman-bug/app/jobs/application_job.rb
Processing routes...
Parsing <REMOVED>/brakeman-bug/config/routes.rb
Processing templates...
Processing <REMOVED>/brakeman-bug/app/views/layouts/application.html.erb
Processing <REMOVED>/brakeman-bug/app/views/layouts/mailer.html.erb
Processing data flow in templates...
Processing layouts/application
Processing layouts/mailer
Processing models...
Processing <REMOVED>/brakeman-bug/app/models/application_record.rb
Processing <REMOVED>/brakeman-bug/app/models/character.rb
Processing controllers...
Processing <REMOVED>/brakeman-bug/app/controllers/application_controller.rb
Processing data flow in controllers...
Processing ApplicationController
Indexing call sites...
Running checks in parallel...
 - CheckBasicAuth
 - CheckBasicAuthTimingAttack
 - CheckCrossSiteScripting
 - CheckContentTag
Automatic to_json escaping is enabled.
 - CheckCookieSerialization
 - CheckCreateWith
 - CheckDefaultRoutes
Checking for XSS in content_tag
 - CheckDeserialize
Checking each controller for default routes
Checking layouts/application for XSS
 - CheckDetailedExceptions
Checking layouts/mailer for XSS
 - CheckDigestDoS
 - CheckDynamicFinders
 - CheckEscapeFunction
 - CheckEvaluation
 - CheckExecute
Finding eval-like calls
 - CheckFileAccess
Processing eval-like calls
 - CheckFileDisclosure
Finding possible file access
Finding calls to load()
 - CheckFilterSkipping
Finding calls using FileUtils
Finding system calls using ``
 - CheckForgerySetting
Finding other system calls
 - CheckHeaderDoS
Processing system calls
 - CheckI18nXSS
Processing found calls
 - CheckJRubyXML
 - CheckJSONEncoding
 - CheckJSONParsing
 - CheckLinkTo
 - CheckLinkToHref
 - CheckMailTo
 - CheckMassAssignment
 - CheckMimeTypeDoS
 - CheckModelAttrAccessible
 - CheckModelAttributes
 - CheckModelSerialize
 - CheckNestedAttributes
 - CheckNestedAttributesBypass
 - CheckNumberToCurrency
 - CheckPermitAttributes
 - CheckQuoteTableName
 - CheckRedirect
 - CheckRegexDoS
Finding calls to redirect_to()
 - CheckRender
Finding dynamic regexes
 - CheckRenderDoS
Processing dynamic regexes
 - CheckRenderInline
 - CheckResponseSplitting
Automatic to_json escaping is enabled.
 - CheckRouteDoS
 - CheckSafeBufferManipulation
 - CheckSanitizeMethods
 - CheckSelectTag
 - CheckSelectVulnerability
 - CheckSend
 - CheckSendFile
Finding instances of #send
 - CheckSessionManipulation
Finding all calls to send_file()
 - CheckSessionSettings
 - CheckSimpleFormat
 - CheckSingleQuotes
 - CheckSkipBeforeFilter
 - CheckSprocketsPathTraversal
 - CheckSQL
 - CheckSQLCVEs
Finding possible SQL calls on models
 - CheckSSLVerify
Finding possible SQL calls with no target
 - CheckStripTags
Finding possible SQL calls using constantized()
 - CheckSymbolDoSCVE
Finding calls to strip_tags()
 - CheckTranslateBug
Finding calls to named_scope or scope
 - CheckUnsafeReflection
Processing possible SQL calls
 - CheckValidationRegex
 - CheckWithoutProtection
 - CheckXMLDoS
Finding all mass assignments
 - CheckYAMLParsing
Processing all mass assignments
Checks finished, collecting results...
Generating report...

== Brakeman Report ==

Application Path: <REMOVED>/brakeman-bug
Rails Version: 5.2.3
Brakeman Version: 4.7.1
Scan Date: 2019-11-21 16:44:22 -0500
Duration: 0.056352 seconds
Checks Run: BasicAuth, BasicAuthTimingAttack, ContentTag, CookieSerialization, CreateWith, CrossSiteScripting, DefaultRoutes, Deserialize, DetailedExceptions, DigestDoS, DynamicFinders, EscapeFunction, Evaluation, Execute, FileAccess, FileDisclosure, FilterSkipping, ForgerySetting, HeaderDoS, I18nXSS, JRubyXML, JSONEncoding, JSONParsing, LinkTo, LinkToHref, MailTo, MassAssignment, MimeTypeDoS, ModelAttrAccessible, ModelAttributes, ModelSerialize, NestedAttributes, NestedAttributesBypass, NumberToCurrency, PermitAttributes, QuoteTableName, Redirect, RegexDoS, Render, RenderDoS, RenderInline, ResponseSplitting, RouteDoS, SQL, SQLCVEs, SSLVerify, SafeBufferManipulation, SanitizeMethods, SelectTag, SelectVulnerability, Send, SendFile, SessionManipulation, SessionSettings, SimpleFormat, SingleQuotes, SkipBeforeFilter, SprocketsPathTraversal, StripTags, SymbolDoSCVE, TranslateBug, UnsafeReflection, ValidationRegex, WithoutProtection, XMLDoS, YAMLParsing

== Overview ==

Controllers: 1
Models: 2
Templates: 2
Errors: 0
Security Warnings: 2

== Warning Types ==

SQL Injection: 2

== Controller Overview ==

Controller: ApplicationController
Parent: ActionController::Base
Routes: [None]

== Template Output ==

layouts/application

[Escaped Output] csrf_meta_tags
[Escaped Output] csp_meta_tag
[Escaped Output] stylesheet_link_tag("application", :media => "all", :"data-turbolinks-track" => "reload")
[Escaped Output] javascript_include_tag("application", :"data-turbolinks-track" => "reload")
[Escaped Output] yield

layouts/mailer

[Escaped Output] yield

== Warnings ==

Confidence: Medium
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: where("      name = '#{name}'\n")
File: app/models/character.rb
Line: 15

Confidence: Medium
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: where("name = '#{name}'\n")
File: app/models/character.rb
Line: 23
@presidentbeef
Copy link
Owner

Thank you for reporting, @cbortz! This should be an easy fix.

presidentbeef added a commit that referenced this issue Nov 21, 2019
Repository owner locked and limited conversation to collaborators Jan 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants