Brakeman does not follow directory symlinks

lubert commented 9 months ago

Background

Brakeman version: 6.1 Rails version: 7.1.3 Ruby version: 3.3.0

Link to Rails application code:

I've created a minimal sample project here https://github.com/lubert/sample-rails-brakeman to demonstrate the bug

Issue

brakeman doesn't seem to follow directory symlinks. File symlinks are OK.
In the sample project I've provided,app/dependencies has 2 directories, bad_dep_symlink is a symlink to ../../bad_dependency, bad_dep_no_symlink is a copy of the same directory.
Running brakeman app from the root only flags warnings with app/dependencies/bad_dep_no_symlink, whereas I expect bad_dep_symlink to also have warnings

presidentbeef commented 8 months ago

Thank you for the sample app!

If you run Brakeman with --debug, you will see both files are parsed and processed.

However, since the files have the same content, they end up only producing one warning. This is because the file name is the very last option for comparing the "location" of two warnings (honestly not sure that ever happens). The class, method, line, etc. are all going to be the same.

Why would you want duplicate warnings for the same file..? :thinking:

lubert commented 8 months ago

Here's my output from brakeman --debug

Loading scanner...
Processing application in /Users/<redacted>/src/sample-rails-brakeman/app
Processing gems...
Parsing Gemfile
[Notice] Detected Rails 7 application
(Processing gems) Duration: 0.002202 seconds
Processing configuration...
Parsing config/environment.rb
Parsing config/application.rb
Parsing config/environments/production.rb
[Notice] Escaping HTML by default
(Processing configuration) Duration: 0.006831 seconds
Parsing files...
Parsing app/channels/application_cable/channel.rb
Parsing app/helpers/application_helper.rb
Parsing app/channels/application_cable/connection.rb
Parsing config/application.rb
Parsing app/mailers/application_mailer.rb
Parsing app/controllers/application_controller.rb
Parsing app/jobs/application_job.rb
Parsing app/models/application_record.rb
Parsing config/boot.rb
Parsing config/environment.rb
Parsing config/environments/production.rb
Parsing config/environments/development.rb
Parsing config/environments/test.rb
Parsing config/initializers/assets.rb
Parsing config/initializers/content_security_policy.rb
Parsing config/initializers/filter_parameter_logging.rb
Parsing config/initializers/inflections.rb
Parsing config/initializers/permissions_policy.rb
Parsing config/puma.rb
Parsing config/routes.rb
Parsing dependencies/bad_dep_no_symlink/bogus.rb
Parsing /Users/<redacted>/src/sample-rails-brakeman/app/app/views/layouts/application.html.erb
Parsing app/views/layouts/application.html.erb
Parsing /Users/<redacted>/src/sample-rails-brakeman/app/app/views/layouts/mailer.html.erb
Parsing app/views/layouts/mailer.html.erb
(Parsing files) Duration: 0.026345 seconds
Detecting file types...
(Detecting file types) Duration: 0.002503 seconds
Processing initializers...
(/Users/<redacted>/src/sample-rails-brakeman/app/config/initializers/assets.rb) Duration: 0.000125 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/config/initializers/filter_parameter_logging.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/config/initializers/filter_parameter_logging.rb) Duration: 5.9e-05 seconds
(Processing initializers) Duration: 0.00022 seconds
Processing libs...
Processing /Users/<redacted>/src/sample-rails-brakeman/app/app/channels/application_cable/channel.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/app/channels/application_cable/channel.rb) Duration: 9.8e-05 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/app/channels/application_cable/connection.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/app/channels/application_cable/connection.rb) Duration: 2.1e-05 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/app/helpers/application_helper.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/app/helpers/application_helper.rb) Duration: 7.0e-06 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/app/jobs/application_job.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/app/jobs/application_job.rb) Duration: 9.0e-06 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/app/mailers/application_mailer.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/app/mailers/application_mailer.rb) Duration: 1.4e-05 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/config/boot.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/config/boot.rb) Duration: 2.6e-05 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/config/environment.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/config/environment.rb) Duration: 1.0e-05 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/config/environments/development.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/config/environments/development.rb) Duration: 0.000208 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/config/environments/test.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/config/environments/test.rb) Duration: 0.000227 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/config/puma.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/config/puma.rb) Duration: 0.000116 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/config/routes.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/config/routes.rb) Duration: 1.7e-05 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/dependencies/bad_dep_no_symlink/bogus.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/dependencies/bad_dep_no_symlink/bogus.rb) Duration: 8.6e-05 seconds
(Processing libs) Duration: 0.000919 seconds
Processing routes...
Parsing config/routes.rb
(Processing routes) Duration: 0.000536 seconds
Processing templates...
Processing /Users/<redacted>/src/sample-rails-brakeman/app/app/views/layouts/application.html.erb
(/Users/<redacted>/src/sample-rails-brakeman/app/app/views/layouts/application.html.erb) Duration: 0.000175 seconds
Processing /Users/<redacted>/src/sample-rails-brakeman/app/app/views/layouts/mailer.html.erb
(/Users/<redacted>/src/sample-rails-brakeman/app/app/views/layouts/mailer.html.erb) Duration: 4.4e-05 seconds
(Processing templates) Duration: 0.000251 seconds
Processing data flow in templates...
Processing layouts/application
(layouts/application) Duration: 0.000159 seconds
Processing layouts/mailer
(layouts/mailer) Duration: 4.2e-05 seconds
(Processing data flow in templates) Duration: 0.000228 seconds
Processing models...
Processing /Users/<redacted>/src/sample-rails-brakeman/app/app/models/application_record.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/app/models/application_record.rb) Duration: 7.4e-05 seconds
(Processing models) Duration: 8.2e-05 seconds
Processing controllers...
Processing /Users/<redacted>/src/sample-rails-brakeman/app/app/controllers/application_controller.rb
(/Users/<redacted>/src/sample-rails-brakeman/app/app/controllers/application_controller.rb) Duration: 0.000179 seconds
(Processing controllers) Duration: 0.00019 seconds
Processing data flow in controllers...
Processing ApplicationController
(ApplicationController) Duration: 7.9e-05 seconds
(Processing data flow in controllers) Duration: 9.5e-05 seconds
Indexing call sites...
(Indexing call sites) Duration: 0.000211 seconds
Running checks in parallel...
 - CheckBasicAuth
 - CheckBasicAuthTimingAttack
 - CheckCrossSiteScripting
 - CheckContentTag
 - CheckCookieSerialization
 - CheckCreateWith
 - CheckCSRFTokenForgeryCVE
 - CheckDefaultRoutes
 - CheckDeserialize
 - CheckDetailedExceptions
 - CheckDigestDoS
 - CheckDynamicFinders
 - CheckEOLRails
 - CheckEOLRuby
 - CheckEscapeFunction
 - CheckEvaluation
 - CheckExecute
 - CheckFileAccess
 - CheckFileDisclosure
 - CheckFilterSkipping
 - CheckForgerySetting
 - CheckHeaderDoS
 - CheckI18nXSS
 - CheckJRubyXML
 - CheckJSONEncoding
 - CheckJSONEntityEscape
 - CheckJSONParsing
 - CheckLinkTo
 - CheckLinkToHref
 - CheckMailTo
 - CheckMassAssignment
 - CheckMimeTypeDoS
 - CheckModelAttrAccessible
 - CheckModelAttributes
 - CheckModelSerialize
 - CheckNestedAttributes
 - CheckNestedAttributesBypass
 - CheckNumberToCurrency
 - CheckPageCachingCVE
 - CheckPathname
 - CheckPermitAttributes
 - CheckQuoteTableName
 - CheckRansack
 - CheckRedirect
 - CheckRegexDoS
 - CheckRender
 - CheckRenderDoS
 - CheckRenderInline
 - CheckResponseSplitting
 - CheckRouteDoS
 - CheckSafeBufferManipulation
 - CheckSanitizeConfigCve
 - CheckSanitizeMethods
 - CheckSelectTag
 - CheckSelectVulnerability
 - CheckSend
 - CheckSendFile
 - CheckSessionManipulation
 - CheckSessionSettings
 - CheckSimpleFormat
 - CheckSingleQuotes
 - CheckSkipBeforeFilter
 - CheckSprocketsPathTraversal
 - CheckSQL
 - CheckSQLCVEs
 - CheckSSLVerify
 - CheckStripTags
 - CheckSymbolDoSCVE
 - CheckTemplateInjection
 - CheckTranslateBug
 - CheckUnsafeReflection
 - CheckUnsafeReflectionMethods
 - CheckValidationRegex
 - CheckVerbConfusion
 - CheckWeakRSAKey
 - CheckWithoutProtection
 - CheckXMLDoS
 - CheckYAMLParsing
Finding all calls to send_file()
Finding calls to strip_tags()
Finding eval-like calls
Processing eval-like calls
Finding ERB.new calls
Processing ERB.new calls
Finding system calls using ``
Finding other system calls
Processing system calls
Automatic to_json escaping is enabled.
Finding possible SQL calls on models
Finding possible SQL calls with no target
Finding possible SQL calls using constantized()
Finding calls to named_scope or scope
Processing possible SQL calls
Finding dynamic regexes
Processing dynamic regexes
Finding possible file access
Finding calls to load()
Finding calls using FileUtils
Processing found calls
Checking each controller for default routes
Automatic to_json escaping is enabled.
Checking layouts/application for XSS
Checking layouts/mailer for XSS
Checking for XSS in content_tag
Finding instances of #send
Finding all mass assignments
Processing all mass assignments
Checks finished, collecting results...
Generating report...

== Brakeman Report ==

Application Path: /Users/<redacted>/src/sample-rails-brakeman/app
Rails Version: 7.1.3
Brakeman Version: 6.1.1
Scan Date: 2024-01-31 12:37:50 -0800
Duration: 0.051255 seconds
Checks Run: 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, Ransack, 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

== Overview ==

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

== Warning Types ==

Command Injection: 1

== 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", :"data-turbo-track" => "reload")
[Escaped Output] yield

layouts/mailer

[Escaped Output] yield

== Warnings ==

Confidence: Medium
Category: Command Injection
Check: Execute
Message: Possible command injection
Code: Kernel.system("ps -o thcount #{@pid}")
File: dependencies/bad_dep_no_symlink/bogus.rb
Line: 5

I don't see the symlinked file getting processed. I've also updated the sample repo to remove the non-symlink, and there are no errors, whereas I'd expect to see the symlinked file flagged. I'm running this on a mac if that helps.

presidentbeef commented 8 months ago

Oh, I think I understand what's happening. You are trying to demonstrate linking to a directory outside of the directory Brakeman is scanning. I can see that does not work currently.

lubert commented 8 months ago

Thanks for taking a look. I'd be interested in working on a fix for this issue if you think it should be supported, but I may need a point in the right direction of what parts of the codebase to look at first.

presidentbeef commented 8 months ago

All the path mangling happens here: https://github.com/presidentbeef/brakeman/blob/main/lib/brakeman/app_tree.rb

lubert commented 8 months ago

Thank you, will take look 👍

lubert commented 8 months ago

Just submitted a PR with a proposed fix. With the fix, the debug output for the sample app shows:

== Warnings ==

Confidence: Medium
Category: Command Injection
Check: Execute
Message: Possible command injection
Code: Kernel.system("ps -o thcount #{@pid}")
File: ../bad_dependency/bogus.rb
Line: 5

presidentbeef / brakeman

Brakeman does not follow directory symlinks #1817

Background

Issue