diff --git a/ql/lib/codeql/ruby/Concepts.qll b/ql/lib/codeql/ruby/Concepts.qll index b276d63b032..37f898c668e 100644 --- a/ql/lib/codeql/ruby/Concepts.qll +++ b/ql/lib/codeql/ruby/Concepts.qll @@ -419,6 +419,15 @@ module HTTP { /** Gets a string that identifies the framework used for this request. */ string getFramework() { result = super.getFramework() } + + /** + * Holds if this request is made using a mode that disables SSL/TLS + * certificate validation, where `disablingNode` represents the point at + * which the validation was disabled. + */ + predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + super.disablesCertificateValidation(disablingNode) + } } /** Provides a class for modeling new HTTP requests. */ @@ -435,6 +444,13 @@ module HTTP { /** Gets a string that identifies the framework used for this request. */ abstract string getFramework(); + + /** + * Holds if this request is made using a mode that disables SSL/TLS + * certificate validation, where `disablingNode` represents the point at + * which the validation was disabled. + */ + abstract predicate disablesCertificateValidation(DataFlow::Node disablingNode); } } diff --git a/ql/lib/codeql/ruby/ast/Literal.qll b/ql/lib/codeql/ruby/ast/Literal.qll index 6820f38996d..3e9714e3ce6 100644 --- a/ql/lib/codeql/ruby/ast/Literal.qll +++ b/ql/lib/codeql/ruby/ast/Literal.qll @@ -194,6 +194,13 @@ class BooleanLiteral extends Literal, TBooleanLiteral { /** Holds if the Boolean literal is `false` or `FALSE`. */ predicate isFalse() { none() } + + /** Gets the value of this Boolean literal. */ + boolean getValue() { + this.isTrue() and result = true + or + this.isFalse() and result = false + } } private class TrueLiteral extends BooleanLiteral, TTrueLiteral { @@ -750,7 +757,7 @@ class HashLiteral extends Literal, THashLiteral { final override string getAPrimaryQlClass() { result = "HashLiteral" } /** - * Gets the `n`th element in this array literal. + * Gets the `n`th element in this hash literal. * * In the following example, the 0th element is a `Pair`, and the 1st element * is a `HashSplatExpr`. @@ -761,7 +768,7 @@ class HashLiteral extends Literal, THashLiteral { */ final Expr getElement(int n) { toGenerated(result) = g.getChild(n) } - /** Gets an element in this array literal. */ + /** Gets an element in this hash literal. */ final Expr getAnElement() { result = this.getElement(_) } /** Gets a key-value `Pair` in this hash literal. */ diff --git a/ql/lib/codeql/ruby/frameworks/http_clients/Excon.qll b/ql/lib/codeql/ruby/frameworks/http_clients/Excon.qll index 18ef8b34a1f..efb9d7be66c 100644 --- a/ql/lib/codeql/ruby/frameworks/http_clients/Excon.qll +++ b/ql/lib/codeql/ruby/frameworks/http_clients/Excon.qll @@ -18,29 +18,113 @@ private import codeql.ruby.ApiGraphs * https://github.com/excon/excon/blob/master/README.md */ class ExconHttpRequest extends HTTP::Client::Request::Range { - DataFlow::Node request; - DataFlow::CallNode responseBody; + DataFlow::Node requestUse; + API::Node requestNode; + API::Node connectionNode; ExconHttpRequest() { - exists(API::Node requestNode | request = requestNode.getAnImmediateUse() | - requestNode = - [ - // one-off requests - API::getTopLevelMember("Excon"), - // connection re-use - API::getTopLevelMember("Excon").getInstance() - ] - .getReturn([ - // Excon#request exists but Excon.request doesn't. - // This shouldn't be a problem - in real code the latter would raise NoMethodError anyway. - "get", "head", "delete", "options", "post", "put", "patch", "trace", "request" - ]) and - responseBody = requestNode.getAMethodCall("body") and - this = request.asExpr().getExpr() + requestUse = requestNode.getAnImmediateUse() and + connectionNode = + [ + // one-off requests + API::getTopLevelMember("Excon"), + // connection re-use + API::getTopLevelMember("Excon").getInstance(), + API::getTopLevelMember("Excon").getMember("Connection").getInstance() + ] and + requestNode = + connectionNode + .getReturn([ + // Excon#request exists but Excon.request doesn't. + // This shouldn't be a problem - in real code the latter would raise NoMethodError anyway. + "get", "head", "delete", "options", "post", "put", "patch", "trace", "request" + ]) and + this = requestUse.asExpr().getExpr() + } + + override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") } + + override predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + // Check for `ssl_verify_peer: false` in the options hash. + exists(DataFlow::Node arg, int i | + i > 0 and arg = connectionNode.getAUse().(DataFlow::CallNode).getArgument(i) + | + argSetsVerifyPeer(arg, false, disablingNode) + ) + or + // Or we see a call to `Excon.defaults[:ssl_verify_peer] = false` before the + // request, and no `ssl_verify_peer: true` in the explicit options hash for + // the request call. + exists(DataFlow::CallNode disableCall | + setsDefaultVerification(disableCall, false) and + disableCall.asExpr().getASuccessor+() = requestUse.asExpr() and + disablingNode = disableCall and + not exists(DataFlow::Node arg, int i | + i > 0 and arg = connectionNode.getAUse().(DataFlow::CallNode).getArgument(i) + | + argSetsVerifyPeer(arg, true, _) + ) ) } - override DataFlow::Node getResponseBody() { result = responseBody } - override string getFramework() { result = "Excon" } } + +/** + * Holds if `arg` represents an options hash that contains the key + * `:ssl_verify_peer` with `value`, where `kvNode` is the data-flow node for + * this key-value pair. + */ +predicate argSetsVerifyPeer(DataFlow::Node arg, boolean value, DataFlow::Node kvNode) { + // Either passed as an individual key:value argument, e.g.: + // Excon.get(..., ssl_verify_peer: false) + isSslVerifyPeerPair(arg.asExpr().getExpr(), value) and + kvNode = arg + or + // Or as a single hash argument, e.g.: + // Excon.get(..., { ssl_verify_peer: false, ... }) + exists(DataFlow::LocalSourceNode optionsNode, Pair p | + p = optionsNode.asExpr().getExpr().(HashLiteral).getAKeyValuePair() and + isSslVerifyPeerPair(p, value) and + optionsNode.flowsTo(arg) and + kvNode.asExpr().getExpr() = p + ) +} + +/** + * Holds if `callNode` sets `Excon.defaults[:ssl_verify_peer]` or + * `Excon.ssl_verify_peer` to `value`. + */ +private predicate setsDefaultVerification(DataFlow::CallNode callNode, boolean value) { + callNode = API::getTopLevelMember("Excon").getReturn("defaults").getAMethodCall("[]=") and + isSslVerifyPeerLiteral(callNode.getArgument(0)) and + hasBooleanValue(callNode.getArgument(1), value) + or + callNode = API::getTopLevelMember("Excon").getAMethodCall("ssl_verify_peer=") and + hasBooleanValue(callNode.getArgument(0), value) +} + +private predicate isSslVerifyPeerLiteral(DataFlow::Node node) { + exists(DataFlow::LocalSourceNode literal | + literal.asExpr().getExpr().(SymbolLiteral).getValueText() = "ssl_verify_peer" and + literal.flowsTo(node) + ) +} + +/** Holds if `node` can contain `value`. */ +private predicate hasBooleanValue(DataFlow::Node node, boolean value) { + exists(DataFlow::LocalSourceNode literal | + literal.asExpr().getExpr().(BooleanLiteral).getValue() = value and + literal.flowsTo(node) + ) +} + +/** Holds if `p` is the pair `ssl_verify_peer: `. */ +private predicate isSslVerifyPeerPair(Pair p, boolean value) { + exists(DataFlow::Node key, DataFlow::Node valueNode | + key.asExpr().getExpr() = p.getKey() and valueNode.asExpr().getExpr() = p.getValue() + | + isSslVerifyPeerLiteral(key) and + hasBooleanValue(valueNode, value) + ) +} diff --git a/ql/lib/codeql/ruby/frameworks/http_clients/Faraday.qll b/ql/lib/codeql/ruby/frameworks/http_clients/Faraday.qll index 5edeacb2a1f..78dd6889dd0 100644 --- a/ql/lib/codeql/ruby/frameworks/http_clients/Faraday.qll +++ b/ql/lib/codeql/ruby/frameworks/http_clients/Faraday.qll @@ -14,25 +14,131 @@ private import codeql.ruby.ApiGraphs * ``` */ class FaradayHttpRequest extends HTTP::Client::Request::Range { - DataFlow::Node request; - DataFlow::CallNode responseBody; + DataFlow::Node requestUse; + API::Node requestNode; + API::Node connectionNode; FaradayHttpRequest() { - exists(API::Node requestNode | - requestNode = - [ - // one-off requests - API::getTopLevelMember("Faraday"), - // connection re-use - API::getTopLevelMember("Faraday").getInstance() - ].getReturn(["get", "head", "delete", "post", "put", "patch", "trace"]) and - responseBody = requestNode.getAMethodCall("body") and - request = requestNode.getAnImmediateUse() and - this = request.asExpr().getExpr() + connectionNode = + [ + // one-off requests + API::getTopLevelMember("Faraday"), + // connection re-use + API::getTopLevelMember("Faraday").getInstance() + ] and + requestNode = + connectionNode.getReturn(["get", "head", "delete", "post", "put", "patch", "trace"]) and + requestUse = requestNode.getAnImmediateUse() and + this = requestUse.asExpr().getExpr() + } + + override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") } + + override predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + // `Faraday::new` takes an options hash as its second argument, and we're + // looking for + // `{ ssl: { verify: false } }` + // or + // `{ ssl: { verify_mode: OpenSSL::SSL::VERIFY_NONE } }` + exists(DataFlow::Node arg, int i | + i > 0 and arg = connectionNode.getAUse().(DataFlow::CallNode).getArgument(i) + | + // Either passed as an individual key:value argument, e.g.: + // Faraday.new(..., ssl: {...}) + isSslOptionsPairDisablingValidation(arg.asExpr().getExpr()) and + disablingNode = arg + or + // Or as a single hash argument, e.g.: + // Faraday.new(..., { ssl: {...} }) + exists(DataFlow::LocalSourceNode optionsNode, Pair p | + p = optionsNode.asExpr().getExpr().(HashLiteral).getAKeyValuePair() and + isSslOptionsPairDisablingValidation(p) and + optionsNode.flowsTo(arg) and + disablingNode.asExpr().getExpr() = p + ) ) } - override DataFlow::Node getResponseBody() { result = responseBody } - override string getFramework() { result = "Faraday" } } + +/** + * Holds if the pair `p` contains the key `:ssl` for which the value is a hash + * containing either `verify: false` or + * `verify_mode: OpenSSL::SSL::VERIFY_NONE`. + */ +private predicate isSslOptionsPairDisablingValidation(Pair p) { + exists(DataFlow::Node key, DataFlow::Node value | + key.asExpr().getExpr() = p.getKey() and value.asExpr().getExpr() = p.getValue() + | + exists(DataFlow::LocalSourceNode literal | + literal.asExpr().getExpr().(SymbolLiteral).getValueText() = "ssl" and + literal.flowsTo(key) + ) and + (isHashWithVerifyFalse(value) or isHashWithVerifyModeNone(value)) + ) +} + +/** + * Holds if `node` represents a hash containing the key-value pair + * `verify: false`. + */ +private predicate isHashWithVerifyFalse(DataFlow::Node node) { + exists(DataFlow::LocalSourceNode hash | + isVerifyFalsePair(hash.asExpr().getExpr().(HashLiteral).getAKeyValuePair()) and + hash.flowsTo(node) + ) +} + +/** + * Holds if `node` represents a hash containing the key-value pair + * `verify_mode: OpenSSL::SSL::VERIFY_NONE`. + */ +private predicate isHashWithVerifyModeNone(DataFlow::Node node) { + exists(DataFlow::LocalSourceNode hash | + isVerifyModeNonePair(hash.asExpr().getExpr().(HashLiteral).getAKeyValuePair()) and + hash.flowsTo(node) + ) +} + +/** + * Holds if the pair `p` has the key `:verify_mode` and the value + * `OpenSSL::SSL::VERIFY_NONE`. + */ +private predicate isVerifyModeNonePair(Pair p) { + exists(DataFlow::Node key, DataFlow::Node value | + key.asExpr().getExpr() = p.getKey() and value.asExpr().getExpr() = p.getValue() + | + exists(DataFlow::LocalSourceNode literal | + literal.asExpr().getExpr().(SymbolLiteral).getValueText() = "verify_mode" and + literal.flowsTo(key) + ) and + value = API::getTopLevelMember("OpenSSL").getMember("SSL").getMember("VERIFY_NONE").getAUse() + ) +} + +/** + * Holds if the pair `p` has the key `:verify` and the value `false`. + */ +private predicate isVerifyFalsePair(Pair p) { + exists(DataFlow::Node key, DataFlow::Node value | + key.asExpr().getExpr() = p.getKey() and value.asExpr().getExpr() = p.getValue() + | + exists(DataFlow::LocalSourceNode literal | + literal.asExpr().getExpr().(SymbolLiteral).getValueText() = "verify" and + literal.flowsTo(key) + ) and + isFalsey(value) + ) +} + +/** Holds if `node` contains `0` or `false`. */ +private predicate isFalsey(DataFlow::Node node) { + exists(DataFlow::LocalSourceNode literal | + ( + literal.asExpr().getExpr().(BooleanLiteral).isFalse() or + literal.asExpr().getExpr().(IntegerLiteral).getValue() = 0 + ) and + literal.flowsTo(node) + ) +} diff --git a/ql/lib/codeql/ruby/frameworks/http_clients/HttpClient.qll b/ql/lib/codeql/ruby/frameworks/http_clients/HttpClient.qll index e8c45e630b3..3db9c653a5c 100644 --- a/ql/lib/codeql/ruby/frameworks/http_clients/HttpClient.qll +++ b/ql/lib/codeql/ruby/frameworks/http_clients/HttpClient.qll @@ -10,31 +10,46 @@ private import codeql.ruby.ApiGraphs * ``` */ class HttpClientRequest extends HTTP::Client::Request::Range { - DataFlow::Node request; - DataFlow::CallNode responseBody; + API::Node requestNode; + API::Node connectionNode; + DataFlow::Node requestUse; + string method; HttpClientRequest() { - exists(API::Node requestNode, string method | - request = requestNode.getAnImmediateUse() and - method in [ - "get", "head", "delete", "options", "post", "put", "trace", "get_content", "post_content" - ] - | - requestNode = API::getTopLevelMember("HTTPClient").getReturn(method) and - ( - // The `get_content` and `post_content` methods return the response body as a string. - // The other methods return a `HTTPClient::Message` object which has various methods - // that return the response body. - method in ["get_content", "post_content"] and responseBody = request - or - not method in ["get_content", "put_content"] and - responseBody = requestNode.getAMethodCall(["body", "http_body", "content", "dump"]) - ) and - this = request.asExpr().getExpr() - ) + connectionNode = + [ + // One-off requests + API::getTopLevelMember("HTTPClient"), + // Conncection re-use + API::getTopLevelMember("HTTPClient").getInstance() + ] and + requestNode = connectionNode.getReturn(method) and + requestUse = requestNode.getAnImmediateUse() and + method in [ + "get", "head", "delete", "options", "post", "put", "trace", "get_content", "post_content" + ] and + this = requestUse.asExpr().getExpr() } - override DataFlow::Node getResponseBody() { result = responseBody } + override DataFlow::Node getResponseBody() { + // The `get_content` and `post_content` methods return the response body as + // a string. The other methods return a `HTTPClient::Message` object which + // has various methods that return the response body. + method in ["get_content", "post_content"] and result = requestUse + or + not method in ["get_content", "put_content"] and + result = requestNode.getAMethodCall(["body", "http_body", "content", "dump"]) + } + + override predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + // Look for calls to set + // `c.ssl_config.verify_mode = OpenSSL::SSL::VERIFY_NONE` + // on an HTTPClient connection object `c`. + disablingNode = + connectionNode.getReturn("ssl_config").getReturn("verify_mode=").getAnImmediateUse() and + disablingNode.(DataFlow::CallNode).getArgument(0) = + API::getTopLevelMember("OpenSSL").getMember("SSL").getMember("VERIFY_NONE").getAUse() + } override string getFramework() { result = "HTTPClient" } } diff --git a/ql/lib/codeql/ruby/frameworks/http_clients/Httparty.qll b/ql/lib/codeql/ruby/frameworks/http_clients/Httparty.qll index b3249441afa..f106195268f 100644 --- a/ql/lib/codeql/ruby/frameworks/http_clients/Httparty.qll +++ b/ql/lib/codeql/ruby/frameworks/http_clients/Httparty.qll @@ -17,30 +17,82 @@ private import codeql.ruby.ApiGraphs * ``` */ class HttpartyRequest extends HTTP::Client::Request::Range { - DataFlow::Node request; - DataFlow::CallNode responseBody; + API::Node requestNode; + DataFlow::Node requestUse; HttpartyRequest() { - exists(API::Node requestNode | request = requestNode.getAnImmediateUse() | - requestNode = - API::getTopLevelMember("HTTParty") - .getReturn(["get", "head", "delete", "options", "post", "put", "patch"]) and - ( - // If HTTParty can recognise the response type, it will parse and return it - // directly from the request call. Otherwise, it will return a `HTTParty::Response` - // object that has a `#body` method. - // So if there's a call to `#body` on the response, treat that as the response body. - exists(DataFlow::Node r | r = requestNode.getAMethodCall("body") | responseBody = r) - or - // Otherwise, treat the response as the response body. - not exists(DataFlow::Node r | r = requestNode.getAMethodCall("body")) and - responseBody = request - ) and - this = request.asExpr().getExpr() + requestUse = requestNode.getAnImmediateUse() and + requestNode = + API::getTopLevelMember("HTTParty") + .getReturn(["get", "head", "delete", "options", "post", "put", "patch"]) and + this = requestUse.asExpr().getExpr() + } + + override DataFlow::Node getResponseBody() { + // If HTTParty can recognise the response type, it will parse and return it + // directly from the request call. Otherwise, it will return a `HTTParty::Response` + // object that has a `#body` method. + // So if there's a call to `#body` on the response, treat that as the response body. + exists(DataFlow::Node r | r = requestNode.getAMethodCall("body") | result = r) + or + // Otherwise, treat the response as the response body. + not exists(DataFlow::Node r | r = requestNode.getAMethodCall("body")) and + result = requestUse + } + + override predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + // The various request methods take an options hash as their second + // argument, and we're looking for `{ verify: false }` or + // `{ verify_peer: // false }`. + exists(DataFlow::Node arg, int i | + i > 0 and arg.asExpr().getExpr() = requestUse.asExpr().getExpr().(MethodCall).getArgument(i) + | + // Either passed as an individual key:value argument, e.g.: + // HTTParty.get(..., verify: false) + isVerifyFalsePair(arg.asExpr().getExpr()) and + disablingNode = arg + or + // Or as a single hash argument, e.g.: + // HTTParty.get(..., { verify: false, ... }) + exists(DataFlow::LocalSourceNode optionsNode, Pair p | + p = optionsNode.asExpr().getExpr().(HashLiteral).getAKeyValuePair() and + isVerifyFalsePair(p) and + optionsNode.flowsTo(arg) and + disablingNode.asExpr().getExpr() = p + ) ) } - override DataFlow::Node getResponseBody() { result = responseBody } - override string getFramework() { result = "HTTParty" } } + +/** Holds if `node` represents the symbol literal `verify` or `verify_peer`. */ +private predicate isVerifyLiteral(DataFlow::Node node) { + exists(DataFlow::LocalSourceNode literal | + literal.asExpr().getExpr().(SymbolLiteral).getValueText() = ["verify", "verify_peer"] and + literal.flowsTo(node) + ) +} + +/** Holds if `node` contains `0` or `false`. */ +private predicate isFalsey(DataFlow::Node node) { + exists(DataFlow::LocalSourceNode literal | + ( + literal.asExpr().getExpr().(BooleanLiteral).isFalse() or + literal.asExpr().getExpr().(IntegerLiteral).getValue() = 0 + ) and + literal.flowsTo(node) + ) +} + +/** + * Holds if `p` is the pair `verify: false` or `verify_peer: false`. + */ +private predicate isVerifyFalsePair(Pair p) { + exists(DataFlow::Node key, DataFlow::Node value | + key.asExpr().getExpr() = p.getKey() and value.asExpr().getExpr() = p.getValue() + | + isVerifyLiteral(key) and + isFalsey(value) + ) +} diff --git a/ql/lib/codeql/ruby/frameworks/http_clients/NetHttp.qll b/ql/lib/codeql/ruby/frameworks/http_clients/NetHttp.qll index e22951bc980..9d9c6f7aff3 100644 --- a/ql/lib/codeql/ruby/frameworks/http_clients/NetHttp.qll +++ b/ql/lib/codeql/ruby/frameworks/http_clients/NetHttp.qll @@ -50,5 +50,20 @@ class NetHttpRequest extends HTTP::Client::Request::Range { override DataFlow::Node getResponseBody() { result = responseBody } + override predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + // A Net::HTTP request bypasses certificate validation if we see a setter + // call like this: + // foo.verify_mode = OpenSSL::SSL::VERIFY_NONE + // and then the receiver of that call flows to the receiver in the request: + // foo.request(...) + exists(DataFlow::CallNode setter | + disablingNode = + API::getTopLevelMember("OpenSSL").getMember("SSL").getMember("VERIFY_NONE").getAUse() and + setter.asExpr().getExpr().(SetterMethodCall).getMethodName() = "verify_mode=" and + disablingNode = setter.getArgument(0) and + localFlow(setter.getReceiver(), request.getReceiver()) + ) + } + override string getFramework() { result = "Net::HTTP" } } diff --git a/ql/lib/codeql/ruby/frameworks/http_clients/OpenURI.qll b/ql/lib/codeql/ruby/frameworks/http_clients/OpenURI.qll index 9e93ebab717..54a2c180fec 100644 --- a/ql/lib/codeql/ruby/frameworks/http_clients/OpenURI.qll +++ b/ql/lib/codeql/ruby/frameworks/http_clients/OpenURI.qll @@ -4,36 +4,110 @@ private import codeql.ruby.ApiGraphs private import codeql.ruby.frameworks.StandardLibrary /** - * A call that makes an HTTP request using `OpenURI`. + * A call that makes an HTTP request using `OpenURI` via `URI.open` or + * `URI.parse(...).open`. + * * ```ruby - * Kernel.open("http://example.com").read * URI.open("http://example.com").readlines * URI.parse("http://example.com").open.read * ``` */ -class OpenURIRequest extends HTTP::Client::Request::Range { - DataFlow::Node request; - DataFlow::CallNode responseBody; +class OpenUriRequest extends HTTP::Client::Request::Range { + API::Node requestNode; + DataFlow::Node requestUse; - OpenURIRequest() { - exists(API::Node requestNode | request = requestNode.getAnImmediateUse() | - requestNode = - [API::getTopLevelMember("URI"), API::getTopLevelMember("URI").getReturn("parse")] - .getReturn("open") and - responseBody = requestNode.getAMethodCall(["read", "readlines"]) and - this = request.asExpr().getExpr() - ) - or - // Kernel.open("http://example.com").read - // open("http://example.com").read - request instanceof KernelMethodCall and - this.getMethodName() = "open" and - request.asExpr().getExpr() = this and - responseBody.asExpr().getExpr().(MethodCall).getMethodName() in ["read", "readlines"] and - request.(DataFlow::LocalSourceNode).flowsTo(responseBody.getReceiver()) + OpenUriRequest() { + requestNode = + [API::getTopLevelMember("URI"), API::getTopLevelMember("URI").getReturn("parse")] + .getReturn("open") and + requestUse = requestNode.getAnImmediateUse() and + this = requestUse.asExpr().getExpr() } - override DataFlow::Node getResponseBody() { result = responseBody } + override DataFlow::Node getResponseBody() { + result = requestNode.getAMethodCall(["read", "readlines"]) + } + + override predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + exists(DataFlow::Node arg | + arg.asExpr().getExpr() = requestUse.asExpr().getExpr().(MethodCall).getArgument(_) + | + argumentDisablesValidation(arg, disablingNode) + ) + } override string getFramework() { result = "OpenURI" } } + +/** + * A call that makes an HTTP request using `OpenURI` and its `Kernel.open` + * interface. + * + * ```ruby + * Kernel.open("http://example.com").read + * ``` + */ +class OpenUriKernelOpenRequest extends HTTP::Client::Request::Range { + DataFlow::Node requestUse; + + OpenUriKernelOpenRequest() { + requestUse instanceof KernelMethodCall and + this.getMethodName() = "open" and + this = requestUse.asExpr().getExpr() + } + + override DataFlow::CallNode getResponseBody() { + result.asExpr().getExpr().(MethodCall).getMethodName() in ["read", "readlines"] and + requestUse.(DataFlow::LocalSourceNode).flowsTo(result.getReceiver()) + } + + override predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + exists(DataFlow::Node arg, int i | + i > 0 and + arg.asExpr().getExpr() = requestUse.asExpr().getExpr().(MethodCall).getArgument(i) + | + argumentDisablesValidation(arg, disablingNode) + ) + } + + override string getFramework() { result = "OpenURI" } +} + +/** + * Holds if the argument `arg` is an options hash that disables certificate + * validation, and `disablingNode` is the specific node representing the + * `ssl_verify_mode: OpenSSL::SSL_VERIFY_NONE` pair. + */ +private predicate argumentDisablesValidation(DataFlow::Node arg, DataFlow::Node disablingNode) { + // Either passed as an individual key:value argument, e.g.: + // URI.open(..., ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE) + isSslVerifyModeNonePair(arg.asExpr().getExpr()) and + disablingNode = arg + or + // Or as a single hash argument, e.g.: + // URI.open(..., { ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE, ... }) + exists(DataFlow::LocalSourceNode optionsNode, Pair p | + p = optionsNode.asExpr().getExpr().(HashLiteral).getAKeyValuePair() and + isSslVerifyModeNonePair(p) and + optionsNode.flowsTo(arg) and + disablingNode.asExpr().getExpr() = p + ) +} + +/** Holds if `p` is the pair `ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE`. */ +private predicate isSslVerifyModeNonePair(Pair p) { + exists(DataFlow::Node key, DataFlow::Node value | + key.asExpr().getExpr() = p.getKey() and value.asExpr().getExpr() = p.getValue() + | + isSslVerifyModeLiteral(key) and + value = API::getTopLevelMember("OpenSSL").getMember("SSL").getMember("VERIFY_NONE").getAUse() + ) +} + +/** Holds if `node` can represent the symbol literal `:ssl_verify_mode`. */ +private predicate isSslVerifyModeLiteral(DataFlow::Node node) { + exists(DataFlow::LocalSourceNode literal | + literal.asExpr().getExpr().(SymbolLiteral).getValueText() = "ssl_verify_mode" and + literal.flowsTo(node) + ) +} diff --git a/ql/lib/codeql/ruby/frameworks/http_clients/RestClient.qll b/ql/lib/codeql/ruby/frameworks/http_clients/RestClient.qll index f2d082be069..3b6ff318b66 100644 --- a/ql/lib/codeql/ruby/frameworks/http_clients/RestClient.qll +++ b/ql/lib/codeql/ruby/frameworks/http_clients/RestClient.qll @@ -9,21 +9,63 @@ private import codeql.ruby.ApiGraphs * ``` */ class RestClientHttpRequest extends HTTP::Client::Request::Range { - DataFlow::Node request; - DataFlow::CallNode responseBody; + DataFlow::Node requestUse; + API::Node requestNode; + API::Node connectionNode; RestClientHttpRequest() { - exists(API::Node requestNode | - requestNode = - API::getTopLevelMember("RestClient") - .getReturn(["get", "head", "delete", "options", "post", "put", "patch"]) and - request = requestNode.getAnImmediateUse() and - responseBody = requestNode.getAMethodCall("body") and - this = request.asExpr().getExpr() + connectionNode = + [ + API::getTopLevelMember("RestClient"), + API::getTopLevelMember("RestClient").getMember("Resource").getInstance() + ] and + requestNode = + connectionNode.getReturn(["get", "head", "delete", "options", "post", "put", "patch"]) and + requestUse = requestNode.getAnImmediateUse() and + this = requestUse.asExpr().getExpr() + } + + override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") } + + override predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + // `RestClient::Resource::new` takes an options hash argument, and we're + // looking for `{ verify_ssl: OpenSSL::SSL::VERIFY_NONE }`. + exists(DataFlow::Node arg, int i | + i > 0 and arg = connectionNode.getAUse().(DataFlow::CallNode).getArgument(i) + | + // Either passed as an individual key:value argument, e.g.: + // RestClient::Resource.new(..., verify_ssl: OpenSSL::SSL::VERIFY_NONE) + isVerifySslNonePair(arg.asExpr().getExpr()) and + disablingNode = arg + or + // Or as a single hash argument, e.g.: + // RestClient::Resource.new(..., { verify_ssl: OpenSSL::SSL::VERIFY_NONE }) + exists(DataFlow::LocalSourceNode optionsNode, Pair p | + p = optionsNode.asExpr().getExpr().(HashLiteral).getAKeyValuePair() and + isVerifySslNonePair(p) and + optionsNode.flowsTo(arg) and + disablingNode.asExpr().getExpr() = p + ) ) } - override DataFlow::Node getResponseBody() { result = responseBody } - override string getFramework() { result = "RestClient" } } + +/** Holds if `p` is the pair `verify_ssl: OpenSSL::SSL::VERIFY_NONE`. */ +private predicate isVerifySslNonePair(Pair p) { + exists(DataFlow::Node key, DataFlow::Node value | + key.asExpr().getExpr() = p.getKey() and value.asExpr().getExpr() = p.getValue() + | + isSslVerifyModeLiteral(key) and + value = API::getTopLevelMember("OpenSSL").getMember("SSL").getMember("VERIFY_NONE").getAUse() + ) +} + +/** Holds if `node` can represent the symbol literal `:verify_ssl`. */ +private predicate isSslVerifyModeLiteral(DataFlow::Node node) { + exists(DataFlow::LocalSourceNode literal | + literal.asExpr().getExpr().(SymbolLiteral).getValueText() = "verify_ssl" and + literal.flowsTo(node) + ) +} diff --git a/ql/lib/codeql/ruby/frameworks/http_clients/Typhoeus.qll b/ql/lib/codeql/ruby/frameworks/http_clients/Typhoeus.qll index 494091ff45c..946c6005c03 100644 --- a/ql/lib/codeql/ruby/frameworks/http_clients/Typhoeus.qll +++ b/ql/lib/codeql/ruby/frameworks/http_clients/Typhoeus.qll @@ -9,20 +9,52 @@ private import codeql.ruby.ApiGraphs * ``` */ class TyphoeusHttpRequest extends HTTP::Client::Request::Range { - DataFlow::Node request; - DataFlow::CallNode responseBody; + DataFlow::Node requestUse; + API::Node requestNode; TyphoeusHttpRequest() { - exists(API::Node requestNode | request = requestNode.getAnImmediateUse() | - requestNode = - API::getTopLevelMember("Typhoeus") - .getReturn(["get", "head", "delete", "options", "post", "put", "patch"]) and - responseBody = requestNode.getAMethodCall("body") and - this = request.asExpr().getExpr() + requestUse = requestNode.getAnImmediateUse() and + requestNode = + API::getTopLevelMember("Typhoeus") + .getReturn(["get", "head", "delete", "options", "post", "put", "patch"]) and + this = requestUse.asExpr().getExpr() + } + + override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") } + + override predicate disablesCertificateValidation(DataFlow::Node disablingNode) { + // Check for `ssl_verifypeer: false` in the options hash. + exists(DataFlow::Node arg, int i | + i > 0 and arg.asExpr().getExpr() = requestUse.asExpr().getExpr().(MethodCall).getArgument(i) + | + // Either passed as an individual key:value argument, e.g.: + // Typhoeus.get(..., ssl_verifypeer: false) + isSslVerifyPeerFalsePair(arg.asExpr().getExpr()) and + disablingNode = arg + or + // Or as a single hash argument, e.g.: + // Typhoeus.get(..., { ssl_verifypeer: false, ... }) + exists(DataFlow::LocalSourceNode optionsNode, Pair p | + p = optionsNode.asExpr().getExpr().(HashLiteral).getAKeyValuePair() and + isSslVerifyPeerFalsePair(p) and + optionsNode.flowsTo(arg) and + disablingNode.asExpr().getExpr() = p + ) ) } - override DataFlow::Node getResponseBody() { result = responseBody } - override string getFramework() { result = "Typhoeus" } } + +// Holds if `p` is the pair `ssl_verifypeer: false`. +private predicate isSslVerifyPeerFalsePair(Pair p) { + p.getKey().(SymbolLiteral).getValueText() = "ssl_verifypeer" and + exists(DataFlow::LocalSourceNode literal, DataFlow::Node value | + ( + literal.asExpr().getExpr().(BooleanLiteral).isFalse() or + literal.asExpr().getExpr().(IntegerLiteral).getValue() = 0 + ) and + literal.flowsTo(value) and + value.asExpr().getExpr() = p.getValue() + ) +} diff --git a/ql/src/queries/security/cwe-295/RequestWithoutValidation.qhelp b/ql/src/queries/security/cwe-295/RequestWithoutValidation.qhelp new file mode 100644 index 00000000000..c94cd04d03d --- /dev/null +++ b/ql/src/queries/security/cwe-295/RequestWithoutValidation.qhelp @@ -0,0 +1,52 @@ + + + +

+Certificate validation is the standard authentication method of a secure TLS +connection. Without it, there is no guarantee about who the other party of a TLS +connection is, making man-in-the-middle attacks more likely to occur. +

+ +

+When testing software that uses TLS connections, it may be useful to +disable the certificate validation temporarily. But disabling it in +production environments is strongly discouraged, unless an alternative +method of authentication is used. +

+
+ + +

+Do not disable certificate validation for TLS connections. +

+
+ + + +

+The following example shows an HTTPS connection that makes a GET request to a +remote server. But the connection is not secure since the +verify_mode option of the connection is set to +OpenSSL::SSL::VERIFY_NONE. As a consequence, anyone can impersonate +the remote server. +

+ + + +

+To make the connection secure, the verify_mode option should have +its default value, or be explicitly set to +OpenSSL::SSL::VERIFY_PEER. +

+ +
+ + +
  • Wikipedia: Transport Layer Security (TLS)
  • +
  • Wikipedia: Man-in-the-middle attack
  • +
  • Ruby-doc: Net::HTTP
  • +
    + +
    \ No newline at end of file diff --git a/ql/src/queries/security/cwe-295/RequestWithoutValidation.ql b/ql/src/queries/security/cwe-295/RequestWithoutValidation.ql new file mode 100644 index 00000000000..cdc17e308a1 --- /dev/null +++ b/ql/src/queries/security/cwe-295/RequestWithoutValidation.ql @@ -0,0 +1,20 @@ +/** + * @name Request without certificate validation + * @description Making a request without certificate validation can allow + * man-in-the-middle attacks. + * @kind problem + * @problem.severity warning + * @security-severity 7.5 + * @precision medium + * @id rb/request-without-cert-validation + * @tags security + * external/cwe/cwe-295 + */ + +import ruby +import codeql.ruby.Concepts +import codeql.ruby.DataFlow + +from HTTP::Client::Request request, DataFlow::Node disablingNode +where request.disablesCertificateValidation(disablingNode) +select request, "This request $@.", disablingNode, "does not validate certificates" diff --git a/ql/src/queries/security/cwe-295/examples/RequestWithoutValidation.rb b/ql/src/queries/security/cwe-295/examples/RequestWithoutValidation.rb new file mode 100644 index 00000000000..d7ab1df197a --- /dev/null +++ b/ql/src/queries/security/cwe-295/examples/RequestWithoutValidation.rb @@ -0,0 +1,9 @@ +require "net/https" +require "uri" + +uri = URI.parse "https://example.com/" +http = Net::HTTP.new uri.host, uri.port +http.use_ssl = true +http.verify_mode = OpenSSL::SSL::VERIFY_NONE +request = Net::HTTP::Get.new uri.request_uri +puts http.request(request).body diff --git a/ql/test/library-tests/frameworks/http_clients/Excon.expected b/ql/test/library-tests/frameworks/http_clients/Excon.expected index e25ae0aff63..0275c675213 100644 --- a/ql/test/library-tests/frameworks/http_clients/Excon.expected +++ b/ql/test/library-tests/frameworks/http_clients/Excon.expected @@ -6,5 +6,7 @@ | Excon.rb:18:9:18:41 | call to head | Excon.rb:19:1:19:10 | call to body | | Excon.rb:21:9:21:44 | call to options | Excon.rb:22:1:22:10 | call to body | | Excon.rb:24:9:24:42 | call to trace | Excon.rb:25:1:25:10 | call to body | -| Excon.rb:28:9:28:33 | call to get | Excon.rb:29:1:29:10 | call to body | -| Excon.rb:31:10:31:38 | call to post | Excon.rb:32:1:32:11 | call to body | +| Excon.rb:28:9:28:34 | call to get | Excon.rb:29:1:29:10 | call to body | +| Excon.rb:31:10:31:39 | call to post | Excon.rb:32:1:32:11 | call to body | +| Excon.rb:35:9:35:34 | call to get | Excon.rb:36:1:36:10 | call to body | +| Excon.rb:38:10:38:39 | call to post | Excon.rb:39:1:39:11 | call to body | diff --git a/ql/test/library-tests/frameworks/http_clients/Excon.rb b/ql/test/library-tests/frameworks/http_clients/Excon.rb index 55166eeca17..fb0bc7e75f1 100644 --- a/ql/test/library-tests/frameworks/http_clients/Excon.rb +++ b/ql/test/library-tests/frameworks/http_clients/Excon.rb @@ -24,9 +24,16 @@ resp7.body resp8 = Excon.trace("http://example.com/") resp8.body -connection = Excon.new("http://example.com") -resp9 = connection.get(path: "/") +connection1 = Excon.new("http://example.com") +resp9 = connection1.get(path: "/") resp9.body -resp10 = connection.post(path: "/foo") +resp10 = connection1.post(path: "/foo") +resp10.body + +connection2 = Excon::Connection.new("http://example.com") +resp9 = connection2.get(path: "/") +resp9.body + +resp10 = connection2.post(path: "/foo") resp10.body \ No newline at end of file diff --git a/ql/test/library-tests/frameworks/http_clients/OpenURI.expected b/ql/test/library-tests/frameworks/http_clients/OpenURI.expected index 990f975f1ec..9da5401ba33 100644 --- a/ql/test/library-tests/frameworks/http_clients/OpenURI.expected +++ b/ql/test/library-tests/frameworks/http_clients/OpenURI.expected @@ -1,4 +1,6 @@ -| OpenURI.rb:3:9:3:41 | call to open | OpenURI.rb:4:1:4:10 | call to read | -| OpenURI.rb:6:9:6:34 | call to open | OpenURI.rb:7:1:7:15 | call to readlines | +openUriRequests | OpenURI.rb:9:9:9:38 | call to open | OpenURI.rb:10:1:10:10 | call to read | | OpenURI.rb:12:9:12:45 | call to open | OpenURI.rb:13:1:13:10 | call to read | +openUriKernelOpenRequests +| OpenURI.rb:3:9:3:41 | call to open | OpenURI.rb:4:1:4:10 | call to read | +| OpenURI.rb:6:9:6:34 | call to open | OpenURI.rb:7:1:7:15 | call to readlines | diff --git a/ql/test/library-tests/frameworks/http_clients/OpenURI.ql b/ql/test/library-tests/frameworks/http_clients/OpenURI.ql index 9193927512a..91d824f9965 100644 --- a/ql/test/library-tests/frameworks/http_clients/OpenURI.ql +++ b/ql/test/library-tests/frameworks/http_clients/OpenURI.ql @@ -1,4 +1,8 @@ import codeql.ruby.frameworks.http_clients.OpenURI import codeql.ruby.DataFlow -query DataFlow::Node openURIRequests(OpenURIRequest e) { result = e.getResponseBody() } +query DataFlow::Node openUriRequests(OpenUriRequest e) { result = e.getResponseBody() } + +query DataFlow::Node openUriKernelOpenRequests(OpenUriKernelOpenRequest e) { + result = e.getResponseBody() +} diff --git a/ql/test/library-tests/frameworks/http_clients/RestClient.expected b/ql/test/library-tests/frameworks/http_clients/RestClient.expected index d39b89fbdcb..4489baf81b5 100644 --- a/ql/test/library-tests/frameworks/http_clients/RestClient.expected +++ b/ql/test/library-tests/frameworks/http_clients/RestClient.expected @@ -5,3 +5,4 @@ | RestClient.rb:15:9:15:47 | call to delete | RestClient.rb:16:1:16:10 | call to body | | RestClient.rb:18:9:18:45 | call to head | RestClient.rb:19:1:19:10 | call to body | | RestClient.rb:21:9:21:48 | call to options | RestClient.rb:22:1:22:10 | call to body | +| RestClient.rb:25:9:25:21 | call to get | RestClient.rb:26:1:26:10 | call to body | diff --git a/ql/test/library-tests/frameworks/http_clients/RestClient.rb b/ql/test/library-tests/frameworks/http_clients/RestClient.rb index 61d8caa43d5..875aab62fc3 100644 --- a/ql/test/library-tests/frameworks/http_clients/RestClient.rb +++ b/ql/test/library-tests/frameworks/http_clients/RestClient.rb @@ -19,4 +19,8 @@ resp6 = RestClient.head("http://example.com") resp6.body resp7 = RestClient.options("http://example.com") -resp7.body \ No newline at end of file +resp7.body + +resource8 = RestClient::Resource.new "http://example.com" +resp8 = resource8.get +resp8.body \ No newline at end of file diff --git a/ql/test/query-tests/security/cwe-295/Excon.rb b/ql/test/query-tests/security/cwe-295/Excon.rb new file mode 100644 index 00000000000..e08f9672ef5 --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/Excon.rb @@ -0,0 +1,49 @@ +require "excon" + +def method1 + # BAD + Excon.defaults[:ssl_verify_peer] = false + Excon.get("http://example.com/") +end + +def method2 + # BAD + Excon.ssl_verify_peer = false + Excon.get("http://example.com/") +end + +def method3(secure) + # BAD + Excon.defaults[:ssl_verify_peer] = (secure ? true : false) + Excon.get("http://example.com/") +end + +def method4 + # BAD + conn = Excon::Connection.new("http://example.com/", ssl_verify_peer: false) + conn.get +end + +def method5 + # BAD + Excon.ssl_verify_peer = true + Excon.new("http://example.com/", ssl_verify_peer: false).get +end + +def method6 + # GOOD + Excon.defaults[:ssl_verify_peer] = true + Excon.get("http://example.com/") +end + +def method7 + # GOOD + Excon.ssl_verify_peer = true + Excon.get("http://example.com/") +end + +def method8 + # GOOD + Excon.defaults[:ssl_verify_peer] = false + Excon.new("http://example.com/", ssl_verify_peer: true) +end \ No newline at end of file diff --git a/ql/test/query-tests/security/cwe-295/Faraday.rb b/ql/test/query-tests/security/cwe-295/Faraday.rb new file mode 100644 index 00000000000..fb66122ed6a --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/Faraday.rb @@ -0,0 +1,28 @@ +require "faraday" + +# BAD +connection = Faraday.new("http://example.com", ssl: { verify: false }) +response = connection.get("/") + +# BAD +connection = Faraday.new("http://example.com", ssl: { verify_mode: OpenSSL::SSL::VERIFY_NONE }) +response = connection.get("/") + +# GOOD +connection = Faraday.new("http://example.com") +response = connection.get("/") + +# GOOD +response = Faraday.get("http://example.com") + +# GOOD +connection = Faraday.new("http://example.com", ssl: { version: :TLSv1 }) +response = connection.get("/") + +# GOOD +connection = Faraday.new("http://example.com", ssl: { verify: true }) +response = connection.get("/") + +# GOOD +connection = Faraday.new("http://example.com", ssl: { verify_mode: OpenSSL::SSL::VERIFY_PEER }) +response = connection.get("/") diff --git a/ql/test/query-tests/security/cwe-295/HttpClient.rb b/ql/test/query-tests/security/cwe-295/HttpClient.rb new file mode 100644 index 00000000000..902950e5be9 --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/HttpClient.rb @@ -0,0 +1,18 @@ +require "httpclient" + +# BAD +client = HTTPClient.new +client.ssl_config.verify_mode = OpenSSL::SSL::VERIFY_NONE +client.get("https://example.com") + +# GOOD +client = HTTPClient.new +client.ssl_config.verify_mode = OpenSSL::SSL::VERIFY_PEER +client.get("https://example.com") + +# GOOD +client = HTTPClient.new +client.get("https://example.com") + +# GOOD +HTTPClient.get("https://example.com/") \ No newline at end of file diff --git a/ql/test/query-tests/security/cwe-295/Httparty.rb b/ql/test/query-tests/security/cwe-295/Httparty.rb new file mode 100644 index 00000000000..562cbbc1f43 --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/Httparty.rb @@ -0,0 +1,37 @@ +require "httparty" + +# BAD +HTTParty.get("http://example.com/", verify: false) + +# BAD +HTTParty.get("http://example.com/", verify_peer: false) + +# BAD +HTTParty.get("http://example.com/", { verify_peer: false }) + +# BAD +HTTParty.post("http://example.com/", body: "some_data", verify: false) + +# BAD +HTTParty.post("http://example.com/", { body: "some_data", verify: false }) + +# GOOD +HTTParty.get("http://example.com/") + +# GOOD +HTTParty.get("http://example.com/", verify: true) + +# GOOD +HTTParty.get("http://example.com/", verify_peer: true) + +# GOOD +HTTParty.post("http://example.com/", body: "some_data") + +# GOOD +HTTParty.post("http://example.com/", body: "some_data", verify: true) + +# GOOD +HTTParty.post("http://example.com/", { body: "some_data" }) + +# GOOD +HTTParty.post("http://example.com/", { body: "some_data", verify: true }) \ No newline at end of file diff --git a/ql/test/query-tests/security/cwe-295/NetHttp.rb b/ql/test/query-tests/security/cwe-295/NetHttp.rb new file mode 100644 index 00000000000..9269eeae531 --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/NetHttp.rb @@ -0,0 +1,10 @@ +require "net/https" +require "uri" + +uri = URI.parse "https://example.com/" +http = Net::HTTP.new uri.host, uri.port +http.use_ssl = true +http.verify_mode = OpenSSL::SSL::VERIFY_NONE +request = Net::HTTP::Get.new uri.request_uri +response = http.request request +puts response.body diff --git a/ql/test/query-tests/security/cwe-295/OpenURI.rb b/ql/test/query-tests/security/cwe-295/OpenURI.rb new file mode 100644 index 00000000000..a825791c823 --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/OpenURI.rb @@ -0,0 +1,47 @@ +require "open-uri" + +# BAD +Kernel.open("https://example.com", ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE) + +# BAD +Kernel.open("https://example.com", { ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE }) + +# BAD +options = { ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE } +Kernel.open("https://example.com", options) + +# BAD +URI.parse("https://example.com").open(ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE) + +# BAD +URI.parse("https://example.com").open({ ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE }) + +# BAD +options = { ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE } +URI.parse("https://example.com").open(options) + +# GOOD +Kernel.open("https://example.com") + +# GOOD +Kernel.open("https://example.com", ssl_verify_mode: OpenSSL::SSL::VERIFY_PEER) + +# GOOD +Kernel.open("https://example.com", { ssl_verify_mode: OpenSSL::SSL::VERIFY_PEER }) + +# GOOD +options = { ssl_verify_mode: OpenSSL::SSL::VERIFY_PEER } +Kernel.open("https://example.com", options) + +# GOOD +URI.parse("https://example.com").open + +# GOOD +URI.parse("https://example.com").open(ssl_verify_mode: OpenSSL::SSL::VERIFY_PEER) + +# GOOD +URI.parse("https://example.com").open({ ssl_verify_mode: OpenSSL::SSL::VERIFY_PEER }) + +# GOOD +options = { ssl_verify_mode: OpenSSL::SSL::VERIFY_PEER } +URI.parse("https://example.com").open(options) \ No newline at end of file diff --git a/ql/test/query-tests/security/cwe-295/RequestWithoutValidation.expected b/ql/test/query-tests/security/cwe-295/RequestWithoutValidation.expected new file mode 100644 index 00000000000..adab0acbbea --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/RequestWithoutValidation.expected @@ -0,0 +1,25 @@ +| Excon.rb:6:3:6:34 | call to get | This request $@. | Excon.rb:5:3:5:34 | call to []= | does not validate certificates | +| Excon.rb:12:3:12:34 | call to get | This request $@. | Excon.rb:11:3:11:23 | call to ssl_verify_peer= | does not validate certificates | +| Excon.rb:18:3:18:34 | call to get | This request $@. | Excon.rb:17:3:17:34 | call to []= | does not validate certificates | +| Excon.rb:24:3:24:10 | call to get | This request $@. | Excon.rb:23:55:23:76 | Pair | does not validate certificates | +| Excon.rb:30:3:30:62 | call to get | This request $@. | Excon.rb:30:36:30:57 | Pair | does not validate certificates | +| Faraday.rb:5:12:5:30 | call to get | This request $@. | Faraday.rb:4:48:4:69 | Pair | does not validate certificates | +| Faraday.rb:9:12:9:30 | call to get | This request $@. | Faraday.rb:8:48:8:94 | Pair | does not validate certificates | +| HttpClient.rb:6:1:6:33 | call to get | This request $@. | HttpClient.rb:5:1:5:29 | call to verify_mode= | does not validate certificates | +| Httparty.rb:4:1:4:50 | call to get | This request $@. | Httparty.rb:4:37:4:49 | Pair | does not validate certificates | +| Httparty.rb:7:1:7:55 | call to get | This request $@. | Httparty.rb:7:37:7:54 | Pair | does not validate certificates | +| Httparty.rb:10:1:10:59 | call to get | This request $@. | Httparty.rb:10:39:10:56 | Pair | does not validate certificates | +| Httparty.rb:13:1:13:70 | call to post | This request $@. | Httparty.rb:13:57:13:69 | Pair | does not validate certificates | +| Httparty.rb:16:1:16:74 | call to post | This request $@. | Httparty.rb:16:59:16:71 | Pair | does not validate certificates | +| NetHttp.rb:9:12:9:31 | call to request | This request $@. | NetHttp.rb:7:1:7:16 | ... = ... | does not validate certificates | +| OpenURI.rb:4:1:4:78 | call to open | This request $@. | OpenURI.rb:4:36:4:77 | Pair | does not validate certificates | +| OpenURI.rb:7:1:7:82 | call to open | This request $@. | OpenURI.rb:7:38:7:79 | Pair | does not validate certificates | +| OpenURI.rb:11:1:11:43 | call to open | This request $@. | OpenURI.rb:10:13:10:54 | Pair | does not validate certificates | +| OpenURI.rb:14:1:14:81 | call to open | This request $@. | OpenURI.rb:14:39:14:80 | Pair | does not validate certificates | +| OpenURI.rb:17:1:17:85 | call to open | This request $@. | OpenURI.rb:17:41:17:82 | Pair | does not validate certificates | +| OpenURI.rb:21:1:21:46 | call to open | This request $@. | OpenURI.rb:20:13:20:54 | Pair | does not validate certificates | +| RestClient.rb:5:12:5:23 | call to get | This request $@. | RestClient.rb:4:60:4:96 | Pair | does not validate certificates | +| RestClient.rb:9:12:9:23 | call to get | This request $@. | RestClient.rb:8:62:8:98 | Pair | does not validate certificates | +| RestClient.rb:14:12:14:23 | call to get | This request $@. | RestClient.rb:12:13:12:49 | Pair | does not validate certificates | +| Typhoeus.rb:4:1:4:62 | call to get | This request $@. | Typhoeus.rb:4:41:4:61 | Pair | does not validate certificates | +| Typhoeus.rb:8:1:8:54 | call to post | This request $@. | Typhoeus.rb:7:37:7:57 | Pair | does not validate certificates | diff --git a/ql/test/query-tests/security/cwe-295/RequestWithoutValidation.qlref b/ql/test/query-tests/security/cwe-295/RequestWithoutValidation.qlref new file mode 100644 index 00000000000..e2caf232ddb --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/RequestWithoutValidation.qlref @@ -0,0 +1 @@ +queries/security/cwe-295/RequestWithoutValidation.ql \ No newline at end of file diff --git a/ql/test/query-tests/security/cwe-295/RestClient.rb b/ql/test/query-tests/security/cwe-295/RestClient.rb new file mode 100644 index 00000000000..4615c0acb80 --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/RestClient.rb @@ -0,0 +1,33 @@ +require "rest-client" + +# BAD +resource = RestClient::Resource.new("https://example.com", verify_ssl: OpenSSL::SSL::VERIFY_NONE) +response = resource.get + +# BAD +resource = RestClient::Resource.new("https://example.com", { verify_ssl: OpenSSL::SSL::VERIFY_NONE }) +response = resource.get + +# BAD +options = { verify_ssl: OpenSSL::SSL::VERIFY_NONE } +resource = RestClient::Resource.new("https://example.com", options) +response = resource.get + +# GOOD +RestClient.get("https://example.com") + +# GOOD +resource = RestClient::Resource.new("https://example.com") +response = resource.get + +# GOOD +resource = RestClient::Resource.new("https://example.com", verify_ssl: OpenSSL::SSL::VERIFY_PEER) +response = resource.get +# BAD +resource = RestClient::Resource.new("https://example.com", { verify_ssl: OpenSSL::SSL::VERIFY_PEER }) +response = resource.get + +# GOOD +options = { verify_ssl: OpenSSL::SSL::VERIFY_PEER } +resource = RestClient::Resource.new("https://example.com", options) +response = resource.get \ No newline at end of file diff --git a/ql/test/query-tests/security/cwe-295/Typhoeus.rb b/ql/test/query-tests/security/cwe-295/Typhoeus.rb new file mode 100644 index 00000000000..aed601cf888 --- /dev/null +++ b/ql/test/query-tests/security/cwe-295/Typhoeus.rb @@ -0,0 +1,11 @@ +require "typhoeus" + +# BAD +Typhoeus.get("https://www.example.com", ssl_verifypeer: false) + +# BAD +post_options = { body: "some data", ssl_verifypeer: false } +Typhoeus.post("https://www.example.com", post_options) + +# GOOD +Typhoeus.get("https://www.example.com") \ No newline at end of file