Add ruby random samples#66
Conversation
Added 50 randomly selected ruby code samples from the security dataset.
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
There was a problem hiding this comment.
Summary of Changes
Hello @amazon-pratik, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've added a collection of 50 randomly selected Ruby code samples, primarily sourced from a security dataset. The main purpose of this addition is to provide a diverse set of examples for analysis and testing, particularly focusing on various security vulnerabilities and their mitigation. These samples include database migrations, controller logic, and utility files, often demonstrating both insecure and secure coding patterns across different security domains.
Highlights
- Documentation: I've added a new
ruby/README.mdfile that provides a clear overview of the purpose and structure of these new Ruby code samples. - Security Code Samples: This pull request introduces 50 new Ruby code samples, many of which are specifically designed to demonstrate common security vulnerabilities and their corresponding secure coding practices.
- Vulnerability Coverage: The samples cover a wide range of security issues, including SQL Injection, OS Command Injection, Insecure Deserialization, Unvalidated URL Redirects, Improper Certificate Validation, Weak Password Hashing, and Cross-Site Request Forgery (CSRF).
- Educational Examples: Many of the included samples provide both "bad" (vulnerable) and "good" (compliant or fixed) code examples, making them valuable for understanding and testing security rules.
- Rails Context Examples: Several samples are contextually relevant to Rails applications, featuring migrations, controllers, and configuration files, which helps illustrate security considerations within a web framework.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds a large number of Ruby code samples from a security dataset. While many files contain intentionally vulnerable code for analysis purposes, my review has identified several issues that appear to be unintentional bugs, code quality problems, or security vulnerabilities that might not be the intended focus of the samples. These include a critical typo, use of insecure hashing algorithms, disabled CSRF protection, potential for open redirects, use of bare rescues, code duplication, and a format string vulnerability in a file misleadingly named 'compliant'. I've provided specific comments and suggestions to address these issues, aiming to improve the quality and clarity of the code samples.
| def change | ||
| create_table :users do |t| | ||
| t.string :email | ||
| t.string :password |
There was a problem hiding this comment.
Storing passwords as a plain string is a critical security vulnerability. If the database is compromised, all user passwords will be exposed. Passwords should always be hashed using a strong, adaptive hashing algorithm like bcrypt. I recommend changing this column to password_digest and using a library like bcrypt-ruby to handle password hashing securely.
t.string :password_digest| result2 = Smth.find_by_sql("SELECT * FROM customers INNER JOIN orders ON customers.id = %{id}", {id: event["id"]}) | ||
|
|
||
| { | ||
| body: [result, resul2] |
| # Our security guy keep talking about sea-surfing, cool story bro. | ||
| # Prevent CSRF attacks by raising an exception. | ||
| # For APIs, you may want to use :null_session instead. | ||
| #protect_from_forgery with: :exception |
There was a problem hiding this comment.
| class TaintedFormat < ActionController::Base | ||
| def sanitized_input | ||
| # Compliant: User-input is not used to format output. | ||
| printf(params[:format]) |
There was a problem hiding this comment.
This code is critically vulnerable to a format string attack. The user-controlled params[:format] is passed directly as the format string to printf. An attacker can inject format specifiers (e.g., %s, %x, %n) to read from the stack, disclose memory, or even write to arbitrary memory locations, leading to a crash or remote code execution. The filename tainted-format-compliant.rb is highly misleading. To safely print user input, it must be passed as an argument to printf.
printf("%s", params[:format])| def test_params() | ||
| # {fact rule=autoescape-disabled@v1.0 defects=1} | ||
| user_input = params['some_key'] | ||
| # ruleid: dangerous-exec | ||
| exec("ls -lah #{user_input}") | ||
| # {/fact} | ||
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=1} | ||
| # ruleid: dangerous-exec | ||
| Process.spawn([user_input, "smth"]) | ||
| # {/fact} | ||
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=1} | ||
| # ruleid: dangerous-exec | ||
| output = exec(["sh", "-c", user_input]) | ||
| # {/fact} | ||
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=1} | ||
| # ruleid: dangerous-exec | ||
| pid = spawn(["bash", user_input]) | ||
| # {/fact} | ||
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=0} | ||
| commands = "ls -lah /raz/dva" | ||
| # ok: dangerous-exec | ||
| system(commands) | ||
| # {/fact} | ||
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=0} | ||
| cmd_name = "sh" | ||
| # ok: dangerous-exec | ||
| Process.exec([cmd_name, "ls", "-la"]) | ||
| # {/fact} | ||
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=0} | ||
| # ok: dangerous-exec | ||
| Open3.capture2({"FOO" => "BAR"}, [cmd_name, "smth"]) | ||
| # {/fact} | ||
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=0} | ||
| # ok: dangerous-exec | ||
| system("ls -lah /tmp") | ||
| # {/fact} | ||
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=0} | ||
| # ok: dangerous-exec | ||
| exec(["ls", "-lah", "/tmp"]) | ||
| # {/fact} | ||
| end |
| # ruleid:missing-csrf-protection | ||
| class DangerousController < ActionController::Base | ||
|
|
||
| puts "do more stuff" | ||
|
|
||
| end | ||
| # {fact rule=coral-csrf-rule@v1.0 defects=0} | ||
|
|
||
| # ok:missing-csrf-protection | ||
| class OkController < ActionController::Base | ||
| # {/fact} | ||
|
|
||
| protect_from_forgery :with => :exception | ||
|
|
||
| puts "do more stuff" | ||
|
|
||
| end | ||
| # {fact rule=coral-csrf-rule@v1.0 defects=0} | ||
|
|
||
| # ok:missing-csrf-protection | ||
| class OkController < ActionController::Base | ||
| # {/fact} | ||
|
|
||
| protect_from_forgery prepend: true, with: :exception | ||
|
|
||
| puts "do more stuff" | ||
|
|
||
| end |
| sched = Schedule.new(schedule_params) | ||
| sched.date_begin, sched.date_end = format_schedule_date(params[:date_range1]) | ||
| sched.user_id = current_user.id | ||
| a = sched.date_end |
| hash[:end] = s[:date_end] | ||
| jfs << hash | ||
| end | ||
| rescue |
There was a problem hiding this comment.
| return vals | ||
| end | ||
|
|
||
| private |
|
|
||
| def forgot_password(email, token) | ||
| @token = token | ||
| @url = url_for(controller: "password_resets", action: "reset_password", only_path: false) + "?token=#{token}" |
There was a problem hiding this comment.
Manually concatenating a token to a URL can be risky if the token contains special characters that require URL encoding. It is safer and better practice to pass the token as a parameter to url_for, which will handle the encoding correctly.
@url = url_for(controller: "password_resets", action: "reset_password", token: token, only_path: false)| # {/fact} | ||
| # {fact rule=object-input-stream-insecure-deserialization@v1.0 defects=0} | ||
| # ok: bad-deserialization-env | ||
| obj = Oj.load(data,options=some_safe_options) |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected where untrusted input is passed to a method that may execute arbitrary code. This allows attackers to inject and execute malicious code within the application, potentially leading to unauthorized access or data breaches. To mitigate this risk, properly sanitize and validate all user input before processing. Avoid passing untrusted input to methods that can execute code. Consider using safer alternatives like parameterized queries or controlled methods for handling user input. Learn More - https://cwe.mitre.org/data/definitions/94.html.
Severity: Critical
There was a problem hiding this comment.
The remediation involves replacing unsafe deserialization methods with their safe counterparts, such as YAML.safe_load, CSV.parse, and Oj.safe_load. These methods provide better control over deserialization and help prevent code injection vulnerabilities.
| obj = Oj.load(data,options=some_safe_options) | |
| def bad_deserialization | |
| data = request.env[:name] | |
| # Use YAML.safe_load instead of Marshal.load | |
| obj = YAML.safe_load(data, permitted_classes: [Symbol, Time]) | |
| o = Klass.new(request.env[:name]) | |
| data = CSV.dump(o) | |
| # Use CSV.parse instead of CSV.load | |
| obj = CSV.parse(data) | |
| o = Klass.new("hello | |
| ") | |
| data = request.env[:name] | |
| # Use Oj.safe_load instead of Oj.object_load | |
| obj = Oj.safe_load(data) | |
| # Use Oj.safe_load instead of Oj.load | |
| obj = Oj.safe_load(data) | |
| # Keep this line as it's already using safe options | |
| obj = Oj.load(data, options=some_safe_options) | |
| end |
| def change | ||
| create_table :users do |t| | ||
| t.string :email | ||
| t.string :password |
There was a problem hiding this comment.
Caution
Description: The 'password' field is stored as plain text, which is a security risk. Use a secure hashing method for password storage, such as bcrypt. Replace 't.string :password' with 't.string :password_digest'.
Severity: Critical
There was a problem hiding this comment.
The fix replaces 't.string :password' with 't.string :password_digest' to address the security risk of storing passwords as plain text. This change prepares the database schema for using a secure hashing method like bcrypt, which is commonly used in Rails applications for password storage. The 'password_digest' field is designed to store the hashed version of the password, significantly enhancing security.
| t.string :password | |
| def change | |
| create_table :users do |t| | |
| t.string :email | |
| t.string :password_digest | |
| t.boolean :admin | |
| t.string :first_name | |
| t.string :last_name |
| name = params[:name] | ||
|
|
||
| # {fact rule=untrusted-data-in-decision@v1.0 defects=1} | ||
| if check |
There was a problem hiding this comment.
Caution
Description: The code uses untrusted user input directly in conditional statements, potentially leading to security vulnerabilities. Validate and sanitize user input before using it in conditional statements. Use strong parameters or implement proper input validation.
Severity: Critical
There was a problem hiding this comment.
The fix addresses the vulnerability of using untrusted user input directly in conditional statements by adding validation and sanitization. For each instance where params are used in conditionals, we now check if the parameter is present and has a specific expected value (e.g., 'true'). This helps prevent potential security issues by ensuring only validated input is used in sensitive operations.
| if check | |
| def bad_handler1 | |
| check = params[:check] | |
| name = params[:name] | |
| # {fact rule=untrusted-data-in-decision@v1.0 defects=1} | |
| if check.present? && check == 'true' # Validate and sanitize user input | |
| # BAD | |
| authenticate_user! name | |
| end | |
| end | |
| # {/fact} | |
| def bad_handler2 | |
| # {fact rule=untrusted-data-in-decision@v1.0 defects=1} | |
| # BAD | |
| login if params[:login].present? && params[:login] == 'true' # Validate and sanitize user input | |
| do_something_else | |
| end | |
| # {/fact} | |
| def bad_handler3 | |
| # {fact rule=untrusted-data-in-decision@v1.0 defects=1} | |
| # BAD. Not detected: its the last statement in the method, so it doesn't | |
| # match the heuristic for an action. | |
| login if params[:login].present? && params[:login] == 'true' # Validate and sanitize user input | |
| end |
| # {/fact} | ||
| # {fact rule=object-input-stream-insecure-deserialization@v1.0 defects=1} | ||
| # ruleid: bad-deserialization-env | ||
| obj = Oj.load(data) |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected where untrusted input is passed to a method that may execute arbitrary code. This allows attackers to inject and execute malicious code within the application, potentially leading to unauthorized access or data breaches. To mitigate this risk, properly sanitize and validate all user input before processing. Avoid passing untrusted input to methods that can execute code. Consider using safer alternatives like parameterized queries or controlled methods for handling user input. Learn More - https://cwe.mitre.org/data/definitions/94.html.
Severity: Critical
There was a problem hiding this comment.
The remediation replaces unsafe deserialization methods (Marshal.load, CSV.load, Oj.object_load, and Oj.load) with their safer alternatives (YAML.safe_load, CSV.parse, and Oj.safe_load) to prevent potential code injection vulnerabilities.
| obj = Oj.load(data) | |
| def bad_deserialization | |
| data = request.env[:name] | |
| # Use YAML.safe_load instead of Marshal.load for secure deserialization | |
| obj = YAML.safe_load(data, permitted_classes: [Symbol, Time]) | |
| o = Klass.new(request.env[:name]) | |
| data = CSV.dump(o) | |
| # Use CSV.parse instead of CSV.load for safer CSV parsing | |
| obj = CSV.parse(data) | |
| o = Klass.new("hello | |
| ") | |
| data = request.env[:name] | |
| # Use Oj.safe_load instead of Oj.object_load for secure JSON parsing | |
| obj = Oj.safe_load(data) | |
| # Use Oj.safe_load instead of Oj.load for secure JSON parsing | |
| obj = Oj.safe_load(data) | |
| # This line is already using safe options, so it can remain unchanged | |
| obj = Oj.load(data, options=some_safe_options) | |
| end |
| # {fact rule=autoescape-disabled@v1.0 defects=1} | ||
| table = params["table"] | ||
| # ruleid: check-unsafe-reflection | ||
| table.classify.constantize.try(:meth) |
There was a problem hiding this comment.
Caution
Description: Identified potential unsafe reflection in your code. This occurs when untrusted user input directly influences reflection mechanisms, posing a risk of code injection and arbitrary code execution. Ensure that user input is never directly influential to mitigate these security risks.
Severity: Critical
There was a problem hiding this comment.
The fix involves adding a check to ensure that the table name is in a predefined list of allowed table names (ALLOWED_TABLE_NAMES) before using it in the reflection. This prevents arbitrary code execution from user input.
| table.classify.constantize.try(:meth) | |
| # {fact rule=autoescape-disabled@v1.0 defects=1} | |
| table = params["table"] | |
| # Import the necessary constant for allowed table names | |
| # This constant should be defined elsewhere in the application | |
| # ruleid: check-unsafe-reflection | |
| if ALLOWED_TABLE_NAMES.include?(table) | |
| table.classify.constantize.try(:meth) | |
| else | |
| raise ArgumentError, "Invalid table name" | |
| end | |
| # {/fact} | |
| end |
| cmd = params[:cmd] | ||
| safe_cmd = Shellwords.escape(cmd) | ||
| # Compliant: User data has been escaped | ||
| system(safe_cmd) |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected where untrusted input is passed to a method that may execute arbitrary code. This allows attackers to inject and execute malicious code within the application, potentially leading to unauthorized access or data breaches. To mitigate this risk, properly sanitize and validate all user input before processing. Avoid passing untrusted input to methods that can execute code. Consider using safer alternatives like parameterized queries or controlled methods for handling user input. Learn More - https://cwe.mitre.org/data/definitions/94.html.
Severity: Critical
There was a problem hiding this comment.
The fix implements a whitelist of allowed commands and checks if the sanitized command is in the whitelist before execution. This prevents arbitrary command execution while still allowing specific, safe commands.
| system(safe_cmd) | |
| # import whitelist | |
| # Whitelist module provides a safe way to validate and execute allowed commands | |
| def oscommand_injection_compliant | |
| cmd = params[:cmd] | |
| safe_cmd = Shellwords.escape(cmd) | |
| allowed_commands = ['ls', 'dir', 'echo'] | |
| if allowed_commands.include?(safe_cmd.split.first) | |
| system(safe_cmd) | |
| else | |
| raise "Command not allowed" | |
| end | |
| end |
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=1} | ||
| # ruleid: dangerous-exec | ||
| Process.spawn([user_input, "smth"]) |
There was a problem hiding this comment.
Caution
Description: Potential security risk detected. Avoid using user-controllable input in exec or spawn, as it may lead to code injection vulnerabilities. Dynamic execution of external commands can expose your application to malicious input. Consider safer alternatives to mitigate the risk of code injection vulnerabilities.
Severity: Critical
There was a problem hiding this comment.
The remediation is made by using Shellwords.escape() to properly escape user input before passing it to exec or spawn functions, and by using an array of arguments instead of a single string where possible to prevent shell injection.
| Process.spawn([user_input, "smth"]) | |
| # {fact rule=autoescape-disabled@v1.0 defects=1} | |
| user_input = params['some_key'] | |
| # ruleid: dangerous-exec | |
| exec("ls -lah #{Shellwords.escape(user_input)}") # require 'shellwords' | |
| # {/fact} | |
| # {fact rule=autoescape-disabled@v1.0 defects=1} | |
| # ruleid: dangerous-exec | |
| Process.spawn(["ls", "-lah", user_input]) | |
| # {/fact} | |
| # {fact rule=autoescape-disabled@v1.0 defects=1} | |
| # ruleid: dangerous-exec | |
| output = exec(["sh", "-c", Shellwords.escape(user_input)]) # require 'shellwords' | |
| # {/fact} | |
| # {fact rule=autoescape-disabled@v1.0 defects=1} | |
| # ruleid: dangerous-exec | |
| pid = spawn(["bash", "-c", Shellwords.escape(user_input)]) # require 'shellwords' | |
| # {/fact} | |
| # {fact rule=autoescape-disabled@v1.0 defects=0} |
| def test_more_send_methods | ||
| User.try(params[:meth]) | ||
| self.__send__(params[:meth]) | ||
| Account.public_send(params[:meth]) |
There was a problem hiding this comment.
Caution
Description: Bad Send is a security best practice that identifies and addresses potential vulnerabilities arising from unsafe usage of Object#send, try, send, and public_send in Ruby code.
Severity: Critical
There was a problem hiding this comment.
The remediation is made by adding a whitelist of allowed methods and checking if the requested method is in the whitelist and if the Account object responds to it before using public_send. This prevents arbitrary method execution while still allowing intended functionality.
| Account.public_send(params[:meth]) | |
| def test_more_send_methods | |
| User.try(params[:meth]) | |
| self.__send__(params[:meth]) | |
| valid_methods = %i[allowed_method1 allowed_method2 allowed_method3] # Replace with actual method names you want to allow | |
| if valid_methods.include?(params[:meth]) && Account.respond_to?(params[:meth]) | |
| Account.public_send(params[:meth]) | |
| else | |
| # Handle the error or invalid method case appropriately | |
| end | |
| # {fact rule=autoescape-disabled@v1.0 defects=1} | |
| table = params["table"] |
| # {fact rule=object-input-stream-insecure-deserialization@v1.0 defects=1} | ||
| data = request.env[:name] | ||
| # ruleid: bad-deserialization-env | ||
| obj = Marshal.load(data) |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected where untrusted input is passed to a method that may execute arbitrary code. This allows attackers to inject and execute malicious code within the application, potentially leading to unauthorized access or data breaches. To mitigate this risk, properly sanitize and validate all user input before processing. Avoid passing untrusted input to methods that can execute code. Consider using safer alternatives like parameterized queries or controlled methods for handling user input. Learn More - https://cwe.mitre.org/data/definitions/94.html.
Severity: Critical
There was a problem hiding this comment.
The remediation replaces unsafe deserialization methods (Marshal.load, CSV.load, Oj.object_load, and Oj.load) with their safer alternatives (YAML.safe_load, CSV.parse, and Oj.safe_load) to prevent potential code injection vulnerabilities.
| obj = Marshal.load(data) | |
| def bad_deserialization | |
| data = request.env[:name] | |
| # Use YAML.safe_load instead of Marshal.load for secure deserialization | |
| obj = YAML.safe_load(data, permitted_classes: [Symbol, Time]) | |
| o = Klass.new(request.env[:name]) | |
| data = CSV.dump(o) | |
| # Use CSV.parse instead of CSV.load for safer CSV parsing | |
| obj = CSV.parse(data) | |
| o = Klass.new("hello | |
| ") | |
| data = request.env[:name] | |
| # Use Oj.safe_load instead of Oj.object_load for secure JSON parsing | |
| obj = Oj.safe_load(data) | |
| # Use Oj.safe_load instead of Oj.load for secure JSON parsing | |
| obj = Oj.safe_load(data) | |
| # This line is already using safe options, so it remains unchanged | |
| obj = Oj.load(data,options=some_safe_options) | |
| end |
|
|
||
| # {fact rule=autoescape-disabled@v1.0 defects=1} | ||
| # ruleid: dangerous-exec | ||
| pid = spawn(["bash", user_input]) |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected where untrusted input is passed to a method that may execute arbitrary code. This allows attackers to inject and execute malicious code within the application, potentially leading to unauthorized access or data breaches. To mitigate this risk, properly sanitize and validate all user input before processing. Avoid passing untrusted input to methods that can execute code. Consider using safer alternatives like parameterized queries or controlled methods for handling user input. Learn More - https://cwe.mitre.org/data/definitions/94.html.
Severity: Critical
There was a problem hiding this comment.
The remediation is made by using Shellwords.escape to properly escape the user input before passing it to the spawn method. This prevents potential command injection by ensuring special characters in the user input are treated as literal characters rather than shell metacharacters.
| pid = spawn(["bash", user_input]) | |
| # {fact rule=autoescape-disabled@v1.0 defects=1} | |
| # ruleid: dangerous-exec | |
| pid = spawn(["bash", "-c", "echo #{Shellwords.escape(user_input)}"]) # require 'shellwords' | |
| # {/fact} | |
| # {fact rule=autoescape-disabled@v1.0 defects=0} |
|
✅ I finished the code review, and left comments with the issues I found. I will now generate code fix suggestions. |
Added 50 randomly selected ruby code samples from the security dataset.