presidentbeef / brakeman

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

Fix compatibility with `--enable-frozen-string-literal` #1855

Closed casperisfine closed 4 months ago

casperisfine commented 5 months ago

Since Ruby 2.3, Ruby can be started with this options, and it's expected to become the default in Ruby 4.0 (no release date yet).

This commit fixes the callsites that assume string literal are mutable.

Note however that a large part of the test suite is still failing, because of two dependencies:

dryrunsecurity[bot] commented 5 months ago

Hi there :wave:, @dryrunsecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Server-Side Request Forgery Analyzer :white_check_mark: 0 findings
Configured Codepaths Analyzer :white_check_mark: 0 findings
Authn/Authz Analyzer :grey_exclamation: 1 finding
Secrets Analyzer :white_check_mark: 0 findings
IDOR Analyzer :white_check_mark: 0 findings
SQL Injection Analyzer :white_check_mark: 0 findings
Sensitive Files Analyzer :white_check_mark: 0 findings

[!Note] :green_circle: Risk threshold not exceeded.

Change Summary (click to expand) The following is a summary of changes in this pull request made by me, your security buddy :robot:. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective. **Summary:** The changes in this pull request are primarily focused on improving the functionality and usability of the Brakeman security scanner for Ruby on Rails applications. The changes span several files and cover a range of improvements, including: 1. **Command-line Option Handling**: The changes to the `Brakeman::Options` module aim to improve the handling and consistency of command-line options for the Brakeman tool. 2. **Erubis Template Handling**: The changes to the various `Brakeman::*Erubis` classes focus on enhancing the handling of Erubis templates, including the integration of a custom `Brakeman::ErubisPatch` module to address compatibility and security-related concerns. 3. **Report Generation**: The changes to the `Brakeman::Report` classes improve the formatting, readability, and efficiency of the security reports generated by Brakeman. 4. **User Input Handling**: The changes to the `Brakeman::Warning` class focus on improving the handling and formatting of user input to ensure consistent and reliable behavior when processing and displaying security warnings. While these changes do not directly address any known security vulnerabilities, they contribute to the overall security and reliability of the Brakeman tool, which is an important application security tool for Ruby on Rails developers. By improving the usability, performance, and robustness of the Brakeman scanner, these changes can indirectly enhance the security of the applications being analyzed. **Files Changed:** 1. `lib/brakeman/options.rb`: The changes focus on improving the handling of command-line options for the Brakeman tool, such as ensuring consistent naming of check options and simplifying the output format option. 2. `lib/brakeman/app_tree.rb`: The changes involve a minor refactoring of the `regex_for_paths` method, which is used to generate a regular expression pattern for filtering files and directories during the Brakeman scan. 3. `gem_common.rb`: The changes update the version constraint for the "slim" gem dependency, likely to ensure compatibility with the latest version of the gem. 4. `lib/brakeman/parsers/erubis_patch.rb`: The changes introduce a new `Brakeman::ErubisPatch` module to address compatibility issues with frozen string literals in the Erubis library. 5. `lib/brakeman/parsers/rails2_erubis.rb`: The changes incorporate the `Brakeman::ErubisPatch` module into the `Brakeman::ScannerErubis` class, suggesting further enhancements to the Erubis handling in the Brakeman scanner. 6. `lib/brakeman/parsers/rails2_xss_plugin_erubis.rb`: The changes focus on improving the handling of dynamic content within Erubis templates, specifically addressing potential XSS vulnerabilities. 7. `lib/brakeman/processors/alias_processor.rb`: The changes are a minor optimization to the `process_array_join` method, improving the performance of the array-to-string conversion process. 8. `lib/brakeman/report/report_markdown.rb`: The changes enhance the generation of Markdown-formatted security reports, improving the readability and usability of the Brakeman output. 9. `lib/brakeman/report/report_text.rb`: The changes are minor optimizations to the text-based report generation, focusing on improving the performance of string concatenation operations. 10. `lib/brakeman/parsers/rails3_erubis.rb`: The changes modify the `Brakeman::Rails3Erubis` class to integrate the `Brakeman::ErubisPatch` module and customize the handling of Erubis templates. 11. `lib/brakeman/report/report_table.rb`: The changes are minor optimizations to the string concatenation operations in the `Brakeman::Report::Table` class. 12. `lib/brakeman/warning.rb`: The changes focus on improving the handling of user input and code formatting in the `Brakeman::Warning` class, ensuring consistent and reliable behavior when processing and displaying security warnings.

Powered by DryRun Security

casperisfine commented 4 months ago

About the dependencies

ruby2ruby: https://github.com/seattlerb/ruby2ruby/pull/58

Looks like @zenspider merged a fix: https://github.com/seattlerb/ruby2ruby/commit/3d5966e30dfe3a04b05e5a6796f6b5d944696001, not sure when we'll get a release thought.

  • erubis: (this one is abandoned so can hardly be fixed).

I could monkey patch the one erubis method that isn't compatible, not sure if that'd be OK with you.

zenspider commented 4 months ago

I can have a release of r2r out this week.

casperisfine commented 4 months ago

I could monkey patch the one erubis method that isn't compatible, not sure if that'd be OK with you.

Ok, so I did it without monkey patch, I just include a module in all of Brakeman's erubis based parsers to change the one method.

zenspider commented 4 months ago

r2r is out

dryrunsecurity[bot] commented 4 months ago

DryRun Security Summary

The provided code changes cover a variety of updates and improvements to the Brakeman security scanner for Ruby on Rails applications, including dependency version updates, CircleCI configuration improvements, refactoring and optimizations, and integration of specific features, all focused on improving the functionality, performance, and security of the Brakeman tool.

Expand for full summary
**Summary:** The provided code changes cover a variety of updates and improvements to the Brakeman security scanner for Ruby on Rails applications. The changes include: 1. Dependency version updates to address potential security vulnerabilities. 2. Improvements to the CircleCI configuration to enable better testing and code quality practices. 3. Refactoring and optimizations to various Brakeman components, such as the command-line options parser, the file path matching logic, and the report generation functionality. 4. Integration and handling of specific features, like the Rails 2 XSS plugin and frozen string literals, to ensure comprehensive security analysis. Overall, the changes appear to be focused on improving the functionality, performance, and security of the Brakeman tool. While none of the individual changes introduce obvious security vulnerabilities, the reviewer should carefully consider the potential impact of each change, especially those related to user input handling, file path management, and report generation, to ensure the ongoing security and reliability of the Brakeman scanner. **Files Changed:** 1. `gem_common.rb`: The changes update the version constraints for the `ruby2ruby` and `slim` dependencies, which is a positive security practice to address potential vulnerabilities in the dependencies. 2. `.circleci/config.yml`: The changes in the CircleCI configuration file enable the use of the frozen string literal feature and the debug version of this feature, which can help identify potential issues related to string literals in the codebase. 3. `lib/brakeman/options.rb`: The changes focus on improving the consistency and readability of the Brakeman command-line options, without any direct impact on the security analysis performed by the tool. 4. `lib/brakeman/app_tree.rb`: The changes are a minor refactoring to improve the readability and maintainability of the regular expression used for matching file paths, without introducing any obvious security concerns. 5. `lib/brakeman/parsers/rails2_erubis.rb`: The changes are focused on improving the functionality and performance of the Brakeman security scanner, without raising any immediate security concerns. 6. `lib/brakeman/parsers/erubis_patch.rb`: The patch ensures the compatibility of the `erubis` library with frozen string literals in Ruby, which is an important change to maintain the reliability of the Brakeman tool. 7. `lib/brakeman/parsers/rails2_xss_plugin_erubis.rb`: The changes demonstrate Brakeman's effort to integrate with the Rails 2 XSS plugin and handle the specific behavior and functionality of Erubis templates, which is crucial for accurate security analysis. 8. `lib/brakeman/parsers/rails3_erubis.rb`: The changes are focused on improving the handling of Erubis templates, which is an important component of the Brakeman security scanner. 9. `lib/brakeman/processors/alias_processor.rb`: The changes are a performance optimization and do not introduce any obvious security vulnerabilities. 10. `lib/brakeman/report/report_markdown.rb`: The changes focus on enhancing the usability and readability of the Brakeman report, which can help developers better understand and address the identified security issues. 11. `lib/brakeman/report/report_table.rb`: The changes are primarily focused on improving the efficiency and performance of the report generation process, without introducing any obvious security vulnerabilities. 12. `lib/brakeman/warning.rb`: The changes are a minor improvement to the `Brakeman::Warning` class and do not raise any significant security concerns. 13. `lib/brakeman/report/report_tabs.rb`: The removal of the sanitization of the `warning_type` field may introduce potential security concerns, such as injection attacks or reduced readability of the report. The reviewer should carefully evaluate the impact of this change. 14. `lib/brakeman/report/report_text.rb`: The changes are a performance optimization and do not directly impact the security of the Brakeman tool or the applications it analyzes.

Code Analysis

We ran 7 analyzers against 14 files and 1 analyzer had findings. 6 analyzers had no findings.

Analyzer Findings
Authn/Authz Analyzer 1 finding

Riskiness

:green_circle: Risk threshold not exceeded.

View PR in the DryRun Dashboard.

casperisfine commented 4 months ago

thanks @zenspider I updated the dependency, now the entire test suite passes with --enable-frozen-string-literal.

presidentbeef commented 4 months ago

Took some time to get all the testing done, but other than one ruby2ruby output that changed (will follow up on that), looks good! Thanks for your contribution!