presidentbeef / brakeman

A static analysis security vulnerability scanner for Ruby on Rails applications
https://brakemanscanner.org/
Other
6.94k stars 722 forks source link

False negatives due to --skip-libs ignoring app/ files. #1839

Open kevinjacobs opened 2 months ago

kevinjacobs commented 2 months ago

Background

Brakeman version: 5.4.0 Rails version: 4.0.8 Ruby version: 3.1.2

Issue

I’d like to report some unexpected false negatives noticed when running with the --skip-libs option.

Documentation [1] states that to “To skip processing of the lib/ directory…”, one should add --skip-libs. However this results in Brakeman ignoring much of the app/ directory as well, in fact it appears that only the contents of app/models/ and app/controllers/ are included in this mode. If one wants to skip the lib/ directory, --skip-files lib/ seems to be a better approach.

I believe this is due to file type detection at [2] assuming code is “library” code unless it fits into a small number of alternative classifications. The last sentence in [3] seems to support this.

Admittedly, options.MD also includes the following:

brakeman --faster
This will disable some features, but will probably be much faster (currently it is the same as --skip-libs --no-branching). WARNING: This may cause Brakeman to miss some vulnerabilities.

But given the other mention of --skip-libs applying to lib/, a reasonable reader might assume that --no-branching is the cause for the above warning.

Reproducer:

git clone https://github.com/railstutorial/sample_app_rails_4.git && cd sample_app_rails_4
brakeman -f json --skip-libs -o skip_libs.json
brakeman -f json --skip-files lib/ -o skip_files.json
diff skip_libs.json skip_files.json

Expected results: Given that there are no warnings from lib/, both outputs should include the same warnings.

Actual results: skip_libs.json misses a warning in app/helpers/sessions_helper.rb.

If this behavior is intended (as it appears to be), the documentation should more clearly state the potential impact of running with --skip-libs.

Thanks!

[1] https://brakemanscanner.org/docs/options/ [2] https://github.com/presidentbeef/brakeman/blob/main/lib/brakeman/processors/lib/file_type_detector.rb#L16 [3] https://github.com/presidentbeef/brakeman/pull/1554

presidentbeef commented 2 months ago

Hi @kevinjacobs - you are right, thank you for reporting this. It should be equivalent to --skip-files lib/ (or maybe --skip-files /lib/) but due to the change to scan (almost) every Ruby file, it no longer matches.

presidentbeef commented 5 days ago

Should remove --skip-libs :thinking: in Brakeman 7.0 I guess.