Ruby: add `rb/sensitive-get-query` query

[alexrford]

trafficstars

Finds cases where an HTTP GET request handler takes sensitive input, such as a password or other credential, from the query string of the request. The bulk of this PR concerns porting SensitiveNode to Ruby.

This uses for HTTP::Server::RequestHandlers, which is currently implemented for Rails ActionController and the ruby graphql gem. In the case of GraphQL, both POST and GET requests are by default both accepted and handled uniformly - the string getAnHttpMethod() implementation is trivial here as I couldn't find a documented way to change this behaviour.

[github-actions[bot]]

QHelp previews:

ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.qhelp

Sensitive data read from GET request

Sensitive information such as passwords should not be transmitted within the query string of the requested URL. Sensitive information within URLs may be logged in various locations, including the user's browser, the web server, and any proxy servers between the two endpoints. URLs may also be displayed on-screen, bookmarked or emailed around by users. They may be disclosed to third parties via the Referer header when any off-site links are followed. Placing sensitive information into the URL therefore increases the risk that it will be captured by an attacker.

Recommendation

Use HTTP POST to send sensitive information as part of the request body; for example, as form data.

Example

The following example shows two route handlers that both receive a username and a password. The first receives this sensitive information from the query parameters of a GET request, which is transmitted in the URL. The second receives this sensitive information from the request body of a POST request.

Rails.application.routes.draw do
  get "users/login", to: "#login_get" # BAD: sensitive data transmitted through query parameters
  post "users/login", to: "users#login_post" # GOOD: sensitive data transmitted in the request body
end

class UsersController < ActionController::Base
  def login_get
    password = params[:password]
    authenticate_user(params[:username], password)
  end

  def login_post
    password = params[:password]
    authenticate_user(params[:username], password)
  end

  private
  def authenticate_user(username, password)
    # ... authenticate the user here
  end
end

References

• CWE: CWE-598: Use of GET Request Method with Sensitive Query Strings
• PortSwigger (Burp): Password Submitted using GET Method
• OWASP: Information Exposure through Query Strings in URL
• Common Weakness Enumeration: CWE-598.

[alexrford]

Finally got around to updating this PR. There are a couple of main changes:

• Switched to using full taint tracking rather than local flow. I've kicked off a DCA run to see how this affects results/performance. I've used the Query/Customization pattern to implement this, which leaves us with an unfortunately named SensitiveGetQueryQuery.qll library. I converted the query to a path-problem as part of this change, which is not consistent with the JS version, but it seems worth using the path graph if we have it.
• Fixed an issue where data from headers in the request headers were also treated as if they were transmitted as plaintext, where the other versions of this query are correctly limited to just query params. This required adding a string RequestInputAccess#getKind() predicate.

[alexrford]

I've rolled the taint tracking changes back since this had a fairly large performance impact and the results that it found should all be picked up with local flow anyway.

[alexrford]

I've rolled the taint tracking changes back since this had a fairly large performance impact and the results that it found should all be picked up with local flow anyway.

I spoke too soon - the performance impact seems to be due to something else. I'll look into this properly tomorrow.

[alexrford]

I think the bulk of the performance impact (~3.5% increase in analysis time) is from computing the new SensitiveNodes. I've not been able to make any significant improvements here. I'm conflicted over if this regression is acceptable - the SensitiveNode supports 5 different queries in JS of which I think 4 can be ported to Ruby whilst reusing this class. As it stands we only use this class for rb/sensitive-get-query in this PR.

[alexrford]

I'll update this tomorrow to reflect the kind changes from the other PR before running DCA again.

[alexrford]

Updated to address review comments. DCA has a couple of FPs that are interesting, as they look to be safe by virtue of appearing in an if statement body that checks for request.post? or request.get?.

[alexrford]

LGTM, perhaps we should implement some sanitizers/barriers for if request.post? checks.

Do you think that this is a blocker for the current PR?

[aibaars]

LGTM, perhaps we should implement some sanitizers/barriers for if request.post? checks.

Do you think that this is a blocker for the current PR?

No problem ;-)