From 5f6cb1684b012038ed64c219822def1addd48839 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 17 Oct 2022 16:50:29 +0200 Subject: [PATCH 01/41] move the code-injection tests into a subfolder --- .../security/cwe-094/{ => CodeInjection}/CodeInjection.expected | 0 .../security/cwe-094/{ => CodeInjection}/CodeInjection.qlref | 0 .../security/cwe-094/{ => CodeInjection}/CodeInjection.rb | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename ruby/ql/test/query-tests/security/cwe-094/{ => CodeInjection}/CodeInjection.expected (100%) rename ruby/ql/test/query-tests/security/cwe-094/{ => CodeInjection}/CodeInjection.qlref (100%) rename ruby/ql/test/query-tests/security/cwe-094/{ => CodeInjection}/CodeInjection.rb (100%) diff --git a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.expected similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected rename to ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.expected diff --git a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.qlref b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.qlref similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-094/CodeInjection.qlref rename to ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.qlref diff --git a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.rb similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb rename to ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.rb From f1668801d3ded16ac25f190e581904b21530cdee Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 17 Oct 2022 16:52:08 +0200 Subject: [PATCH 02/41] add a `rb/unsafe-code-construction` query rebase --- .../UnsafeCodeConstructionCustomizations.qll | 94 +++++++++++++++++++ .../security/UnsafeCodeConstructionQuery.qll | 33 +++++++ .../cwe-094/UnsafeCodeConstruction.qhelp | 55 +++++++++++ .../cwe-094/UnsafeCodeConstruction.ql | 23 +++++ .../examples/UnsafeCodeConstruction.rb | 10 ++ .../examples/UnsafeCodeConstructionSafe.rb | 11 +++ .../UnsafeCodeConstruction.expected | 16 ++++ .../UnsafeCodeConstruction.qlref | 1 + .../UnsafeCodeConstruction/impl/unsafeCode.rb | 19 ++++ .../unsafe-code.gemspec | 5 + 10 files changed, 267 insertions(+) create mode 100644 ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll create mode 100644 ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll create mode 100644 ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp create mode 100644 ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.ql create mode 100644 ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction.rb create mode 100644 ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstructionSafe.rb create mode 100644 ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected create mode 100644 ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.qlref create mode 100644 ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb create mode 100644 ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/unsafe-code.gemspec diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll new file mode 100644 index 00000000000..188bbb34cc9 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll @@ -0,0 +1,94 @@ +/** + * Provides default sources, sinks and sanitizers for reasoning about + * code constructed from library input vulnerabilities, as + * well as extension points for adding your own. + */ + +private import codeql.ruby.DataFlow +private import codeql.ruby.ApiGraphs +private import codeql.ruby.frameworks.core.Gem::Gem as Gem +private import codeql.ruby.AST as Ast +private import codeql.ruby.Concepts as Concepts + +/** + * Module containing sources, sinks, and sanitizers for code constructed from library input. + */ +module UnsafeCodeConstruction { + /** A source for code constructed from library input vulnerabilities. */ + abstract class Source extends DataFlow::Node { } + + /** An input parameter to a gem seen as a source. */ + private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode { + LibraryInputAsSource() { this = Gem::getALibraryInput() } + } + + /** A sink for code constructed from library input vulnerabilities. */ + abstract class Sink extends DataFlow::Node { + /** + * Gets the node where the unsafe code is executed. + */ + abstract DataFlow::Node getCodeSink(); + + /** + * Gets the type of sink. + */ + string getSinkType() { result = "code construction" } + } + + /** Gets a node that is eventually executed as code at `codeExec`. */ + DataFlow::Node getANodeExecutedAsCode(Concepts::CodeExecution codeExec) { + result = getANodeExecutedAsCode(TypeTracker::TypeBackTracker::end(), codeExec) + } + + import codeql.ruby.typetracking.TypeTracker as TypeTracker + + /** Gets a node that is eventually executed as code at `codeExec`, type-tracked with `t`. */ + private DataFlow::LocalSourceNode getANodeExecutedAsCode( + TypeTracker::TypeBackTracker t, Concepts::CodeExecution codeExec + ) { + t.start() and + result = codeExec.getCode().getALocalSource() + or + exists(TypeTracker::TypeBackTracker t2 | + result = getANodeExecutedAsCode(t2, codeExec).backtrack(t2, t) + ) + } + + /** + * A string constructed from a string-literal (e.g. `"foo #{sink}"`), + * where the resulting string ends up being executed as a code. + */ + class StringFormatAsSink extends Sink { + Concepts::CodeExecution s; + Ast::StringLiteral lit; + + StringFormatAsSink() { + any(DataFlow::Node n | n.asExpr().getExpr() = lit) = getANodeExecutedAsCode(s) and + this.asExpr().getExpr() = lit.getComponent(_) + } + + override DataFlow::Node getCodeSink() { result = s } + + override string getSinkType() { result = "string interpolation" } + } + + import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat + + /** + * A string constructed from a printf-style call, + * where the resulting string ends up being executed as a code. + */ + class TaintedFormatStringAsSink extends Sink { + Concepts::CodeExecution s; + TaintedFormat::PrintfStyleCall call; + + TaintedFormatStringAsSink() { + call = getANodeExecutedAsCode(s) and + this = [call.getFormatArgument(_), call.getFormatString()] + } + + override DataFlow::Node getCodeSink() { result = s } + + override string getSinkType() { result = "string format" } + } +} diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll new file mode 100644 index 00000000000..4b27a258716 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll @@ -0,0 +1,33 @@ +/** + * Provides a taint-tracking configuration for reasoning about code + * constructed from library input vulnerabilities. + * + * Note, for performance reasons: only import this file if `Configuration` is needed, + * otherwise `UnsafeCodeConstructionCustomizations` should be imported instead. + */ + +import codeql.ruby.DataFlow +import UnsafeCodeConstructionCustomizations::UnsafeCodeConstruction +private import codeql.ruby.TaintTracking +private import codeql.ruby.dataflow.BarrierGuards + +/** + * A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities. + */ +class Configuration extends TaintTracking::Configuration { + Configuration() { this = "UnsafeShellCommandConstruction" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + override predicate isSanitizer(DataFlow::Node node) { + node instanceof StringConstCompareBarrier or + node instanceof StringConstArrayInclusionCallBarrier + } + + // override to require the path doesn't have unmatched return steps + override DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureHasSourceCallContext + } +} diff --git a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp new file mode 100644 index 00000000000..c430259b029 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp @@ -0,0 +1,55 @@ + + + + +

+When a library function dynamically constructs code in a potentially unsafe way, then +it's important to document to clients of the library that the function should only be +used with trusted inputs. + +If the function is not documented as being potentially unsafe, then a client may +incorrectly use inputs containing unsafe code fragments, and thereby leave the +client vulnerable to code-injection attacks. +

+ + + + +

+Properly document library functions that construct code from unsanitized +inputs, or avoid constructing code in the first place. +

+ + + +

+The following example shows two methods implemented using `eval`: a simple +deserialization routine and a getter method. +If untrusted inputs are used with these methods, +then an attacker might be able to execute arbitrary code on the system. +

+ + + +

+To avoid this problem, either properly document that the function is potentially +unsafe, or use an alternative solution such as `JSON.parse` or another library, like in the examples below, +that does not allow arbitrary code to be executed. +

+ + + + + + +
  • +OWASP: +Code Injection. +
    • +
  • +Wikipedia: Code Injection. +
    • +     +     diff --git a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.ql b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.ql new file mode 100644 index 00000000000..6b7b923e934 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.ql @@ -0,0 +1,23 @@ +/** + * @name Unsafe code constructed from library input + * @description Using externally controlled strings to construct code may allow a malicious + * user to execute arbitrary code. + * @kind path-problem + * @problem.severity warning + * @security-severity 6.1 + * @precision medium + * @id rb/unsafe-code-construction + * @tags security + * external/cwe/cwe-094 + * external/cwe/cwe-079 + * external/cwe/cwe-116 + */ + +import codeql.ruby.security.UnsafeCodeConstructionQuery +import DataFlow::PathGraph + +from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode +where cfg.hasFlowPath(source, sink) and sinkNode = sink.getNode() +select sink.getNode(), source, sink, + "This " + sinkNode.getSinkType() + " which depends on $@ is later $@.", source.getNode(), + "library input", sinkNode.getCodeSink(), "interpreted as code" diff --git a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction.rb b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction.rb new file mode 100644 index 00000000000..d900a950a50 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction.rb @@ -0,0 +1,10 @@ +module MyLib + def unsafeDeserialize(value) + eval("foo = #{value}") + foo + end + + def unsafeGetter(obj, path) + eval("obj.#{path}") + end +end diff --git a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstructionSafe.rb b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstructionSafe.rb new file mode 100644 index 00000000000..aa2445ae234 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstructionSafe.rb @@ -0,0 +1,11 @@ +require 'json' + +module MyLib + def safeDeserialize(value) + JSON.parse(value) + end + + def safeGetter(obj, path) + obj.dig(*path.split(".")) + end +end diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected new file mode 100644 index 00000000000..56fc308e99d --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected @@ -0,0 +1,16 @@ +edges +| impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} | +| impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x | +| impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x | +nodes +| impl/unsafeCode.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} | +| impl/unsafeCode.rb:7:12:7:12 | x : | semmle.label | x : | +| impl/unsafeCode.rb:8:30:8:30 | x | semmle.label | x | +| impl/unsafeCode.rb:12:12:12:12 | x : | semmle.label | x : | +| impl/unsafeCode.rb:13:33:13:33 | x | semmle.label | x | +subpaths +#select +| impl/unsafeCode.rb:3:17:3:25 | #{...} | impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:2:12:2:17 | target | library input | impl/unsafeCode.rb:3:5:3:27 | call to eval | interpreted as code | +| impl/unsafeCode.rb:8:30:8:30 | x | impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x | This string format which depends on $@ is later $@. | impl/unsafeCode.rb:7:12:7:12 | x | library input | impl/unsafeCode.rb:8:5:8:32 | call to eval | interpreted as code | +| impl/unsafeCode.rb:13:33:13:33 | x | impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x | This string format which depends on $@ is later $@. | impl/unsafeCode.rb:12:12:12:12 | x | library input | impl/unsafeCode.rb:13:5:13:35 | call to eval | interpreted as code | diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.qlref b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.qlref new file mode 100644 index 00000000000..ec336901db5 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.qlref @@ -0,0 +1 @@ +queries/security/cwe-094/UnsafeCodeConstruction.ql \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb new file mode 100644 index 00000000000..4ee84d07ad2 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb @@ -0,0 +1,19 @@ +class Foobar + def foo1(target) + eval("foo = #{target}") # NOT OK + end + + # sprintf + def foo2(x) + eval(sprintf("foo = %s", x)) # NOT OK + end + + # String#% + def foo3(x) + eval("foo = %{foo}" % {foo: x}) # NOT OK + end + + def indirect_eval(x) + eval(x) # OK - no construction. + end +end diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/unsafe-code.gemspec b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/unsafe-code.gemspec new file mode 100644 index 00000000000..ab9639a5fc6 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/unsafe-code.gemspec @@ -0,0 +1,5 @@ +Gem::Specification.new do |s| + s.name = 'unsafe-code' + s.require_path = "impl" + end + \ No newline at end of file From e7c6571f5240dde63851c808d56348f0b7a55866 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 17 Oct 2022 21:00:59 +0200 Subject: [PATCH 03/41] remove the "send(..)" and similar from unsafe-code-construction --- .../ruby/security/UnsafeCodeConstructionCustomizations.qll | 3 ++- .../cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll index 188bbb34cc9..07ca5978a8a 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll @@ -47,7 +47,8 @@ module UnsafeCodeConstruction { TypeTracker::TypeBackTracker t, Concepts::CodeExecution codeExec ) { t.start() and - result = codeExec.getCode().getALocalSource() + result = codeExec.getCode().getALocalSource() and + codeExec.runsArbitraryCode() // methods like `Object.send` is benign here, because of the string-construction the attacker cannot control the entire method name or exists(TypeTracker::TypeBackTracker t2 | result = getANodeExecutedAsCode(t2, codeExec).backtrack(t2, t) diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb index 4ee84d07ad2..30bf04bab84 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb @@ -16,4 +16,8 @@ class Foobar def indirect_eval(x) eval(x) # OK - no construction. end + + def send_stuff(x) + foo.send("foo_#{x}") # OK - attacker cannot control entire string. + end end From 2033dd2dcc96ef903bceba0aa9a7de7165416b40 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 18 Oct 2022 12:35:30 +0200 Subject: [PATCH 04/41] remove parameters named "code" as source --- .../ruby/security/UnsafeCodeConstructionCustomizations.qll | 5 ++++- .../cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll index 07ca5978a8a..fe261de3dca 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll @@ -19,7 +19,10 @@ module UnsafeCodeConstruction { /** An input parameter to a gem seen as a source. */ private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode { - LibraryInputAsSource() { this = Gem::getALibraryInput() } + LibraryInputAsSource() { + this = Gem::getALibraryInput() and + not this.getName() = "code" + } } /** A sink for code constructed from library input vulnerabilities. */ diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb index 30bf04bab84..d7d720d5d32 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb @@ -20,4 +20,8 @@ class Foobar def send_stuff(x) foo.send("foo_#{x}") # OK - attacker cannot control entire string. end + + def named_code(code) + foo.send("def \n #{code} \n end") # OK - parameter is named code + end end From 0f2a48f461ea33962bb9477cf7fbbbc0a4efedb9 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 18 Oct 2022 12:37:50 +0200 Subject: [PATCH 05/41] fix QL-for-QL warnings --- .../UnsafeCodeConstructionCustomizations.qll | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll index fe261de3dca..6a03718808c 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll @@ -64,11 +64,12 @@ module UnsafeCodeConstruction { */ class StringFormatAsSink extends Sink { Concepts::CodeExecution s; - Ast::StringLiteral lit; StringFormatAsSink() { - any(DataFlow::Node n | n.asExpr().getExpr() = lit) = getANodeExecutedAsCode(s) and - this.asExpr().getExpr() = lit.getComponent(_) + exists(Ast::StringLiteral lit | + any(DataFlow::Node n | n.asExpr().getExpr() = lit) = getANodeExecutedAsCode(s) and + this.asExpr().getExpr() = lit.getComponent(_) + ) } override DataFlow::Node getCodeSink() { result = s } @@ -84,11 +85,12 @@ module UnsafeCodeConstruction { */ class TaintedFormatStringAsSink extends Sink { Concepts::CodeExecution s; - TaintedFormat::PrintfStyleCall call; TaintedFormatStringAsSink() { - call = getANodeExecutedAsCode(s) and - this = [call.getFormatArgument(_), call.getFormatString()] + exists(TaintedFormat::PrintfStyleCall call | + call = getANodeExecutedAsCode(s) and + this = [call.getFormatArgument(_), call.getFormatString()] + ) } override DataFlow::Node getCodeSink() { result = s } From 3461404bbba7d417779eec305cdfcc3a13bb9112 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 24 Nov 2022 17:35:27 +0100 Subject: [PATCH 06/41] add basic support for arrays --- .../UnsafeCodeConstructionCustomizations.qll | 22 +++++++++++++++++-- .../UnsafeCodeConstruction.expected | 4 ++++ .../UnsafeCodeConstruction/impl/unsafeCode.rb | 7 ++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll index 6a03718808c..559a21f761a 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll @@ -4,10 +4,9 @@ * well as extension points for adding your own. */ -private import codeql.ruby.DataFlow +private import ruby private import codeql.ruby.ApiGraphs private import codeql.ruby.frameworks.core.Gem::Gem as Gem -private import codeql.ruby.AST as Ast private import codeql.ruby.Concepts as Concepts /** @@ -58,6 +57,25 @@ module UnsafeCodeConstruction { ) } + /** + * A string constructed using a `.join(...)` call, where the resulting string ends up being executed as code. + */ + class ArrayJoin extends Sink { + Concepts::CodeExecution s; + DataFlow::CallNode call; + + ArrayJoin() { + call.getMethodName() = "join" and + call.getNumberOfArguments() = 1 and // any string. E.g. ";" or "\n". + call = getANodeExecutedAsCode(s) and + this = call.getReceiver() + } + + override DataFlow::Node getCodeSink() { result = s } + + override string getSinkType() { result = "array" } + } + /** * A string constructed from a string-literal (e.g. `"foo #{sink}"`), * where the resulting string ends up being executed as a code. diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected index 56fc308e99d..1f4d4e33719 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected @@ -2,6 +2,7 @@ edges | impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} | | impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x | | impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x | +| impl/unsafeCode.rb:28:17:28:22 | my_arr : | impl/unsafeCode.rb:29:10:29:15 | my_arr | nodes | impl/unsafeCode.rb:2:12:2:17 | target : | semmle.label | target : | | impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} | @@ -9,8 +10,11 @@ nodes | impl/unsafeCode.rb:8:30:8:30 | x | semmle.label | x | | impl/unsafeCode.rb:12:12:12:12 | x : | semmle.label | x : | | impl/unsafeCode.rb:13:33:13:33 | x | semmle.label | x | +| impl/unsafeCode.rb:28:17:28:22 | my_arr : | semmle.label | my_arr : | +| impl/unsafeCode.rb:29:10:29:15 | my_arr | semmle.label | my_arr | subpaths #select | impl/unsafeCode.rb:3:17:3:25 | #{...} | impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:2:12:2:17 | target | library input | impl/unsafeCode.rb:3:5:3:27 | call to eval | interpreted as code | | impl/unsafeCode.rb:8:30:8:30 | x | impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x | This string format which depends on $@ is later $@. | impl/unsafeCode.rb:7:12:7:12 | x | library input | impl/unsafeCode.rb:8:5:8:32 | call to eval | interpreted as code | | impl/unsafeCode.rb:13:33:13:33 | x | impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x | This string format which depends on $@ is later $@. | impl/unsafeCode.rb:12:12:12:12 | x | library input | impl/unsafeCode.rb:13:5:13:35 | call to eval | interpreted as code | +| impl/unsafeCode.rb:29:10:29:15 | my_arr | impl/unsafeCode.rb:28:17:28:22 | my_arr : | impl/unsafeCode.rb:29:10:29:15 | my_arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:28:17:28:22 | my_arr | library input | impl/unsafeCode.rb:29:5:29:27 | call to eval | interpreted as code | diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb index d7d720d5d32..ca3605ec050 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb @@ -24,4 +24,11 @@ class Foobar def named_code(code) foo.send("def \n #{code} \n end") # OK - parameter is named code end + + def joinStuff(my_arr) + eval(my_arr.join("\n")) # NOT OK + end + + # TODO: [x, y].join("\n") is not yet supported + # TODO: list << element. end From 80c92dc3e65a07a75f487a9b0ddcdfe8ca3e09cf Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 24 Nov 2022 17:47:03 +0100 Subject: [PATCH 07/41] add support for array pushes --- .../security/UnsafeCodeConstructionQuery.qll | 16 ++++++++++++++++ .../UnsafeCodeConstruction.expected | 11 +++++++++++ .../UnsafeCodeConstruction/impl/unsafeCode.rb | 16 ++++++++++++++-- 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll index 4b27a258716..a5670a783e4 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll @@ -30,4 +30,20 @@ class Configuration extends TaintTracking::Configuration { override DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } + + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + // if an array element gets tainted, then we treat the entire array as tainted + exists(DataFlow::CallNode call | + call.getMethodName() = ["<<", "push", "append"] and + call.getReceiver() = succ and + pred = call.getArgument(0) and + call.getNumberOfArguments() = 1 + ) + or + exists(DataFlow::CallNode call | + call.getMethodName() = "[]" and + succ = call and + pred = call.getArgument(_) + ) + } } diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected index 1f4d4e33719..ffc53d43faf 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected @@ -3,6 +3,9 @@ edges | impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x | | impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x | | impl/unsafeCode.rb:28:17:28:22 | my_arr : | impl/unsafeCode.rb:29:10:29:15 | my_arr | +| impl/unsafeCode.rb:32:21:32:21 | x : | impl/unsafeCode.rb:34:10:34:12 | arr | +| impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:40:10:40:12 | arr | +| impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:44:10:44:12 | arr | nodes | impl/unsafeCode.rb:2:12:2:17 | target : | semmle.label | target : | | impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} | @@ -12,9 +15,17 @@ nodes | impl/unsafeCode.rb:13:33:13:33 | x | semmle.label | x | | impl/unsafeCode.rb:28:17:28:22 | my_arr : | semmle.label | my_arr : | | impl/unsafeCode.rb:29:10:29:15 | my_arr | semmle.label | my_arr | +| impl/unsafeCode.rb:32:21:32:21 | x : | semmle.label | x : | +| impl/unsafeCode.rb:34:10:34:12 | arr | semmle.label | arr | +| impl/unsafeCode.rb:37:15:37:15 | x : | semmle.label | x : | +| impl/unsafeCode.rb:40:10:40:12 | arr | semmle.label | arr | +| impl/unsafeCode.rb:44:10:44:12 | arr | semmle.label | arr | subpaths #select | impl/unsafeCode.rb:3:17:3:25 | #{...} | impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:2:12:2:17 | target | library input | impl/unsafeCode.rb:3:5:3:27 | call to eval | interpreted as code | | impl/unsafeCode.rb:8:30:8:30 | x | impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x | This string format which depends on $@ is later $@. | impl/unsafeCode.rb:7:12:7:12 | x | library input | impl/unsafeCode.rb:8:5:8:32 | call to eval | interpreted as code | | impl/unsafeCode.rb:13:33:13:33 | x | impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x | This string format which depends on $@ is later $@. | impl/unsafeCode.rb:12:12:12:12 | x | library input | impl/unsafeCode.rb:13:5:13:35 | call to eval | interpreted as code | | impl/unsafeCode.rb:29:10:29:15 | my_arr | impl/unsafeCode.rb:28:17:28:22 | my_arr : | impl/unsafeCode.rb:29:10:29:15 | my_arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:28:17:28:22 | my_arr | library input | impl/unsafeCode.rb:29:5:29:27 | call to eval | interpreted as code | +| impl/unsafeCode.rb:34:10:34:12 | arr | impl/unsafeCode.rb:32:21:32:21 | x : | impl/unsafeCode.rb:34:10:34:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:32:21:32:21 | x | library input | impl/unsafeCode.rb:34:5:34:24 | call to eval | interpreted as code | +| impl/unsafeCode.rb:40:10:40:12 | arr | impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:40:10:40:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:37:15:37:15 | x | library input | impl/unsafeCode.rb:40:5:40:24 | call to eval | interpreted as code | +| impl/unsafeCode.rb:44:10:44:12 | arr | impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:44:10:44:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:37:15:37:15 | x | library input | impl/unsafeCode.rb:44:5:44:24 | call to eval | interpreted as code | diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb index ca3605ec050..dac4703abda 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb @@ -29,6 +29,18 @@ class Foobar eval(my_arr.join("\n")) # NOT OK end - # TODO: [x, y].join("\n") is not yet supported - # TODO: list << element. + def joinWithElemt(x) + arr = [x, "foobar"] + eval(arr.join("\n")) # NOT OK + end + + def pushArr(x, y) + arr = [] + arr.push(x) + eval(arr.join("\n")) # NOT OK + + arr2 = [] + arr2 << y + eval(arr.join("\n")) # NOT OK + end end From 378cc1aed2e17589235d0bcb102f367cef5b30d8 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 24 Nov 2022 18:01:54 +0100 Subject: [PATCH 08/41] add support for string-like-literals --- .../ruby/security/UnsafeCodeConstructionCustomizations.qll | 2 +- .../UnsafeCodeConstruction/UnsafeCodeConstruction.expected | 4 ++++ .../cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb | 7 +++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll index 559a21f761a..4599179db5c 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll @@ -84,7 +84,7 @@ module UnsafeCodeConstruction { Concepts::CodeExecution s; StringFormatAsSink() { - exists(Ast::StringLiteral lit | + exists(Ast::StringlikeLiteral lit | any(DataFlow::Node n | n.asExpr().getExpr() = lit) = getANodeExecutedAsCode(s) and this.asExpr().getExpr() = lit.getComponent(_) ) diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected index ffc53d43faf..5b7cd78cca3 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected @@ -6,6 +6,7 @@ edges | impl/unsafeCode.rb:32:21:32:21 | x : | impl/unsafeCode.rb:34:10:34:12 | arr | | impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:40:10:40:12 | arr | | impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:44:10:44:12 | arr | +| impl/unsafeCode.rb:47:15:47:15 | x : | impl/unsafeCode.rb:49:9:49:12 | #{...} | nodes | impl/unsafeCode.rb:2:12:2:17 | target : | semmle.label | target : | | impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} | @@ -20,6 +21,8 @@ nodes | impl/unsafeCode.rb:37:15:37:15 | x : | semmle.label | x : | | impl/unsafeCode.rb:40:10:40:12 | arr | semmle.label | arr | | impl/unsafeCode.rb:44:10:44:12 | arr | semmle.label | arr | +| impl/unsafeCode.rb:47:15:47:15 | x : | semmle.label | x : | +| impl/unsafeCode.rb:49:9:49:12 | #{...} | semmle.label | #{...} | subpaths #select | impl/unsafeCode.rb:3:17:3:25 | #{...} | impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:2:12:2:17 | target | library input | impl/unsafeCode.rb:3:5:3:27 | call to eval | interpreted as code | @@ -29,3 +32,4 @@ subpaths | impl/unsafeCode.rb:34:10:34:12 | arr | impl/unsafeCode.rb:32:21:32:21 | x : | impl/unsafeCode.rb:34:10:34:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:32:21:32:21 | x | library input | impl/unsafeCode.rb:34:5:34:24 | call to eval | interpreted as code | | impl/unsafeCode.rb:40:10:40:12 | arr | impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:40:10:40:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:37:15:37:15 | x | library input | impl/unsafeCode.rb:40:5:40:24 | call to eval | interpreted as code | | impl/unsafeCode.rb:44:10:44:12 | arr | impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:44:10:44:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:37:15:37:15 | x | library input | impl/unsafeCode.rb:44:5:44:24 | call to eval | interpreted as code | +| impl/unsafeCode.rb:49:9:49:12 | #{...} | impl/unsafeCode.rb:47:15:47:15 | x : | impl/unsafeCode.rb:49:9:49:12 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:47:15:47:15 | x | library input | impl/unsafeCode.rb:51:5:51:13 | call to eval | interpreted as code | diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb index dac4703abda..64bba0aadc6 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb @@ -43,4 +43,11 @@ class Foobar arr2 << y eval(arr.join("\n")) # NOT OK end + + def hereDoc(x) + foo = <<~HERE + #{x} + HERE + eval(foo) # NOT OK + end end From 0817238177ec1c818a05d69c0c633154cb65c9d4 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 24 Nov 2022 18:02:27 +0100 Subject: [PATCH 09/41] drive-by: same change in unsafe-shell-command-construction --- .../security/UnsafeShellCommandConstructionCustomizations.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll index 88e9625c822..d4fa74cab89 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll @@ -67,7 +67,7 @@ module UnsafeShellCommandConstruction { */ class StringInterpolationAsSink extends Sink { Concepts::SystemCommandExecution s; - Ast::StringLiteral lit; + Ast::StringlikeLiteral lit; StringInterpolationAsSink() { isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and From 53f24a528147d52173e788f7fae0d79290013a2f Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 24 Nov 2022 18:28:42 +0100 Subject: [PATCH 10/41] fix QL-for-QL warning --- .../security/UnsafeCodeConstructionCustomizations.qll | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll index 4599179db5c..b6c9c3b5219 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll @@ -62,13 +62,14 @@ module UnsafeCodeConstruction { */ class ArrayJoin extends Sink { Concepts::CodeExecution s; - DataFlow::CallNode call; ArrayJoin() { - call.getMethodName() = "join" and - call.getNumberOfArguments() = 1 and // any string. E.g. ";" or "\n". - call = getANodeExecutedAsCode(s) and - this = call.getReceiver() + exists(DataFlow::CallNode call | + call.getMethodName() = "join" and + call.getNumberOfArguments() = 1 and // any string. E.g. ";" or "\n". + call = getANodeExecutedAsCode(s) and + this = call.getReceiver() + ) } override DataFlow::Node getCodeSink() { result = s } From f75b853ae40f1d5f5a98f4d1c3d14d5732a30988 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Fri, 25 Nov 2022 11:08:14 +0100 Subject: [PATCH 11/41] add change-note --- .../src/change-notes/2022-11-25-unsafe-code-construction.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ruby/ql/src/change-notes/2022-11-25-unsafe-code-construction.md diff --git a/ruby/ql/src/change-notes/2022-11-25-unsafe-code-construction.md b/ruby/ql/src/change-notes/2022-11-25-unsafe-code-construction.md new file mode 100644 index 00000000000..485a902693b --- /dev/null +++ b/ruby/ql/src/change-notes/2022-11-25-unsafe-code-construction.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `rb/unsafe-code-construction`, to detect libraries that unsafely construct code from their inputs. From fd7442868f7a90ae221db5bf27dd1fd10e7dc936 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 28 Nov 2022 13:45:24 +0100 Subject: [PATCH 12/41] fix copy-pate error in UnsafeCodeConstructionQuery.qll --- .../ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll index a5670a783e4..68e8c56100c 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll @@ -12,7 +12,7 @@ private import codeql.ruby.TaintTracking private import codeql.ruby.dataflow.BarrierGuards /** - * A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities. + * A taint-tracking configuration for detecting code constructed from library input vulnerabilities. */ class Configuration extends TaintTracking::Configuration { Configuration() { this = "UnsafeShellCommandConstruction" } From d95a4a7bafc2ea4fc0ac8195c9bf890285821842 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 13 Dec 2022 19:33:45 +0100 Subject: [PATCH 13/41] add a second example of how to use module_eval without constructing a code-string --- .../cwe-094/UnsafeCodeConstruction.qhelp | 27 +++++++++++++++++-- .../examples/UnsafeCodeConstruction2.rb | 17 ++++++++++++ .../examples/UnsafeCodeConstruction2Safe.rb | 13 +++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2.rb create mode 100644 ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2Safe.rb diff --git a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp index c430259b029..34227b85539 100644 --- a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp +++ b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp @@ -25,7 +25,7 @@ inputs, or avoid constructing code in the first place.

    -The following example shows two methods implemented using `eval`: a simple +The following example shows two methods implemented using eval: a simple deserialization routine and a getter method. If untrusted inputs are used with these methods, then an attacker might be able to execute arbitrary code on the system. @@ -35,7 +35,7 @@ then an attacker might be able to execute arbitrary code on the system.

    To avoid this problem, either properly document that the function is potentially -unsafe, or use an alternative solution such as `JSON.parse` or another library, like in the examples below, +unsafe, or use an alternative solution such as JSON.parse or another library, like in the examples below, that does not allow arbitrary code to be executed.

    @@ -43,6 +43,29 @@ that does not allow arbitrary code to be executed.     + +

    +As another example, consider the below code which dynamically constructs +a class that has a getter method with a custom name. +

    + + + +

    +The example dynamically constructs a string which is then executed using module_eval. +This code will break if the specified name is not a valid Ruby identifier, and +if the value is controlled by an attacker, then this could lead to code injection. +

    + +

    +A more robust implementation, that is also immune to code injection, +can be made by using module_eval with a block and using define_method +to define the getter method. +

    + + +     +
  • OWASP: diff --git a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2.rb b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2.rb new file mode 100644 index 00000000000..3452dfab665 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2.rb @@ -0,0 +1,17 @@ +require 'json' + +module BadMakeGetter + # Makes a class with a method named `getter_name` that returns `val` + def self.define_getter_class(getter_name, val) + new_class = Class.new + new_class.module_eval <<-END + def #{getter_name} + #{JSON.dump(val)} + end + END + new_class + end +end + +one = BadMakeGetter.define_getter_class(:one, "foo") +puts "One is #{one.new.one}" \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2Safe.rb b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2Safe.rb new file mode 100644 index 00000000000..6c64684b183 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2Safe.rb @@ -0,0 +1,13 @@ +# Uses `define_method` instead of constructing a string +module GoodMakeGetter + def self.define_getter_class(getter_name, val) + new_class = Class.new + new_class.module_eval do + define_method(getter_name) { val } + end + new_class + end +end + +two = GoodMakeGetter.define_getter_class(:two, "bar") +puts "Two is #{two.new.two}" From d3812f5906db7e01f0b9d50a6e8b3efd4ab1c9f8 Mon Sep 17 00:00:00 2001 From: Harry Maclean Date: Wed, 28 Dec 2022 11:20:56 +1300 Subject: [PATCH 14/41] Ruby: Add another code injection example to qhelp --- .../cwe-094/UnsafeCodeConstruction.qhelp | 26 +++++++++++++++++++ .../examples/UnsafeCodeConstruction3.rb | 12 +++++++++ .../examples/UnsafeCodeConstruction3Safe.rb | 9 +++++++ 3 files changed, 47 insertions(+) create mode 100644 ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3.rb create mode 100644 ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb diff --git a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp index 34227b85539..92821f9d759 100644 --- a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp +++ b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp @@ -66,6 +66,26 @@ to define the getter method. + +

    +This example dynamically registers a method on another class which +forwards its arguments to the registering module. This approach uses +module_eval and string interpolation to construct class variables +and methods. +

    + + + +

    +A safer approach is to use class_variable_set and +class_variable_get along with define_method. String +interpolation is still used to construct the class variable name, but this is +safe because class_variable_set is not susceptible to code injection. +

    + + +     +
  • OWASP: @@ -74,5 +94,11 @@ OWASP:
  • Wikipedia: Code Injection.
    • +
  • +Ruby documentation: define_method. +
    • +
  • +Ruby documentation: class_variable_set. +
    •  diff --git a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3.rb b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3.rb new file mode 100644 index 00000000000..69ea7e03e7b --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3.rb @@ -0,0 +1,12 @@ +module Invoker + def attach(klass, name) + invoker = self + klass.module_eval <<-CODE + @@#{name} = invoker + + def #{name}(*args) + @@#{name}.call(*args) + end + CODE + end +end diff --git a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb new file mode 100644 index 00000000000..d18fbf78d98 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb @@ -0,0 +1,9 @@ +module Invoker + def attach(klass, name) + var = :"@@#{name}" + klass.class_variable_set(var, self) + klass.define_method(name) do |*args| + self.class.class_variable_get(var).call(*args) + end + end +end From a6571a05aba788fbc65550195e3c1427631a279d Mon Sep 17 00:00:00 2001 From: Harry Maclean Date: Wed, 28 Dec 2022 11:34:55 +1300 Subject: [PATCH 15/41] Ruby: Include send example in qhelp --- .../queries/security/cwe-094/UnsafeCodeConstruction.qhelp | 8 +++++++- .../security/cwe-094/examples/UnsafeCodeConstruction3.rb | 7 +++---- .../cwe-094/examples/UnsafeCodeConstruction3Safe.rb | 7 ++++--- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp index 92821f9d759..97c7adf4c87 100644 --- a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp +++ b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp @@ -69,7 +69,7 @@ to define the getter method.

    This example dynamically registers a method on another class which -forwards its arguments to the registering module. This approach uses +forwards its arguments to a target class. This approach uses module_eval and string interpolation to construct class variables and methods.

    @@ -81,6 +81,12 @@ A safer approach is to use class_variable_set and class_variable_get along with define_method. String interpolation is still used to construct the class variable name, but this is safe because class_variable_set is not susceptible to code injection. +To construct a dynamic method call we use send, which is ulnerable +to code injection: if an attacker can control the first argument, they can call +any method on the receiver. However this is less powerful than being able to run +arbitrary Ruby code, so it is an improvement in security. We also document to +callers that they should not pass arbitrary user data to the name +parameter.

    diff --git a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3.rb b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3.rb index 69ea7e03e7b..e1a81bafe52 100644 --- a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3.rb +++ b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3.rb @@ -1,11 +1,10 @@ module Invoker - def attach(klass, name) - invoker = self + def attach(klass, name, target) klass.module_eval <<-CODE - @@#{name} = invoker + @@#{name} = target def #{name}(*args) - @@#{name}.call(*args) + @@#{name}.#{name}(*args) end CODE end diff --git a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb index d18fbf78d98..65608f8aa20 100644 --- a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb +++ b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb @@ -1,9 +1,10 @@ module Invoker - def attach(klass, name) + # Do not pass arbitrary user input to +name+. + def attach(klass, name, target) var = :"@@#{name}" - klass.class_variable_set(var, self) + klass.class_variable_set(var, target) klass.define_method(name) do |*args| - self.class.class_variable_get(var).call(*args) + self.class.class_variable_get(var).send(name, *args) end end end From 3815a5a096b5b2f6083c04245a4f23a5d2d2a225 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 2 Jan 2023 10:17:08 +0100 Subject: [PATCH 16/41] fix qhelp syntax --- .../queries/security/cwe-094/UnsafeCodeConstruction.qhelp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp index 97c7adf4c87..63f566aaa57 100644 --- a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp +++ b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp @@ -80,7 +80,7 @@ and methods. A safer approach is to use class_variable_set and class_variable_get along with define_method. String interpolation is still used to construct the class variable name, but this is -safe because class_variable_set is not susceptible to code injection. +safe because class_variable_set is not susceptible to code injection. To construct a dynamic method call we use send, which is ulnerable to code injection: if an attacker can control the first argument, they can call any method on the receiver. However this is less powerful than being able to run @@ -101,10 +101,10 @@ OWASP: Wikipedia: Code Injection.
  • -Ruby documentation: define_method. +Ruby documentation: define_method.
  • -Ruby documentation: class_variable_set. +Ruby documentation: class_variable_set.
    • From 3811eae67946eba9814c4945fd77e554890dc76c Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 2 Jan 2023 11:26:58 +0100 Subject: [PATCH 17/41] simplify the qhelp for unsafe-code-construction The `send()` example is not flagged by any current query, so it was weird talking about it as "vulnerable". --- .../cwe-094/UnsafeCodeConstruction.qhelp | 19 ++++++++++--------- .../examples/UnsafeCodeConstruction3Safe.rb | 1 - 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp index 63f566aaa57..6936aee149d 100644 --- a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp +++ b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp @@ -54,11 +54,11 @@ a class that has a getter method with a custom name.

    The example dynamically constructs a string which is then executed using module_eval. This code will break if the specified name is not a valid Ruby identifier, and -if the value is controlled by an attacker, then this could lead to code injection. +if the value is controlled by an attacker, then this could lead to code-injection.

    -A more robust implementation, that is also immune to code injection, +A more robust implementation, that is also immune to code-injection, can be made by using module_eval with a block and using define_method to define the getter method.

    @@ -80,13 +80,14 @@ and methods. A safer approach is to use class_variable_set and class_variable_get along with define_method. String interpolation is still used to construct the class variable name, but this is -safe because class_variable_set is not susceptible to code injection. -To construct a dynamic method call we use send, which is ulnerable -to code injection: if an attacker can control the first argument, they can call -any method on the receiver. However this is less powerful than being able to run -arbitrary Ruby code, so it is an improvement in security. We also document to -callers that they should not pass arbitrary user data to the name -parameter. +safe because class_variable_set is not susceptible to code-injection. +

    + +

    +send is used to dynamically call the method specified by name. +This is a more robust alternative than the previous example, because it does not allow +arbitrary code to be executed, but it does still allow for any method to be called +on the target object.

    diff --git a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb index 65608f8aa20..ac32224ce72 100644 --- a/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb +++ b/ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3Safe.rb @@ -1,5 +1,4 @@ module Invoker - # Do not pass arbitrary user input to +name+. def attach(klass, name, target) var = :"@@#{name}" klass.class_variable_set(var, target) From d9176541c63639425155986462f91b67c7f7d083 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 5 Jan 2023 20:02:54 +0100 Subject: [PATCH 18/41] Apply suggestions from code review Co-authored-by: Alex Ford --- .../ruby/security/UnsafeCodeConstructionCustomizations.qll | 4 ++-- .../src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll index b6c9c3b5219..68c894f4705 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll @@ -81,10 +81,10 @@ module UnsafeCodeConstruction { * A string constructed from a string-literal (e.g. `"foo #{sink}"`), * where the resulting string ends up being executed as a code. */ - class StringFormatAsSink extends Sink { + class StringInterpolationAsSink extends Sink { Concepts::CodeExecution s; - StringFormatAsSink() { + StringInterpolationAsSink() { exists(Ast::StringlikeLiteral lit | any(DataFlow::Node n | n.asExpr().getExpr() = lit) = getANodeExecutedAsCode(s) and this.asExpr().getExpr() = lit.getComponent(_) diff --git a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp index 6936aee149d..f6dcb452661 100644 --- a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp +++ b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp @@ -69,7 +69,7 @@ to define the getter method.

    This example dynamically registers a method on another class which -forwards its arguments to a target class. This approach uses +forwards its arguments to a target object. This approach uses module_eval and string interpolation to construct class variables and methods.

    From f98ff65b11c1c07565283c05ba4b759c10d65832 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 5 Jan 2023 20:03:51 +0100 Subject: [PATCH 19/41] use eval() instead of send() in test --- .../security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb index 64bba0aadc6..27dd1d81f01 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb +++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb @@ -22,7 +22,7 @@ class Foobar end def named_code(code) - foo.send("def \n #{code} \n end") # OK - parameter is named code + eval("def \n #{code} \n end") # OK - parameter is named code end def joinStuff(my_arr) From aad56097ac93e1b5c7d217d91e201e1f6141dd79 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 30 Nov 2022 13:04:10 +0100 Subject: [PATCH 20/41] Add Cleartext Loggin query for Swift. With some caveats: see TODO comments and failing tests. --- .../codeql/swift/dataflow/ExternalFlow.qll | 1 + .../swift/security/CleartextLogging.qll | 67 +++++++++ .../swift/security/CleartextLoggingQuery.qll | 32 +++++ .../Security/CWE-312/CleartextLogging.qhelp | 50 +++++++ .../Security/CWE-312/CleartextLogging.ql | 24 ++++ .../CWE-312/CleartextLoggingBad.swift | 0 .../CWE-312/CleartextLoggingGood.swift | 0 .../CWE-312/CleartextLoggingTest.expected | 0 .../Security/CWE-312/CleartextLoggingTest.ql | 24 ++++ .../CWE-312/cleartextLoggingTest.swift | 130 ++++++++++++++++++ 10 files changed, 328 insertions(+) create mode 100644 swift/ql/lib/codeql/swift/security/CleartextLogging.qll create mode 100644 swift/ql/lib/codeql/swift/security/CleartextLoggingQuery.qll create mode 100644 swift/ql/src/queries/Security/CWE-312/CleartextLogging.qhelp create mode 100644 swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql create mode 100644 swift/ql/src/queries/Security/CWE-312/CleartextLoggingBad.swift create mode 100644 swift/ql/src/queries/Security/CWE-312/CleartextLoggingGood.swift create mode 100644 swift/ql/test/query-tests/Security/CWE-312/CleartextLoggingTest.expected create mode 100644 swift/ql/test/query-tests/Security/CWE-312/CleartextLoggingTest.ql create mode 100644 swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift diff --git a/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll b/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll index 2c90d1e7351..60f8bc9782b 100644 --- a/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll +++ b/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll @@ -89,6 +89,7 @@ private module Frameworks { private import codeql.swift.frameworks.StandardLibrary.UrlSession private import codeql.swift.frameworks.StandardLibrary.WebView private import codeql.swift.frameworks.Alamofire.Alamofire + private import codeql.swift.security.CleartextLogging private import codeql.swift.security.PathInjection private import codeql.swift.security.PredicateInjection } diff --git a/swift/ql/lib/codeql/swift/security/CleartextLogging.qll b/swift/ql/lib/codeql/swift/security/CleartextLogging.qll new file mode 100644 index 00000000000..9e5e551effd --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/CleartextLogging.qll @@ -0,0 +1,67 @@ +/** Provides classes and predicates to reason about cleartext logging of sensitive data vulnerabilities. */ + +import swift +private import codeql.swift.dataflow.DataFlow +private import codeql.swift.dataflow.ExternalFlow +private import codeql.swift.security.SensitiveExprs + +/** A data flow sink for cleartext logging of sensitive data vulnerabilities. */ +abstract class CleartextLoggingSink extends DataFlow::Node { } + +/** A sanitizer for cleartext logging of sensitive data vulnerabilities. */ +abstract class CleartextLoggingSanitizer extends DataFlow::Node { } + +/** + * A unit class for adding additional taint steps. + * + * Extend this class to add additional taint steps that should apply to paths related to + * cleartext logging of sensitive data vulnerabilities. + */ +class CleartextLoggingAdditionalTaintStep extends Unit { + /** + * Holds if the step from `n1` to `n2` should be considered a taint + * step for flows related to cleartext logging of sensitive data vulnerabilities. + */ + abstract predicate step(DataFlow::Node n1, DataFlow::Node n2); +} + +private class DefaultCleartextLoggingSink extends CleartextLoggingSink { + DefaultCleartextLoggingSink() { sinkNode(this, "logging") } +} + +// TODO: Remove this. It shouldn't be necessary. +private class EncryptionCleartextLoggingSanitizer extends CleartextLoggingSanitizer { + EncryptionCleartextLoggingSanitizer() { this.asExpr() instanceof EncryptedExpr } +} + +/* + * TODO: Add a sanitizer for the OsLogMessage interpolation with .private/.sensitive privacy options, + * or restrict the sinks to require .public interpolation depending on what the default behavior is. + */ + +private class LoggingSinks extends SinkModelCsv { + override predicate row(string row) { + row = + [ + ";;false;print(_:separator:terminator:);;;Argument[0].ArrayElement;logging", + ";;false;print(_:separator:terminator:);;;Argument[1..2];logging", + ";;false;print(_:separator:terminator:toStream:);;;Argument[0].ArrayElement;logging", + ";;false;print(_:separator:terminator:toStream:);;;Argument[1..2];logging", + ";;false;NSLog(_:_:);;;Argument[0];logging", + ";;false;NSLog(_:_:);;;Argument[1].ArrayElement;logging", + ";;false;NSLogv(_:_:);;;Argument[0];logging", + ";;false;NSLogv(_:_:);;;Argument[1].ArrayElement;logging", + ";;false;vfprintf(_:_:_:);;;Agument[1..2];logging", + ";Logger;true;log(_:);;;Argument[0];logging", + ";Logger;true;log(level:_:);;;Argument[1];logging", + ";Logger;true;trace(_:);;;Argument[1];logging", + ";Logger;true;debug(_:);;;Argument[1];logging", + ";Logger;true;info(_:);;;Argument[1];logging", + ";Logger;true;notice(_:);;;Argument[1];logging", + ";Logger;true;warning(_:);;;Argument[1];logging", + ";Logger;true;error(_:);;;Argument[1];logging", + ";Logger;true;critical(_:);;;Argument[1];logging", + ";Logger;true;fault(_:);;;Argument[1];logging", + ] + } +} diff --git a/swift/ql/lib/codeql/swift/security/CleartextLoggingQuery.qll b/swift/ql/lib/codeql/swift/security/CleartextLoggingQuery.qll new file mode 100644 index 00000000000..d67f66e74b9 --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/CleartextLoggingQuery.qll @@ -0,0 +1,32 @@ +/** + * Provides a taint-tracking configuration for reasoning about cleartext logging of + * sensitive data vulnerabilities. + */ + +import swift +private import codeql.swift.dataflow.DataFlow +private import codeql.swift.dataflow.TaintTracking +private import codeql.swift.security.CleartextLogging +private import codeql.swift.security.SensitiveExprs + +/** + * A taint-tracking configuration for cleartext logging of sensitive data vulnerabilities. + */ +class CleartextLoggingConfiguration extends TaintTracking::Configuration { + CleartextLoggingConfiguration() { this = "CleartextLoggingConfiguration" } + + override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof SensitiveExpr } + + override predicate isSink(DataFlow::Node sink) { sink instanceof CleartextLoggingSink } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof CleartextLoggingSanitizer + } + + // Disregard paths that contain other paths. This helps with performance. + override predicate isSanitizerIn(DataFlow::Node node) { this.isSource(node) } + + override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) { + any(CleartextLoggingAdditionalTaintStep s).step(n1, n2) + } +} diff --git a/swift/ql/src/queries/Security/CWE-312/CleartextLogging.qhelp b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.qhelp new file mode 100644 index 00000000000..fc5c7ed6f5e --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.qhelp @@ -0,0 +1,50 @@ + + + + +

    +Sensitive information that is logged unencrypted is accessible to an attacker +who gains access to the logs. +

    +     + + +

    +Ensure that sensitive information is always encrypted or obfuscated before being +logged. +

    + +

    +In general, decrypt sensitive information only at the point where it is +necessary for it to be used in cleartext. +

    + +

    +Be aware that external processes often store the standard out and +standard error streams of the application, causing logged sensitive +information to be stored. +

    +     + + +

    +The following example code logs user credentials (in this case, their password) +in plain text: +

    + +

    +Instead, the credentials should be encrypted, obfuscated, or omitted entirely: +

    + +     + + + +
  • M. Dowd, J. McDonald and J. Schuhm, The Art of Software Security Assessment, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.
    • +
  • M. Howard and D. LeBlanc, Writing Secure Code, 2nd Edition, Chapter 9 - 'Protecting Secret Data', p. 299. Microsoft, 2002.
    • +
  • OWASP: Password Plaintext Storage.
    • + +     +     diff --git a/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql new file mode 100644 index 00000000000..2b21b5da1bc --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql @@ -0,0 +1,24 @@ +/** + * @name Clear-text logging of sensitive information + * @description Logging sensitive information without encryption or hashing can + * expose it to an attacker. + * @kind path-problem + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @id swift/clear-text-logging + * @tags security + * external/cwe/cwe-312 + * external/cwe/cwe-359 + * external/cwe/cwe-532 + */ + +import swift +import codeql.swift.dataflow.DataFlow +import codeql.swift.security.CleartextLoggingQuery +import DataFlow::PathGraph + +from DataFlow::PathNode src, DataFlow::PathNode sink +where any(CleartextLoggingConfiguration c).hasFlowPath(src, sink) +select sink.getNode(), src, sink, "This $@ is written to a log file.", src.getNode(), + "potentially sensitive information" diff --git a/swift/ql/src/queries/Security/CWE-312/CleartextLoggingBad.swift b/swift/ql/src/queries/Security/CWE-312/CleartextLoggingBad.swift new file mode 100644 index 00000000000..e69de29bb2d diff --git a/swift/ql/src/queries/Security/CWE-312/CleartextLoggingGood.swift b/swift/ql/src/queries/Security/CWE-312/CleartextLoggingGood.swift new file mode 100644 index 00000000000..e69de29bb2d diff --git a/swift/ql/test/query-tests/Security/CWE-312/CleartextLoggingTest.expected b/swift/ql/test/query-tests/Security/CWE-312/CleartextLoggingTest.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/swift/ql/test/query-tests/Security/CWE-312/CleartextLoggingTest.ql b/swift/ql/test/query-tests/Security/CWE-312/CleartextLoggingTest.ql new file mode 100644 index 00000000000..d57dc15bbad --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-312/CleartextLoggingTest.ql @@ -0,0 +1,24 @@ +import swift +import codeql.swift.dataflow.DataFlow +import codeql.swift.security.CleartextLoggingQuery +import TestUtilities.InlineExpectationsTest + +class CleartextLogging extends InlineExpectationsTest { + CleartextLogging() { this = "CleartextLogging" } + + override string getARelevantTag() { result = "hasCleartextLogging" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + exists( + CleartextLoggingConfiguration config, DataFlow::Node source, DataFlow::Node sink, + Expr sinkExpr + | + config.hasFlow(source, sink) and + sinkExpr = sink.asExpr() and + location = sinkExpr.getLocation() and + element = sinkExpr.toString() and + tag = "hasCleartextLogging" and + value = source.asExpr().getLocation().getStartLine().toString() + ) + } +} diff --git a/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift new file mode 100644 index 00000000000..659ff02840d --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift @@ -0,0 +1,130 @@ +// --- stubs --- + +func NSLog(_ format: String, _ args: CVarArg...) {} +func NSLogv(_ format: String, _ args: CVaListPointer) {} + +struct OSLogType : RawRepresentable { + static let `default` = OSLogType(rawValue: 0) + let rawValue: UInt8 + init(rawValue: UInt8) { self.rawValue = rawValue} +} + +struct OSLogStringAlignment { + static var none = OSLogStringAlignment() +} + +struct OSLogPrivacy { + enum Mask { case none } + static var auto = OSLogPrivacy() + static var `private` = OSLogPrivacy() + static var sensitive = OSLogPrivacy() + + static func auto(mask: OSLogPrivacy.Mask) -> OSLogPrivacy { return .auto } + static func `private`(mask: OSLogPrivacy.Mask) -> OSLogPrivacy { return .private } + static func `sensitive`(mask: OSLogPrivacy.Mask) -> OSLogPrivacy { return .sensitive } +} + +struct OSLogInterpolation : StringInterpolationProtocol { + typealias StringLiteralType = String + init(literalCapacity: Int, interpolationCount: Int) {} + func appendLiteral(_: Self.StringLiteralType) {} + mutating func appendInterpolation(_ argumentString: @autoclosure @escaping () -> String, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ argumentString: @autoclosure @escaping () -> String, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto, attributes: String) {} +} + +struct OSLogMessage : ExpressibleByStringInterpolation { + typealias StringInterpolation = OSLogInterpolation + typealias StringLiteralType = String + typealias ExtendedGraphemeClusterLiteralType = String + typealias UnicodeScalarLiteralType = String + init(stringInterpolation: OSLogInterpolation) {} + init(stringLiteral: String) {} + init(extendedGraphemeClusterLiteral: String) {} + init(unicodeScalarLiteral: String) {} +} + +struct Logger { + func log(_ message: OSLogMessage) {} + func log(level: OSLogType, _ message: OSLogMessage) {} + func notice(_: OSLogMessage) {} + func debug(_: OSLogMessage) {} + func trace(_: OSLogMessage) {} + func info(_: OSLogMessage) {} + func error(_: OSLogMessage) {} + func warning(_: OSLogMessage) {} + func fault(_: OSLogMessage) {} + func critical(_: OSLogMessage) {} + +} + +// --- tests --- + +func test1(password: String, passwordHash : String) { + print(password) // $ MISSING: hasCleartextLogging=63 + print(password, separator: "") // $ MISSING: $ hasCleartextLogging=64 + print("", separator: password) // $ hasCleartextLogging=65 + print(password, separator: "", terminator: "") // $ MISSING: hasCleartextLogging=66 + print("", separator: password, terminator: "") // $ hasCleartextLogging=67 + print("", separator: "", terminator: password) // $ hasCleartextLogging=68 + + NSLog(password) // $ hasCleartextLogging=70 + NSLog("%@", password as! CVarArg) // $ MISSING: hasCleartextLogging=71 + NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ MISSING: hasCleartextLogging=72 + NSLog("\(password)") // $ hasCleartextLogging=73 + NSLogv("%@", getVaList([password as! CVarArg])) // $ MISSING: hasCleartextLogging=74 + NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ MISSING: hasCleartextLogging=75 + + let log = Logger() + log.log("\(password)") // $ hasCleartextLogging=78 + log.log("\(password, privacy: .private)") // Safe + log.log(level: .default, "\(password)") // $ hasCleartextLogging=80 + log.trace("\(password)") // $ hasCleartextLogging=81 + log.debug("\(password)") // $ hasCleartextLogging=82 + log.info("\(password)") // $ hasCleartextLogging=83 + log.notice("\(password)") // $ hasCleartextLogging=84 + log.warning("\(password)") // $ hasCleartextLogging=85 + log.error("\(password)") // $ hasCleartextLogging=86 + log.critical("\(password)") // $ hasCleartextLogging=87 + log.fault("\(password)") // $ hasCleartextLogging=88 +} +/* +class MyClass { + var harmless = "abc" + var password = "123" +} + +func test3(x: String) { + // alternative evidence of sensitivity... + + UserDefaults.standard.set(x, forKey: "myKey") // $ MISSING: hasCleartextLogging + doSomething(password: x); + UserDefaults.standard.set(x, forKey: "myKey") // $ hasCleartextLogging + + let y = getPassword(); + UserDefaults.standard.set(y, forKey: "myKey") // $ hasCleartextLogging + + let z = MyClass() + UserDefaults.standard.set(z.harmless, forKey: "myKey") // Safe + UserDefaults.standard.set(z.password, forKey: "myKey") // $ hasCleartextLogging +} + +func test4(passwd: String) { + // sanitizers... + + var x = passwd; + var y = passwd; + var z = passwd; + + UserDefaults.standard.set(x, forKey: "myKey") // $ hasCleartextLogging + UserDefaults.standard.set(y, forKey: "myKey") // $ hasCleartextLogging + UserDefaults.standard.set(z, forKey: "myKey") // $ hasCleartextLogging + + x = encrypt(x); + hash(data: &y); + z = ""; + + UserDefaults.standard.set(x, forKey: "myKey") // Safe + UserDefaults.standard.set(y, forKey: "myKey") // Safe + UserDefaults.standard.set(z, forKey: "myKey") // Safe +} +*/ \ No newline at end of file From b203a9eb6e90344c6955c5a74a7075a53253a056 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 7 Dec 2022 18:31:32 +0100 Subject: [PATCH 21/41] Add a sanitizer for OSLogPrivacy options Add test cases to verify how the sanitizer behaves depending on the argument type and the privacy option being used. --- .../swift/security/CleartextLogging.qll | 50 ++++++++++-- .../CWE-312/cleartextLoggingTest.swift | 77 +++++++++++++------ 2 files changed, 98 insertions(+), 29 deletions(-) diff --git a/swift/ql/lib/codeql/swift/security/CleartextLogging.qll b/swift/ql/lib/codeql/swift/security/CleartextLogging.qll index 9e5e551effd..e6261543077 100644 --- a/swift/ql/lib/codeql/swift/security/CleartextLogging.qll +++ b/swift/ql/lib/codeql/swift/security/CleartextLogging.qll @@ -29,15 +29,51 @@ private class DefaultCleartextLoggingSink extends CleartextLoggingSink { DefaultCleartextLoggingSink() { sinkNode(this, "logging") } } -// TODO: Remove this. It shouldn't be necessary. -private class EncryptionCleartextLoggingSanitizer extends CleartextLoggingSanitizer { - EncryptionCleartextLoggingSanitizer() { this.asExpr() instanceof EncryptedExpr } +/** + * A sanitizer for `OSLogMessage`s configured with the appropriate privacy option. + * Numeric and boolean arguments aren't redacted unless the `public` option is used. + * Arguments of other types are always redacted unless the `private` or `sensitive` options are used. + */ +private class OsLogPrivacyCleartextLoggingSanitizer extends CleartextLoggingSanitizer { + OsLogPrivacyCleartextLoggingSanitizer() { + exists(CallExpr c, AutoClosureExpr e | + c.getStaticTarget().getName().matches("appendInterpolation(_:%privacy:%)") and + c.getArgument(0).getExpr() = e and + this.asExpr() = e + | + e.getExpr().getType() instanceof OsLogNonRedactedType and + c.getArgumentWithLabel("privacy").getExpr().(OsLogPrivacyRef).isSafe() + or + not e.getExpr().getType() instanceof OsLogNonRedactedType and + not c.getArgumentWithLabel("privacy").getExpr().(OsLogPrivacyRef).isPublic() + ) + } } -/* - * TODO: Add a sanitizer for the OsLogMessage interpolation with .private/.sensitive privacy options, - * or restrict the sinks to require .public interpolation depending on what the default behavior is. - */ +/** A type that isn't redacted by default in an `OSLogMessage`. */ +private class OsLogNonRedactedType extends Type { + OsLogNonRedactedType() { + this.getName() = [["", "U"] + "Int" + ["", "16", "32", "64"], "Double", "Float", "Bool"] + } +} + +/** A reference to a field of `OsLogPrivacy`. */ +private class OsLogPrivacyRef extends MemberRefExpr { + string optionName; + + OsLogPrivacyRef() { + exists(FieldDecl f | this.getMember() = f | + f.getEnclosingDecl().(NominalTypeDecl).getName() = "OSLogPrivacy" and + optionName = f.getName() + ) + } + + /** Holds if this is a safe privacy option (private or sensitive). */ + predicate isSafe() { optionName = ["private", "sensitive"] } + + /** Holds if this is a public (that is, unsafe) privacy option. */ + predicate isPublic() { optionName = "public" } +} private class LoggingSinks extends SinkModelCsv { override predicate row(string row) { diff --git a/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift index 659ff02840d..21d1387fb79 100644 --- a/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift +++ b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift @@ -13,10 +13,16 @@ struct OSLogStringAlignment { static var none = OSLogStringAlignment() } +enum OSLogIntegerFormatting { case decimal } +enum OSLogInt32ExtendedFormat { case none } +enum OSLogFloatFormatting { case fixed } +enum OSLogBoolFormat { case truth } + struct OSLogPrivacy { enum Mask { case none } static var auto = OSLogPrivacy() static var `private` = OSLogPrivacy() + static var `public` = OSLogPrivacy() static var sensitive = OSLogPrivacy() static func auto(mask: OSLogPrivacy.Mask) -> OSLogPrivacy { return .auto } @@ -30,6 +36,23 @@ struct OSLogInterpolation : StringInterpolationProtocol { func appendLiteral(_: Self.StringLiteralType) {} mutating func appendInterpolation(_ argumentString: @autoclosure @escaping () -> String, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} mutating func appendInterpolation(_ argumentString: @autoclosure @escaping () -> String, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto, attributes: String) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int8, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int16, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int32, format: OSLogInt32ExtendedFormat, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int32, format: OSLogInt32ExtendedFormat, privacy: OSLogPrivacy = .auto, attributes: String) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int32, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int64, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt8, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt16, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt32, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt64, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Double, format: OSLogFloatFormatting = .fixed, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Double, format: OSLogFloatFormatting = .fixed, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto, attributes: String) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Float,format: OSLogFloatFormatting = .fixed, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {} + mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Float, format: OSLogFloatFormatting = .fixed, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto, attributes: String) {} + mutating func appendInterpolation(_ boolean: @autoclosure @escaping () -> Bool, format: OSLogBoolFormat = .truth, privacy: OSLogPrivacy = .auto) {} } struct OSLogMessage : ExpressibleByStringInterpolation { @@ -60,32 +83,42 @@ struct Logger { // --- tests --- func test1(password: String, passwordHash : String) { - print(password) // $ MISSING: hasCleartextLogging=63 - print(password, separator: "") // $ MISSING: $ hasCleartextLogging=64 - print("", separator: password) // $ hasCleartextLogging=65 - print(password, separator: "", terminator: "") // $ MISSING: hasCleartextLogging=66 - print("", separator: password, terminator: "") // $ hasCleartextLogging=67 - print("", separator: "", terminator: password) // $ hasCleartextLogging=68 + print(password) // $ MISSING: hasCleartextLogging=86 + print(password, separator: "") // $ MISSING: $ hasCleartextLogging=97 + print("", separator: password) // $ hasCleartextLogging=88 + print(password, separator: "", terminator: "") // $ MISSING: hasCleartextLogging=89 + print("", separator: password, terminator: "") // $ hasCleartextLogging=90 + print("", separator: "", terminator: password) // $ hasCleartextLogging=91 - NSLog(password) // $ hasCleartextLogging=70 - NSLog("%@", password as! CVarArg) // $ MISSING: hasCleartextLogging=71 - NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ MISSING: hasCleartextLogging=72 - NSLog("\(password)") // $ hasCleartextLogging=73 - NSLogv("%@", getVaList([password as! CVarArg])) // $ MISSING: hasCleartextLogging=74 - NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ MISSING: hasCleartextLogging=75 + NSLog(password) // $ hasCleartextLogging=93 + NSLog("%@", password as! CVarArg) // $ MISSING: hasCleartextLogging=94 + NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ MISSING: hasCleartextLogging=95 + NSLog("\(password)") // $ hasCleartextLogging=96 + NSLogv("%@", getVaList([password as! CVarArg])) // $ MISSING: hasCleartextLogging=97 + NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ MISSING: hasCleartextLogging=98 + let bankAccount: Int = 0 let log = Logger() - log.log("\(password)") // $ hasCleartextLogging=78 + // These MISSING test cases will be fixed when we properly generate the CFG around autoclosures. + log.log("\(password)") // Safe + log.log("\(password, privacy: .auto)") // Safe log.log("\(password, privacy: .private)") // Safe - log.log(level: .default, "\(password)") // $ hasCleartextLogging=80 - log.trace("\(password)") // $ hasCleartextLogging=81 - log.debug("\(password)") // $ hasCleartextLogging=82 - log.info("\(password)") // $ hasCleartextLogging=83 - log.notice("\(password)") // $ hasCleartextLogging=84 - log.warning("\(password)") // $ hasCleartextLogging=85 - log.error("\(password)") // $ hasCleartextLogging=86 - log.critical("\(password)") // $ hasCleartextLogging=87 - log.fault("\(password)") // $ hasCleartextLogging=88 + log.log("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=106 + log.log("\(password, privacy: .sensitive)") // Safe + log.log("\(bankAccount)") // $ MISSING: hasCleartextLogging=108 + log.log("\(bankAccount, privacy: .auto)") // $ MISSING: hasCleartextLogging=109 + log.log("\(bankAccount, privacy: .private)") // Safe + log.log("\(bankAccount, privacy: .public)") // $ MISSING: hasCleartextLogging=111 + log.log("\(bankAccount, privacy: .sensitive)") // Safe + log.log(level: .default, "\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=113 + log.trace("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=114 + log.debug("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=115 + log.info("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=116 + log.notice("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=117 + log.warning("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=118 + log.error("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=119 + log.critical("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=120 + log.fault("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=121 } /* class MyClass { From c1f19dd145f2991d23d9ddf74d236a12929b6577 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 9 Dec 2022 11:04:14 +0100 Subject: [PATCH 22/41] Add stub so that tests work on Linux --- .../CWE-312/cleartextLoggingTest.swift | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift index 21d1387fb79..fc4b5021d9d 100644 --- a/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift +++ b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift @@ -2,6 +2,7 @@ func NSLog(_ format: String, _ args: CVarArg...) {} func NSLogv(_ format: String, _ args: CVaListPointer) {} +func getVaList(_ args: [CVarArg]) -> CVaListPointer { return CVaListPointer(_fromUnsafeMutablePointer: UnsafeMutablePointer(bitPattern: 0)!) } struct OSLogType : RawRepresentable { static let `default` = OSLogType(rawValue: 0) @@ -83,19 +84,19 @@ struct Logger { // --- tests --- func test1(password: String, passwordHash : String) { - print(password) // $ MISSING: hasCleartextLogging=86 - print(password, separator: "") // $ MISSING: $ hasCleartextLogging=97 - print("", separator: password) // $ hasCleartextLogging=88 - print(password, separator: "", terminator: "") // $ MISSING: hasCleartextLogging=89 - print("", separator: password, terminator: "") // $ hasCleartextLogging=90 - print("", separator: "", terminator: password) // $ hasCleartextLogging=91 + print(password) // $ MISSING: hasCleartextLogging=87 + print(password, separator: "") // $ MISSING: $ hasCleartextLogging=88 + print("", separator: password) // $ hasCleartextLogging=89 + print(password, separator: "", terminator: "") // $ MISSING: hasCleartextLogging=90 + print("", separator: password, terminator: "") // $ hasCleartextLogging=91 + print("", separator: "", terminator: password) // $ hasCleartextLogging=92 - NSLog(password) // $ hasCleartextLogging=93 - NSLog("%@", password as! CVarArg) // $ MISSING: hasCleartextLogging=94 - NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ MISSING: hasCleartextLogging=95 - NSLog("\(password)") // $ hasCleartextLogging=96 - NSLogv("%@", getVaList([password as! CVarArg])) // $ MISSING: hasCleartextLogging=97 - NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ MISSING: hasCleartextLogging=98 + NSLog(password) // $ hasCleartextLogging=94 + NSLog("%@", password as! CVarArg) // $ MISSING: hasCleartextLogging=95 + NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ MISSING: hasCleartextLogging=96 + NSLog("\(password)") // $ hasCleartextLogging=97 + NSLogv("%@", getVaList([password as! CVarArg])) // $ MISSING: hasCleartextLogging=98 + NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ MISSING: hasCleartextLogging=99 let bankAccount: Int = 0 let log = Logger() @@ -103,22 +104,22 @@ func test1(password: String, passwordHash : String) { log.log("\(password)") // Safe log.log("\(password, privacy: .auto)") // Safe log.log("\(password, privacy: .private)") // Safe - log.log("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=106 + log.log("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=107 log.log("\(password, privacy: .sensitive)") // Safe - log.log("\(bankAccount)") // $ MISSING: hasCleartextLogging=108 - log.log("\(bankAccount, privacy: .auto)") // $ MISSING: hasCleartextLogging=109 + log.log("\(bankAccount)") // $ MISSING: hasCleartextLogging=109 + log.log("\(bankAccount, privacy: .auto)") // $ MISSING: hasCleartextLogging=110 log.log("\(bankAccount, privacy: .private)") // Safe - log.log("\(bankAccount, privacy: .public)") // $ MISSING: hasCleartextLogging=111 + log.log("\(bankAccount, privacy: .public)") // $ MISSING: hasCleartextLogging=112 log.log("\(bankAccount, privacy: .sensitive)") // Safe - log.log(level: .default, "\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=113 - log.trace("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=114 - log.debug("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=115 - log.info("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=116 - log.notice("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=117 - log.warning("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=118 - log.error("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=119 - log.critical("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=120 - log.fault("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=121 + log.log(level: .default, "\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=114 + log.trace("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=115 + log.debug("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=116 + log.info("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=117 + log.notice("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=118 + log.warning("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=119 + log.error("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=120 + log.critical("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=121 + log.fault("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=122 } /* class MyClass { From 7e0869965c32bda2b269453a6a849b1daca71787 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 9 Dec 2022 11:11:07 +0100 Subject: [PATCH 23/41] Uncomment tests --- .../CWE-312/cleartextLoggingTest.swift | 36 +++++-------------- 1 file changed, 9 insertions(+), 27 deletions(-) diff --git a/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift index fc4b5021d9d..6e0fb9038b3 100644 --- a/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift +++ b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift @@ -121,44 +121,26 @@ func test1(password: String, passwordHash : String) { log.critical("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=121 log.fault("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=122 } -/* + class MyClass { var harmless = "abc" var password = "123" } +func getPassword() -> String { return "" } +func doSomething(password: String) { } + func test3(x: String) { // alternative evidence of sensitivity... - UserDefaults.standard.set(x, forKey: "myKey") // $ MISSING: hasCleartextLogging + NSLog(x) // $ MISSING: hasCleartextLogging=137 doSomething(password: x); - UserDefaults.standard.set(x, forKey: "myKey") // $ hasCleartextLogging + NSLog(x) // $ hasCleartextLogging=137 let y = getPassword(); - UserDefaults.standard.set(y, forKey: "myKey") // $ hasCleartextLogging + NSLog(y) // $ hasCleartextLogging=140 let z = MyClass() - UserDefaults.standard.set(z.harmless, forKey: "myKey") // Safe - UserDefaults.standard.set(z.password, forKey: "myKey") // $ hasCleartextLogging + NSLog(z.harmless) // Safe + NSLog(z.password) // $ hasCleartextLogging=145 } - -func test4(passwd: String) { - // sanitizers... - - var x = passwd; - var y = passwd; - var z = passwd; - - UserDefaults.standard.set(x, forKey: "myKey") // $ hasCleartextLogging - UserDefaults.standard.set(y, forKey: "myKey") // $ hasCleartextLogging - UserDefaults.standard.set(z, forKey: "myKey") // $ hasCleartextLogging - - x = encrypt(x); - hash(data: &y); - z = ""; - - UserDefaults.standard.set(x, forKey: "myKey") // Safe - UserDefaults.standard.set(y, forKey: "myKey") // Safe - UserDefaults.standard.set(z, forKey: "myKey") // Safe -} -*/ \ No newline at end of file From 33029b0ed8f4d2e4fd0efc61d9a4ce7953274130 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 9 Dec 2022 11:13:34 +0100 Subject: [PATCH 24/41] Fix sanitizer QLDoc --- swift/ql/lib/codeql/swift/security/CleartextLogging.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/swift/ql/lib/codeql/swift/security/CleartextLogging.qll b/swift/ql/lib/codeql/swift/security/CleartextLogging.qll index e6261543077..309edf16f71 100644 --- a/swift/ql/lib/codeql/swift/security/CleartextLogging.qll +++ b/swift/ql/lib/codeql/swift/security/CleartextLogging.qll @@ -31,8 +31,8 @@ private class DefaultCleartextLoggingSink extends CleartextLoggingSink { /** * A sanitizer for `OSLogMessage`s configured with the appropriate privacy option. - * Numeric and boolean arguments aren't redacted unless the `public` option is used. - * Arguments of other types are always redacted unless the `private` or `sensitive` options are used. + * Numeric and boolean arguments aren't redacted unless the `private` or `sensitive` options are used. + * Arguments of other types are always redacted unless the `public` option is used. */ private class OsLogPrivacyCleartextLoggingSanitizer extends CleartextLoggingSanitizer { OsLogPrivacyCleartextLoggingSanitizer() { From 160d89fb4ed55ac5a2913a53e4ef25e0b601ca9b Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 12 Dec 2022 09:36:11 +0100 Subject: [PATCH 25/41] Add qhelp examples --- swift/ql/src/queries/Security/CWE-312/CleartextLoggingBad.swift | 2 ++ .../ql/src/queries/Security/CWE-312/CleartextLoggingGood.swift | 2 ++ 2 files changed, 4 insertions(+) diff --git a/swift/ql/src/queries/Security/CWE-312/CleartextLoggingBad.swift b/swift/ql/src/queries/Security/CWE-312/CleartextLoggingBad.swift index e69de29bb2d..036001e8717 100644 --- a/swift/ql/src/queries/Security/CWE-312/CleartextLoggingBad.swift +++ b/swift/ql/src/queries/Security/CWE-312/CleartextLoggingBad.swift @@ -0,0 +1,2 @@ +let password = "P@ssw0rd" +NSLog("User password changed to \(password)") diff --git a/swift/ql/src/queries/Security/CWE-312/CleartextLoggingGood.swift b/swift/ql/src/queries/Security/CWE-312/CleartextLoggingGood.swift index e69de29bb2d..1d90ac5e565 100644 --- a/swift/ql/src/queries/Security/CWE-312/CleartextLoggingGood.swift +++ b/swift/ql/src/queries/Security/CWE-312/CleartextLoggingGood.swift @@ -0,0 +1,2 @@ +let password = "P@ssw0rd" +NSLog("User password changed") From 49a41c98ee31f910e9dadce27b68df8c3c831116 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 9 Jan 2023 18:00:22 +0100 Subject: [PATCH 26/41] Test that hashed passwords are 'safe' to log This doesn't seem completely right, but the heuristic approach we have regarding sensitive expressions has to draw the line somewhere. --- .../CWE-312/cleartextLoggingTest.swift | 58 +++++++++++-------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift index 6e0fb9038b3..9151ab0f44f 100644 --- a/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift +++ b/swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift @@ -90,13 +90,16 @@ func test1(password: String, passwordHash : String) { print(password, separator: "", terminator: "") // $ MISSING: hasCleartextLogging=90 print("", separator: password, terminator: "") // $ hasCleartextLogging=91 print("", separator: "", terminator: password) // $ hasCleartextLogging=92 + print(passwordHash) // Safe - NSLog(password) // $ hasCleartextLogging=94 - NSLog("%@", password as! CVarArg) // $ MISSING: hasCleartextLogging=95 - NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ MISSING: hasCleartextLogging=96 - NSLog("\(password)") // $ hasCleartextLogging=97 - NSLogv("%@", getVaList([password as! CVarArg])) // $ MISSING: hasCleartextLogging=98 - NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ MISSING: hasCleartextLogging=99 + NSLog(password) // $ hasCleartextLogging=95 + NSLog("%@", password as! CVarArg) // $ MISSING: hasCleartextLogging=96 + NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ MISSING: hasCleartextLogging=97 + NSLog("\(password)") // $ hasCleartextLogging=98 + NSLogv("%@", getVaList([password as! CVarArg])) // $ MISSING: hasCleartextLogging=99 + NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ MISSING: hasCleartextLogging=100 + NSLog(passwordHash) // SAfe + NSLogv("%@", getVaList([passwordHash as! CVarArg])) // Safe let bankAccount: Int = 0 let log = Logger() @@ -104,22 +107,31 @@ func test1(password: String, passwordHash : String) { log.log("\(password)") // Safe log.log("\(password, privacy: .auto)") // Safe log.log("\(password, privacy: .private)") // Safe - log.log("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=107 + log.log("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=110 + log.log("\(passwordHash, privacy: .public)") // Safe log.log("\(password, privacy: .sensitive)") // Safe - log.log("\(bankAccount)") // $ MISSING: hasCleartextLogging=109 - log.log("\(bankAccount, privacy: .auto)") // $ MISSING: hasCleartextLogging=110 + log.log("\(bankAccount)") // $ MISSING: hasCleartextLogging=113 + log.log("\(bankAccount, privacy: .auto)") // $ MISSING: hasCleartextLogging=114 log.log("\(bankAccount, privacy: .private)") // Safe - log.log("\(bankAccount, privacy: .public)") // $ MISSING: hasCleartextLogging=112 + log.log("\(bankAccount, privacy: .public)") // $ MISSING: hasCleartextLogging=116 log.log("\(bankAccount, privacy: .sensitive)") // Safe - log.log(level: .default, "\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=114 - log.trace("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=115 - log.debug("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=116 - log.info("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=117 - log.notice("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=118 - log.warning("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=119 - log.error("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=120 - log.critical("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=121 - log.fault("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=122 + log.log(level: .default, "\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=118 + log.trace("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=119 + log.trace("\(passwordHash, privacy: .public)") // Safe + log.debug("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=121 + log.debug("\(passwordHash, privacy: .public)") // Safe + log.info("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=123 + log.info("\(passwordHash, privacy: .public)") // Safe + log.notice("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=125 + log.notice("\(passwordHash, privacy: .public)") // Safe + log.warning("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=127 + log.warning("\(passwordHash, privacy: .public)") // Safe + log.error("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=129 + log.error("\(passwordHash, privacy: .public)") // Safe + log.critical("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=131 + log.critical("\(passwordHash, privacy: .public)") // Safe + log.fault("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=133 + log.fault("\(passwordHash, privacy: .public)") // Safe } class MyClass { @@ -133,14 +145,14 @@ func doSomething(password: String) { } func test3(x: String) { // alternative evidence of sensitivity... - NSLog(x) // $ MISSING: hasCleartextLogging=137 + NSLog(x) // $ MISSING: hasCleartextLogging=148 doSomething(password: x); - NSLog(x) // $ hasCleartextLogging=137 + NSLog(x) // $ hasCleartextLogging=149 let y = getPassword(); - NSLog(y) // $ hasCleartextLogging=140 + NSLog(y) // $ hasCleartextLogging=152 let z = MyClass() NSLog(z.harmless) // Safe - NSLog(z.password) // $ hasCleartextLogging=145 + NSLog(z.password) // $ hasCleartextLogging=157 } From 8e0a018673c6968831a9e8612a2ecd45521ca8c8 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 9 Jan 2023 18:05:09 +0100 Subject: [PATCH 27/41] Consider Int8 and UInt8 as OsLogNonRedactedTypes --- swift/ql/lib/codeql/swift/security/CleartextLogging.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/swift/ql/lib/codeql/swift/security/CleartextLogging.qll b/swift/ql/lib/codeql/swift/security/CleartextLogging.qll index 309edf16f71..b3aa3ba414e 100644 --- a/swift/ql/lib/codeql/swift/security/CleartextLogging.qll +++ b/swift/ql/lib/codeql/swift/security/CleartextLogging.qll @@ -53,7 +53,7 @@ private class OsLogPrivacyCleartextLoggingSanitizer extends CleartextLoggingSani /** A type that isn't redacted by default in an `OSLogMessage`. */ private class OsLogNonRedactedType extends Type { OsLogNonRedactedType() { - this.getName() = [["", "U"] + "Int" + ["", "16", "32", "64"], "Double", "Float", "Bool"] + this.getName() = [["", "U"] + "Int" + ["", "8", "16", "32", "64"], "Double", "Float", "Bool"] } } From f2658a0936912267c9e42fadc19952a5ff3943f1 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 10 Jan 2023 12:56:22 +0100 Subject: [PATCH 28/41] apply suggestions from doc review Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- .../src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp index f6dcb452661..32e8c369ef5 100644 --- a/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp +++ b/ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp @@ -5,7 +5,7 @@

    -When a library function dynamically constructs code in a potentially unsafe way, then +When a library function dynamically constructs code in a potentially unsafe way, it's important to document to clients of the library that the function should only be used with trusted inputs. @@ -35,7 +35,7 @@ then an attacker might be able to execute arbitrary code on the system.

    To avoid this problem, either properly document that the function is potentially -unsafe, or use an alternative solution such as JSON.parse or another library, like in the examples below, +unsafe, or use an alternative solution such as JSON.parse or another library that does not allow arbitrary code to be executed.

    From 23a847b1cfca06e56c3962d8ea06798dfba9f92f Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 10 Jan 2023 10:10:36 +0100 Subject: [PATCH 29/41] track shell:true more in js/shell-command-constructed-from-input --- .../ql/lib/semmle/javascript/ApiGraphs.qll | 7 ++++++- ...eShellCommandConstructionCustomizations.qll | 5 +++++ .../UnsafeShellCommandConstruction.expected | 18 ++++++++++++++++++ .../UnsafeShellCommandConstruction/lib/lib.js | 9 +++++++++ 4 files changed, 38 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index 3e708098635..a9c8bca2345 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll @@ -210,7 +210,12 @@ module API { * This is similar to `asSink()` but additionally includes nodes that transitively reach a sink by data flow. * See `asSink()` for examples. */ - DataFlow::Node getAValueReachingSink() { result = Impl::trackDefNode(this.asSink()) } + DataFlow::Node getAValueReachingSink() { + result = Impl::trackDefNode(this.asSink()) + or + // in case `asSink()` is not a `SourceNode`. + result = asSink() + } /** DEPRECATED. This predicate has been renamed to `asSink`. */ deprecated DataFlow::Node getARhs() { result = this.asSink() } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll index ca6920db466..6a2b4d94f35 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll @@ -166,6 +166,11 @@ module UnsafeShellCommandConstruction { .asExpr() .(BooleanLiteral) .getValue() = "true" + or + exists(API::Node node | + node.asSink() = sys.getOptionsArg() and + node.getMember("shell").getAValueReachingSink().mayHaveBooleanValue(true) + ) } /** diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected index de11fb884c9..92cca128c59 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected @@ -282,6 +282,14 @@ nodes | lib/lib.js:543:23:543:26 | name | | lib/lib.js:545:23:545:26 | name | | lib/lib.js:545:23:545:26 | name | +| lib/lib.js:550:39:550:42 | name | +| lib/lib.js:550:39:550:42 | name | +| lib/lib.js:551:33:551:36 | args | +| lib/lib.js:552:23:552:26 | args | +| lib/lib.js:552:23:552:26 | args | +| lib/lib.js:555:25:555:37 | ["-rf", name] | +| lib/lib.js:555:33:555:36 | name | +| lib/lib.js:555:33:555:36 | name | | lib/subLib2/compiled-file.ts:3:26:3:29 | name | | lib/subLib2/compiled-file.ts:3:26:3:29 | name | | lib/subLib2/compiled-file.ts:4:25:4:28 | name | @@ -659,6 +667,14 @@ edges | lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | | lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | | lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | +| lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name | +| lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name | +| lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name | +| lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name | +| lib/lib.js:551:33:551:36 | args | lib/lib.js:552:23:552:26 | args | +| lib/lib.js:551:33:551:36 | args | lib/lib.js:552:23:552:26 | args | +| lib/lib.js:555:25:555:37 | ["-rf", name] | lib/lib.js:551:33:551:36 | args | +| lib/lib.js:555:33:555:36 | name | lib/lib.js:555:25:555:37 | ["-rf", name] | | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | @@ -775,6 +791,8 @@ edges | lib/lib.js:537:11:537:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:537:3:537:27 | cp.exec ... + name) | shell command | | lib/lib.js:543:11:543:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:543:3:543:27 | cp.exec ... + name) | shell command | | lib/lib.js:545:11:545:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:545:3:545:27 | cp.exec ... + name) | shell command | +| lib/lib.js:552:23:552:26 | args | lib/lib.js:550:39:550:42 | name | lib/lib.js:552:23:552:26 | args | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:550:39:550:42 | name | library input | lib/lib.js:552:9:552:38 | cp.spaw ... wnOpts) | shell command | +| lib/lib.js:555:33:555:36 | name | lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:550:39:550:42 | name | library input | lib/lib.js:552:9:552:38 | cp.spaw ... wnOpts) | shell command | | lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/compiled-file.ts:3:26:3:29 | name | library input | lib/subLib2/compiled-file.ts:4:5:4:29 | cp.exec ... + name) | shell command | | lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | lib/subLib2/special-file.js:3:28:3:31 | name | lib/subLib2/special-file.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/special-file.js:3:28:3:31 | name | library input | lib/subLib2/special-file.js:4:2:4:26 | cp.exec ... + name) | shell command | | lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | lib/subLib3/my-file.ts:3:28:3:31 | name | lib/subLib3/my-file.ts:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib3/my-file.ts:3:28:3:31 | name | library input | lib/subLib3/my-file.ts:4:2:4:26 | cp.exec ... + name) | shell command | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js index 64b279d7d05..728ddbfa91e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js @@ -545,3 +545,12 @@ module.exports.sanitizer4 = function (name) { cp.exec("rm -rf " + name); // NOT OK } } + + +module.exports.shellThing = function (name) { + function indirectShell(cmd, args, spawnOpts) { + cp.spawn(cmd, args, spawnOpts); // NOT OK + } + + indirectShell("rm", ["-rf", name], {shell: true}); +} \ No newline at end of file From 43696f5e27db5495d35259099c0a3328e7953a56 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 10 Jan 2023 10:24:19 +0100 Subject: [PATCH 30/41] add explicit this --- javascript/ql/lib/semmle/javascript/ApiGraphs.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index a9c8bca2345..80a5487943a 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll @@ -214,7 +214,7 @@ module API { result = Impl::trackDefNode(this.asSink()) or // in case `asSink()` is not a `SourceNode`. - result = asSink() + result = this.asSink() } /** DEPRECATED. This predicate has been renamed to `asSink`. */ From 71af8ab02281f1021455dfee355c2f5ccf990179 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Fri, 13 Jan 2023 13:18:52 +0100 Subject: [PATCH 31/41] simplifications inspired by review --- javascript/ql/lib/semmle/javascript/ApiGraphs.qll | 7 +------ .../UnsafeShellCommandConstructionCustomizations.qll | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index 80a5487943a..3e708098635 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll @@ -210,12 +210,7 @@ module API { * This is similar to `asSink()` but additionally includes nodes that transitively reach a sink by data flow. * See `asSink()` for examples. */ - DataFlow::Node getAValueReachingSink() { - result = Impl::trackDefNode(this.asSink()) - or - // in case `asSink()` is not a `SourceNode`. - result = this.asSink() - } + DataFlow::Node getAValueReachingSink() { result = Impl::trackDefNode(this.asSink()) } /** DEPRECATED. This predicate has been renamed to `asSink`. */ deprecated DataFlow::Node getARhs() { result = this.asSink() } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll index 6a2b4d94f35..3b140fa4a9f 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll @@ -169,7 +169,7 @@ module UnsafeShellCommandConstruction { or exists(API::Node node | node.asSink() = sys.getOptionsArg() and - node.getMember("shell").getAValueReachingSink().mayHaveBooleanValue(true) + node.getMember("shell").asSink().mayHaveBooleanValue(true) ) } From cf7189bb2896a9cde9cdfad284c4cb5eb1de0cd2 Mon Sep 17 00:00:00 2001 From: jelaiw <14236583+jelaiw@users.noreply.github.com> Date: Fri, 13 Jan 2023 19:16:11 -0600 Subject: [PATCH 32/41] Fix small typo in good/bad code sample. --- java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.java b/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.java index 7d8299f2369..6a66258747f 100644 --- a/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.java +++ b/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.java @@ -7,6 +7,6 @@ byte[] encrypted = cipher.doFinal(input.getBytes("UTF-8")); // ... // GOOD: AES is a strong algorithm -Cipher des = Cipher.getInstance("AES"); +Cipher aes = Cipher.getInstance("AES"); -// ... \ No newline at end of file +// ... From fdb3b65bcea14b162a59a4a6c6b8e7a1d2a43cdd Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 16 Jan 2023 11:57:37 +0100 Subject: [PATCH 33/41] Apply suggestions from code review Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com> --- .../Security/CWE-312/CleartextLogging.qhelp | 18 +++++++----------- .../Security/CWE-312/CleartextLogging.ql | 2 +- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/swift/ql/src/queries/Security/CWE-312/CleartextLogging.qhelp b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.qhelp index fc5c7ed6f5e..5959fe5ef8d 100644 --- a/swift/ql/src/queries/Security/CWE-312/CleartextLogging.qhelp +++ b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.qhelp @@ -5,37 +5,33 @@

    -Sensitive information that is logged unencrypted is accessible to an attacker -who gains access to the logs. +Attackers could gain access to sensitive information that is logged unencrypted.

    -Ensure that sensitive information is always encrypted or obfuscated before being -logged. +Always make sure to encrypt or obfuscate sensitive information before you log it.

    -In general, decrypt sensitive information only at the point where it is -necessary for it to be used in cleartext. +Generally, you should decrypt sensitive information only at the point where it is necessary for it to be used in cleartext.

    -Be aware that external processes often store the standard out and -standard error streams of the application, causing logged sensitive -information to be stored. +Be aware that external processes often store the standard output and +standard error streams of the application. This will include logged sensitive information.

    The following example code logs user credentials (in this case, their password) -in plain text: +in plaintext:

    -Instead, the credentials should be encrypted, obfuscated, or omitted entirely: +Instead, you should encrypt or obfuscate the credentials, or omit them entirely:

    diff --git a/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql index 2b21b5da1bc..ecff134f0bd 100644 --- a/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql +++ b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql @@ -1,6 +1,6 @@ /** * @name Clear-text logging of sensitive information - * @description Logging sensitive information without encryption or hashing can + * @description Logging sensitive information in plaintext can * expose it to an attacker. * @kind path-problem * @problem.severity error From 0017461e2dd52e6767897a1a0726a7acdb2d7666 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 16 Jan 2023 15:35:58 +0100 Subject: [PATCH 34/41] Update swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com> --- swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql index ecff134f0bd..df5c128685f 100644 --- a/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql +++ b/swift/ql/src/queries/Security/CWE-312/CleartextLogging.ql @@ -1,5 +1,5 @@ /** - * @name Clear-text logging of sensitive information + * @name Cleartext logging of sensitive information * @description Logging sensitive information in plaintext can * expose it to an attacker. * @kind path-problem From 1d62751e159d1fa2dcd4a0de6f177a9f8a6fbecf Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Fri, 13 Jan 2023 13:40:24 +0100 Subject: [PATCH 35/41] test QL-for-QL on mac/win --- .github/workflows/ql-for-ql-tests.yml | 40 ++++++++++++++++++++++++++- ql/rust-toolchain.toml | 7 +++++ 2 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 ql/rust-toolchain.toml diff --git a/.github/workflows/ql-for-ql-tests.yml b/.github/workflows/ql-for-ql-tests.yml index 1663c5d2fd8..ff894bcc6a9 100644 --- a/.github/workflows/ql-for-ql-tests.yml +++ b/.github/workflows/ql-for-ql-tests.yml @@ -33,7 +33,7 @@ jobs: ~/.cargo/registry ~/.cargo/git ql/target - key: ${{ runner.os }}-${{ steps.os_version.outputs.version }}-qltest-cargo-${{ hashFiles('ql/**/Cargo.lock') }} + key: ${{ runner.os }}-${{ steps.os_version.outputs.version }}-qltest-cargo-${{ hashFiles('ql/rust-toolchain.toml', 'ql/**/Cargo.lock') }} - name: Build extractor run: | cd ql; @@ -49,3 +49,41 @@ jobs: find ql/ql/src "(" -name "*.ql" -or -name "*.qll" ")" -print0 | xargs -0 "${CODEQL}" query format --check-only env: CODEQL: ${{ steps.find-codeql.outputs.codeql-path }} + + other-os: + strategy: + matrix: + os: [macos-latest, windows-latest] + needs: [qltest] + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@v3 + - name: Install GNU tar + if: runner.os == 'macOS' + run: | + brew install gnu-tar + echo "/usr/local/opt/gnu-tar/libexec/gnubin" >> $GITHUB_PATH + - name: Find codeql + id: find-codeql + uses: github/codeql-action/init@77a8d2d10c0b403a8b4aadbd223dc489ecd22683 + with: + languages: javascript # does not matter + - uses: ./.github/actions/os-version + id: os_version + - uses: actions/cache@v3 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + ql/target + key: ${{ runner.os }}-${{ steps.os_version.outputs.version }}-qltest-cargo-${{ hashFiles('ql/rust-toolchain.toml', 'ql/**/Cargo.lock') }} + - name: Build extractor + run: | + cd ql; + codeqlpath=$(dirname ${{ steps.find-codeql.outputs.codeql-path }}); + env "PATH=$PATH:$codeqlpath" ./scripts/create-extractor-pack.sh + - name: Run a single QL tests + run: | + "${CODEQL}" test run --check-databases --search-path "${{ github.workspace }}/ql/extractor-pack" ql/ql/test/queries/style/DeadCode/DeadCode.qlref + env: + CODEQL: ${{ steps.find-codeql.outputs.codeql-path }} \ No newline at end of file diff --git a/ql/rust-toolchain.toml b/ql/rust-toolchain.toml new file mode 100644 index 00000000000..c0ca7a2593a --- /dev/null +++ b/ql/rust-toolchain.toml @@ -0,0 +1,7 @@ +# This file specifies the Rust version used to develop and test the QL +# extractor. It is set to the lowest version of Rust we want to support. + +[toolchain] +channel = "1.54" +profile = "minimal" +components = [ "rustfmt" ] From 0685732e3f8c4cf98a65176b13adbb8ef1bfbaa2 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Fri, 13 Jan 2023 13:51:22 +0100 Subject: [PATCH 36/41] delete ql/ specific format step now that we have an all-languages format check --- .github/workflows/ql-for-ql-tests.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.github/workflows/ql-for-ql-tests.yml b/.github/workflows/ql-for-ql-tests.yml index ff894bcc6a9..93bac75c3ad 100644 --- a/.github/workflows/ql-for-ql-tests.yml +++ b/.github/workflows/ql-for-ql-tests.yml @@ -44,11 +44,6 @@ jobs: "${CODEQL}" test run --check-databases --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --search-path "${{ github.workspace }}/ql/extractor-pack" --consistency-queries ql/ql/consistency-queries ql/ql/test env: CODEQL: ${{ steps.find-codeql.outputs.codeql-path }} - - name: Check QL formatting - run: | - find ql/ql/src "(" -name "*.ql" -or -name "*.qll" ")" -print0 | xargs -0 "${CODEQL}" query format --check-only - env: - CODEQL: ${{ steps.find-codeql.outputs.codeql-path }} other-os: strategy: From 1de65131fe33b4d3d1a11a785c1c0e6a6b6bbbff Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Fri, 13 Jan 2023 13:52:35 +0100 Subject: [PATCH 37/41] add compilation cache to QL-for-QL tests --- .github/workflows/ql-for-ql-tests.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ql-for-ql-tests.yml b/.github/workflows/ql-for-ql-tests.yml index 93bac75c3ad..d4a1baa9bf8 100644 --- a/.github/workflows/ql-for-ql-tests.yml +++ b/.github/workflows/ql-for-ql-tests.yml @@ -39,9 +39,14 @@ jobs: cd ql; codeqlpath=$(dirname ${{ steps.find-codeql.outputs.codeql-path }}); env "PATH=$PATH:$codeqlpath" ./scripts/create-extractor-pack.sh + - name: Cache compilation cache + id: query-cache + uses: ./.github/actions/cache-query-compilation + with: + key: ql-for-ql-tests - name: Run QL tests run: | - "${CODEQL}" test run --check-databases --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --search-path "${{ github.workspace }}/ql/extractor-pack" --consistency-queries ql/ql/consistency-queries ql/ql/test + "${CODEQL}" test run --check-databases --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --search-path "${{ github.workspace }}/ql/extractor-pack" --consistency-queries ql/ql/consistency-queries --compilation-cache "${{ steps.query-cache.outputs.cache-dir }}" ql/ql/test env: CODEQL: ${{ steps.find-codeql.outputs.codeql-path }} From 2c1ecb507d429207bd4a5f6e666e12726ae54112 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Fri, 13 Jan 2023 13:59:09 +0100 Subject: [PATCH 38/41] fix windows --- .github/workflows/ql-for-ql-tests.yml | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ql-for-ql-tests.yml b/.github/workflows/ql-for-ql-tests.yml index d4a1baa9bf8..d0196bcc961 100644 --- a/.github/workflows/ql-for-ql-tests.yml +++ b/.github/workflows/ql-for-ql-tests.yml @@ -78,12 +78,28 @@ jobs: ql/target key: ${{ runner.os }}-${{ steps.os_version.outputs.version }}-qltest-cargo-${{ hashFiles('ql/rust-toolchain.toml', 'ql/**/Cargo.lock') }} - name: Build extractor + if: runner.os != 'Windows' run: | cd ql; codeqlpath=$(dirname ${{ steps.find-codeql.outputs.codeql-path }}); env "PATH=$PATH:$codeqlpath" ./scripts/create-extractor-pack.sh - - name: Run a single QL tests + - name: Build extractor (Windows) + if: runner.os == 'Windows' + shell: pwsh + run: | + cd ql; + $Env:PATH += ";$(dirname ${{ steps.find-codeql.outputs.codeql-path }})" + pwsh ./scripts/create-extractor-pack.ps1 + - name: Run a single QL tests - Unix + if: runner.os != 'Windows' run: | "${CODEQL}" test run --check-databases --search-path "${{ github.workspace }}/ql/extractor-pack" ql/ql/test/queries/style/DeadCode/DeadCode.qlref env: - CODEQL: ${{ steps.find-codeql.outputs.codeql-path }} \ No newline at end of file + CODEQL: ${{ steps.find-codeql.outputs.codeql-path }} + - name: Run a single QL tests - Windows + if: runner.os == 'Windows' + shell: pwsh + run: | + $Env:PATH += ";$(dirname ${{ steps.find-codeql.outputs.codeql-path }})" + codeql test run --check-databases --search-path "${{ github.workspace }}/ql/extractor-pack" ql/ql/test/queries/style/DeadCode/DeadCode.qlref + \ No newline at end of file From 587adea80901a9b6f62e8b94b0e56cd44c0491f9 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Sun, 15 Jan 2023 15:15:23 +0100 Subject: [PATCH 39/41] QL: add --working-dir to qltest.cmd to fix qltest --- ql/tools/qltest.cmd | 1 + 1 file changed, 1 insertion(+) diff --git a/ql/tools/qltest.cmd b/ql/tools/qltest.cmd index 2573d4f929e..15b42c8af35 100644 --- a/ql/tools/qltest.cmd +++ b/ql/tools/qltest.cmd @@ -8,6 +8,7 @@ type NUL && "%CODEQL_DIST%\codeql.exe" database index-files ^ --include-extension=.yml ^ --size-limit=5m ^ --language=ql ^ + --working-dir=. ^ "%CODEQL_EXTRACTOR_QL_WIP_DATABASE%" exit /b %ERRORLEVEL% From 9e153cfb0d710ae83ff9fa03389e794cc91775db Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 16 Jan 2023 14:36:57 +0100 Subject: [PATCH 40/41] change the Ruby-build test such that Windows fails --- .github/workflows/ruby-build.yml | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ruby-build.yml b/.github/workflows/ruby-build.yml index e371ffcae27..bdb09bc9c7e 100644 --- a/.github/workflows/ruby-build.yml +++ b/.github/workflows/ruby-build.yml @@ -205,11 +205,6 @@ jobs: - name: Fetch CodeQL uses: ./.github/actions/fetch-codeql - - uses: actions/checkout@v3 - with: - repository: Shopify/example-ruby-app - ref: 67a0decc5eb550f3a9228eda53925c3afd40dfe9 - - name: Download Ruby bundle uses: actions/download-artifact@v3 with: @@ -218,26 +213,15 @@ jobs: - name: Unzip Ruby bundle shell: bash run: unzip -q -d "${{ runner.temp }}/ruby-bundle" "${{ runner.temp }}/codeql-ruby-bundle.zip" - - name: Prepare test files - shell: bash - run: | - echo "import codeql.ruby.AST select count(File f)" > "test.ql" - echo "| 4 |" > "test.expected" - echo 'name: sample-tests - version: 0.0.0 - dependencies: - codeql/ruby-all: "*" - extractor: ruby - tests: . - ' > qlpack.yml + - name: Run QL test shell: bash run: | - codeql test run --search-path "${{ runner.temp }}/ruby-bundle" --additional-packs "${{ runner.temp }}/ruby-bundle" . + codeql test run --search-path "${{ runner.temp }}/ruby-bundle" --additional-packs "${{ runner.temp }}/ruby-bundle" ruby/ql/test/library-tests/ast/constants/ - name: Create database shell: bash run: | - codeql database create --search-path "${{ runner.temp }}/ruby-bundle" --language ruby --source-root . ../database + codeql database create --search-path "${{ runner.temp }}/ruby-bundle" --language ruby --source-root ruby/ql/test/library-tests/ast/constants/ ../database - name: Analyze database shell: bash run: | From 713599963be58ba8be517c434af44d1aa350e8ff Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 16 Jan 2023 14:47:49 +0100 Subject: [PATCH 41/41] add --working-dir to Ruby qltest.cmd to fix Windows --- ruby/tools/qltest.cmd | 1 + 1 file changed, 1 insertion(+) diff --git a/ruby/tools/qltest.cmd b/ruby/tools/qltest.cmd index e43fe47e3fa..00a223cae5c 100644 --- a/ruby/tools/qltest.cmd +++ b/ruby/tools/qltest.cmd @@ -8,6 +8,7 @@ type NUL && "%CODEQL_DIST%\codeql.exe" database index-files ^ --include=**/Gemfile ^ --size-limit=5m ^ --language=ruby ^ + --working-dir=. ^ "%CODEQL_EXTRACTOR_RUBY_WIP_DATABASE%" exit /b %ERRORLEVEL%