diff --git a/README.org b/README.org index 3828277..dd9c0e4 100644 --- a/README.org +++ b/README.org @@ -216,6 +216,78 @@ - /Users/hohn/local/codeql-cli-end-to-end/sarif-cli/non-sarif-metadata/README.org * TODO CodeQL for Query Writers - - https://github.com/hohn/codeql-workshop-sql-injection-java - + https://github.com/hohn/codeql-workshop-sql-injection-java/blob/master/session/README.org - +** Identify the problem + =./add-user= is reading from =STDIN=, and writing to a database; looking at the code in + [[./add-user.c]] leads to + : count = read(STDIN_FILENO, buf, BUFSIZE - 1); + for the read and + : rc = sqlite3_exec(db, query, NULL, 0, &zErrMsg); + for the write. + + This problem is thus a dataflow problem; in codeql terminology we have + - a /source/ at the =read(STDIN_FILENO, buf, BUFSIZE - 1);= + - a /sink/ at the =sqlite3_exec(db, query, NULL, 0, &zErrMsg);= + + We write codeql to identify these two, and then connect them via + - a /dataflow configuration/ -- for this problem, the more general /taintflow + configuration/. + +** Develop the query bottom-up + 1. Identify the /source/ part of the + : Console.ReadLine()?.Trim() ?? string.Empty; + expression, the =Console.ReadLine()= call. + Start from a =from..where..select= then convert to a predicate or class. + The =from..where..select= is found in [[./SqlInjection-source.ql]] + + 2. Identify the /sink/ part of the + : var command = new SqliteCommand(query, connection)) + expression, the =query= argument. + Again start from =from..where..select=, + then convert to a predicate or class. + There is a subtlety here; + [[https://codeql.github.com/docs/codeql-language-guides/codeql-library-for-csharp/][the docs]] mention 'The Expr class represents all C# expressions in the + program. An expression is something producing a value such as a+b or new + List().' Use the 'view AST' option from the results of step 1 to see + what is needed here. It's not obvious. + The =from..where..select= is found in [[./SqlInjection-sink.ql]] + + 3. Fill in the /taintflow configuration/ boilerplate. The [[https://codeql.github.com/docs/codeql-language-guides/analyzing-data-flow-in-csharp/#using-global-taint-tracking][documentation]] + explains in detail. For this example, use + #+BEGIN_SRC java + module MyFlowConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + ... + } + + predicate isSink(DataFlow::Node sink) { + ... + } + } + + module MyFlow = TaintTracking::Global; + + from DataFlow::Node source, DataFlow::Node sink + where MyFlow::flow(source, sink) + select source, "Dataflow to $@.", sink, sink.toString() + #+END_SRC + + Note the different CodeQL classes used here and their connections: =Node=, + =ExprNode=, =ParameterNode= are part of the DFG (data flow graph), =Expr= and + =Parameter= are part of the AST (abstract syntax tree). Here, this means + using + : source.asExpr() = call + for the source and + : sink.asExpr() = queryArg + for the sink. + + 4. Also, note that we want the flow path. So the query changes from + : * @kind problem + to + : * @kind path-problem + There are other changes, see [[./SqlInjection-flow-with-path.ql]] + + 5. Try this with dataflow instead of taintflow, and notice that there are no + results. + + + diff --git a/SqlInjection-flow-no-path.ql b/SqlInjection-flow-no-path.ql new file mode 100644 index 0000000..abec4ba --- /dev/null +++ b/SqlInjection-flow-no-path.ql @@ -0,0 +1,33 @@ +/** + * @name SQLI Vulnerability + * @description Using untrusted strings in a sql query allows sql injection attacks. + * @kind problem + * @id workshop/sqlivulnerable + * @problem.severity warning + */ + +import csharp + +module MyFlowConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + exists(MethodCall call | + call.getTarget().getDeclaringType().hasFullyQualifiedName("System", "Console") and + call.getTarget().getName() = "ReadLine" and + source.asExpr() = call + ) + } + + predicate isSink(DataFlow::Node sink) { + exists(ObjectCreation oc, Expr queryArg | + oc.getObjectType().getName() = "SqliteCommand" and + oc.getArgument(0) = queryArg and + sink.asExpr() = queryArg + ) + } +} + +module MyFlow = TaintTracking::Global; + +from DataFlow::Node source, DataFlow::Node sink +where MyFlow::flow(source, sink) +select source, "Taintflow to $@.", sink, sink.toString() diff --git a/SqlInjection-flow-with-path.ql b/SqlInjection-flow-with-path.ql new file mode 100644 index 0000000..fee931b --- /dev/null +++ b/SqlInjection-flow-with-path.ql @@ -0,0 +1,34 @@ +/** + * @name SQLI Vulnerability + * @description Using untrusted strings in a sql query allows sql injection attacks. + * @kind path-problem + * @id workshop/sqlivulnerable + * @problem.severity warning + */ + +import csharp + +module MyFlowConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + exists(MethodCall call | + call.getTarget().getDeclaringType().hasFullyQualifiedName("System", "Console") and + call.getTarget().getName() = "ReadLine" and + source.asExpr() = call + ) + } + + predicate isSink(DataFlow::Node sink) { + exists(ObjectCreation oc, Expr queryArg | + oc.getObjectType().getName() = "SqliteCommand" and + oc.getArgument(0) = queryArg and + sink.asExpr() = queryArg + ) + } +} + +module MyFlow = TaintTracking::Global; +import MyFlow::PathGraph + +from MyFlow::PathNode source, MyFlow::PathNode sink +where MyFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "Taintflow found" diff --git a/SqlInjection-sink.ql b/SqlInjection-sink.ql new file mode 100644 index 0000000..a82e562 --- /dev/null +++ b/SqlInjection-sink.ql @@ -0,0 +1,21 @@ +/** + * @name SQLI Vulnerability + * @description Using untrusted strings in a sql query allows sql injection attacks. + * @kind problem + * @id workshop/sqlivulnerable + * @problem.severity warning + */ + +import csharp + +/* + * 2. Identify the /sink/ part of the + * : var command = new SqliteCommand(query, connection)) + * expression, the =query= argument. + */ + +from ObjectCreation oc, Expr queryArg +where + oc.getObjectType().getName() = "SqliteCommand" and + oc.getArgument(0) = queryArg +select queryArg, "Sink identified: " + queryArg.toString() diff --git a/SqlInjection-source.ql b/SqlInjection-source.ql new file mode 100644 index 0000000..a85818e --- /dev/null +++ b/SqlInjection-source.ql @@ -0,0 +1,22 @@ +/** + * @name SQLI Vulnerability + * @description Using untrusted strings in a sql query allows sql injection attacks. + * @kind problem + * @id workshop/sqlivulnerable + * @problem.severity warning + */ + +import csharp + +/* + * 1. Identify the /source/ part of the + * : Console.ReadLine()?.Trim() ?? string.Empty; + * : read(STDIN_FILENO, buf, BUFSIZE - 1); + * expression, the =Console.ReadLine()= call. + */ + +from MethodCall call +where + call.getTarget().getDeclaringType().hasFullyQualifiedName("System", "Console") and + call.getTarget().getName() = "ReadLine" +select call, "Source identified: " + call.toString() diff --git a/SqlInjection.ql b/SqlInjection.ql new file mode 100644 index 0000000..af5baa1 --- /dev/null +++ b/SqlInjection.ql @@ -0,0 +1,34 @@ +/** + * @name SQLI Vulnerability + * @description Using untrusted strings in a sql query allows sql injection attacks. + * @kind path-problem + * @id workshop/sqlivulnerable + * @problem.severity warning + */ + +import csharp + +module MyFlowConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + exists(MethodCall call | + call.getTarget().getDeclaringType().hasFullyQualifiedName("System", "Console") and + call.getTarget().getName() = "ReadLine" and + source.asExpr() = call + ) + } + + predicate isSink(DataFlow::Node sink) { + exists(ObjectCreation oc, Expr queryArg | + oc.getObjectType().getName() = "SqliteCommand" and + oc.getArgument(0) = queryArg and + sink.asExpr() = queryArg + ) + } +} + +module Flow = DataFlow::Global; +import Flow::PathGraph + +from Flow::PathNode source, Flow::PathNode sink +where Flow::flowPath(source, sink) +select sink, source, sink, "Dataflow found"