Skip to content

Ruby: Add rb/log-injection query #10008

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

Merged
merged 4 commits into from
Aug 17, 2022
Merged

Conversation

alexrford
Copy link
Contributor

Detects some cases where a logging call may allow a malicious user to forge new log entries. The concrete implementation is mostly identical to the JS version of this query.

@github-actions
Copy link
Contributor

QHelp previews:

ruby/ql/src/queries/security/cwe-117/LogInjection.qhelp

Log injection

If unsanitized user input is written to a log entry, a malicious user may able to forge new log entries.

Forgery can occur if a user provides some input with characters that are interpreted when the log output is displayed. If the log is displayed as a plain text file, then new line characters can be used by a malicious user. If the log is displayed as HTML, then arbitrary HTML may be included to spoof log entries.

Recommendation

User input should be suitably sanitized before it is logged. Suitable means of sanitization depend on how the log entries will be displayed or consumed.

If the log entries are in plain text then line breaks should be removed from user input, using String#gsub or similar. Care should also be taken that user input is clearly marked in log entries.

For log entries that will be displayed in HTML, user input should be HTML-encoded before being logged, to prevent forgery and other forms of HTML injection.

Example

In the example, a username, provided by the user, is logged using `Logger#info`.

In the first case, it is logged without any sanitization. If a malicious user provides `username=Guest%0a[INFO]+User:+Admin%0a` as a username parameter, the log entry will be split in two different lines, where the second line will be `[INFO]+User:+Admin`.

require 'logger'

class UsersController < ApplicationController
  def login
    logger = Logger.new STDOUT
    username = params[:username]

    # BAD: log message constructed with unsanitized user input
    logger.info "attempting to login user: " + username

    # ... login logic ...
  end
end

In the second example, String#gsub is used to ensure no line endings are present in the user input.

require 'logger'

class UsersController < ApplicationController
  def login
    logger = Logger.new STDOUT
    username = params[:username]

    # GOOD: log message constructed with unsanitized user input
    sanitized_username = username.gsub("\n", "")
    logger.info "attempting to login user: " + sanitized_username

    # ... login logic ...
  end
end

References

@alexrford alexrford marked this pull request as ready for review August 10, 2022 16:27
@alexrford alexrford requested a review from a team as a code owner August 10, 2022 16:27
@alexrford
Copy link
Contributor Author

Having some issues starting a DCA run right now - I'll trigger one tomorrow.

Copy link
Contributor

@hmac hmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks great!

@alexrford alexrford merged commit d4d6657 into github:main Aug 17, 2022
@alexrford alexrford deleted the rb/log-injection branch August 17, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants