Skip to content
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

Ruby: Add Unsafe HMAC Comparison Query. #13825

Merged
merged 19 commits into from
Sep 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions ruby/ql/src/experimental/cwe-208/UnsafeHmacComparison.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/**
* @name Unsafe HMAC Comparison
* @description An HMAC is being compared using the equality operator. This may be vulnerable to a cryptographic timing attack
* because the equality operation does not occur in constant time."
* @kind path-problem
* @problem.severity error
* @security-severity 6.0
* @precision high
* @id rb/unsafe-hmac-comparison
* @tags security
* external/cwe/cwe-208
*/

private import codeql.ruby.AST
boveus marked this conversation as resolved.
Show resolved Hide resolved
private import codeql.ruby.DataFlow
private import codeql.ruby.ApiGraphs

private class OpenSslHmacSource extends DataFlow::Node {
OpenSslHmacSource() {
exists(API::Node hmacNode | hmacNode = API::getTopLevelMember("OpenSSL").getMember("HMAC") |
this = hmacNode.getAMethodCall(["hexdigest", "to_s", "digest", "base64digest"])
or
this = hmacNode.getAnInstantiation()
)
}
}

private module UnsafeHmacComparison {
private module Config implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof OpenSslHmacSource }

// Holds if a given sink is an Equality Operation (== or !=)
predicate isSink(DataFlow::Node sink) {
any(EqualityOperation eqOp).getAnOperand() = sink.asExpr().getExpr()
}
}

import DataFlow::Global<Config>
}

boveus marked this conversation as resolved.
Show resolved Hide resolved
private import UnsafeHmacComparison::PathGraph

from UnsafeHmacComparison::PathNode source, UnsafeHmacComparison::PathNode sink
where UnsafeHmacComparison::flowPath(source, sink)
select sink.getNode(), source, sink, "This comparison is potentially vulnerable to a timing attack."
21 changes: 21 additions & 0 deletions ruby/ql/src/experimental/cwe-208/UnsafeHmacComparison.qlhelp
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
Using the `==` or `!=` operator to compare a known valid HMAC with a user-supplied HMAC digest could lead to a timing attack, as these operations do not occur in constant time.
</p>
</overview>
<recommendation>
<p>
Instead of using `==` or `!=` to compare a known valid HMAC with a user-supplied HMAC digest use Rack::Utils#secure_compare, ActiveSupport::SecurityUtils#secure_compare or OpenSSL.secure_compare
</p>
</recommendation>
<example>
<p>
In this example, the HMAC is validated using the `==` operation.
</p>
<sample src="./examples/unsafe_hmac_comparison.rb" />
</example>
</qhelp>
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class UnsafeHmacComparison
def verify_hmac(host, hmac, salt)
sha1 = OpenSSL::Digest.new('sha1')
if OpenSSL::HMAC.digest(sha1, salt, host) == hmac
puts "HMAC verified"
else
puts "HMAC not verified"
end
end
end