diff --git a/src/main/scala/io/joern/scanners/ghidra/MainArgsToStrcpy.scala b/src/main/scala/io/joern/scanners/ghidra/UserInputIntoDangerousFunctions.scala similarity index 53% rename from src/main/scala/io/joern/scanners/ghidra/MainArgsToStrcpy.scala rename to src/main/scala/io/joern/scanners/ghidra/UserInputIntoDangerousFunctions.scala index 7c60727..13e67bd 100644 --- a/src/main/scala/io/joern/scanners/ghidra/MainArgsToStrcpy.scala +++ b/src/main/scala/io/joern/scanners/ghidra/UserInputIntoDangerousFunctions.scala @@ -7,7 +7,7 @@ import io.shiftleft.semanticcpg.language._ import io.shiftleft.dataflowengineoss.language._ import io.shiftleft.dataflowengineoss.queryengine.EngineContext -object MainArgsToStrcpy extends QueryBundle { +object UserInputIntoDangerousFunctions extends QueryBundle { implicit val resolver: ICallResolver = NoResolve @@ -24,9 +24,30 @@ object MainArgsToStrcpy extends QueryBundle { score = 4, withStrRep({ cpg => def source = cpg.method.fullName("main").parameter - def sink = cpg.call.methodFullName("strcpy").argument + def sink = cpg.method.fullName("strcpy").parameter.index(2) sink.reachableBy(source).l }), tags = List(QueryTags.badfn) ) + + @q + def getenvToStrcpy()(implicit context: EngineContext): Query = + Query.make( + name = "getenv-to-strcpy", + author = Crew.claudiu, + title = "`getenv` fn arguments used in strcpy source buffer", + description = + """ + |User-input ends up in source buffer argument of strcpy, which might overflow the destination buffer. + |""".stripMargin, + score = 4, + withStrRep({ cpg => + def source = + cpg.call.methodFullName("getenv").cfgNext.isCall.argument(2) + def sink = cpg.method.fullName("strcpy").parameter.index(2) + sink.reachableBy(source).l + }), + tags = List(QueryTags.badfn) + ) + } diff --git a/src/test/resources/testbinaries/buf2.c b/src/test/resources/testbinaries/buf2.c new file mode 100644 index 0000000..c44fcb4 --- /dev/null +++ b/src/test/resources/testbinaries/buf2.c @@ -0,0 +1,18 @@ +#include +#include +#include + +// gcc -fno-stack-protector -z execstack -no-pie -o buf2 buf2.c +int main(int argc, char *argv[]) { + const char* inEnv = getenv("BUF2IN"); + if (inEnv == NULL) { + printf("BUF2IN environment variable not set."); + return -1; + } + + char c[6]; + strcpy(c, inEnv); + printf("First argument is: %s\n", c); + return 0; +} + diff --git a/src/test/resources/testbinaries/buf2.exe b/src/test/resources/testbinaries/buf2.exe new file mode 100755 index 0000000..d273174 Binary files /dev/null and b/src/test/resources/testbinaries/buf2.exe differ diff --git a/src/test/resources/testbinaries/buf2_neg.c b/src/test/resources/testbinaries/buf2_neg.c new file mode 100644 index 0000000..e8cde7e --- /dev/null +++ b/src/test/resources/testbinaries/buf2_neg.c @@ -0,0 +1,18 @@ +#include +#include +#include + +// gcc -fno-stack-protector -z execstack -no-pie -o buf2_neg buf2_neg.c +int main(int argc, char *argv[]) { + const char* inEnv = getenv("BUF2IN"); + if (inEnv == NULL) { + printf("BUF2IN environment variable not set."); + return -1; + } + + char c[6]; + strcpy(c, "NOTHING"); + printf("First argument is: %s\n", c); + return 0; +} + diff --git a/src/test/resources/testbinaries/buf2_neg.exe b/src/test/resources/testbinaries/buf2_neg.exe new file mode 100755 index 0000000..444cc07 Binary files /dev/null and b/src/test/resources/testbinaries/buf2_neg.exe differ diff --git a/src/test/scala/io/joern/scanners/ghidra/MainArgsToStrcpyTests.scala b/src/test/scala/io/joern/scanners/ghidra/MainArgsToStrcpyTests.scala deleted file mode 100644 index 35cc4b5..0000000 --- a/src/test/scala/io/joern/scanners/ghidra/MainArgsToStrcpyTests.scala +++ /dev/null @@ -1,14 +0,0 @@ -package io.joern.scanners.ghidra - -import io.joern.suites.GhidraQueryTestSuite - -class MainArgsToStrcpyTests extends GhidraQueryTestSuite { - override def queryBundle = MainArgsToStrcpy - - "find main function with data flow between argument and strcpy" in { - buildCpgForBin("buf1.exe") - val query = queryBundle.mainArgsToStrcpy() - val results = findMatchingMethodParam(query) - results shouldBe Set("main") - } -} diff --git a/src/test/scala/io/joern/scanners/ghidra/UserInputIntoDangerousFunctionsTests.scala b/src/test/scala/io/joern/scanners/ghidra/UserInputIntoDangerousFunctionsTests.scala new file mode 100644 index 0000000..2adbc4a --- /dev/null +++ b/src/test/scala/io/joern/scanners/ghidra/UserInputIntoDangerousFunctionsTests.scala @@ -0,0 +1,46 @@ +package io.joern.scanners.ghidra + +import io.joern.suites.GhidraQueryTestSuite + +class UserInputIntoDangerousFunctionsTests extends GhidraQueryTestSuite { + override def queryBundle = UserInputIntoDangerousFunctions + + "mainArgsToStrcpy query" when { + def query = queryBundle.mainArgsToStrcpy() + "executed on CPG for binary with dataflow between `main` fn args and `strcpy` source argument" should { + "find the `main` function among the tracking points returned" in { + buildCpgForBin("buf1.exe") + val results = methodNamesForMatchedPoints(query) + results shouldBe Set("main") + } + } + } + + "getenvToStrcpy query" when { + def query = queryBundle.getenvToStrcpy() + + "executed on CPG for binary call to `strcpy`, but no call to `getenv`" should { + "return an empty set of matched method names" in { + buildCpgForBin("buf1.exe") + val results = methodNamesForMatchedPoints(query) + results shouldBe Set() + } + } + + "executed on CPG for binary call to `strcpy`, and call to `getenv`, but no dataflow between them" should { + "return an empty set of matched method names" in { + buildCpgForBin("buf2_neg.exe") + val results = methodNamesForMatchedPoints(query) + results shouldBe Set() + } + } + + "executed on CPG for binary with dataflow between `getenv` return value and `strcpy` source argument" should { + "find main function with data flow between getenv and strcpy" in { + buildCpgForBin("buf2.exe") + val results = methodNamesForMatchedPoints(query) + results shouldBe Set("main") + } + } + } +} diff --git a/src/test/scala/io/joern/suites/GhidraQueryTestSuite.scala b/src/test/scala/io/joern/suites/GhidraQueryTestSuite.scala index a1617bc..a415acf 100644 --- a/src/test/scala/io/joern/suites/GhidraQueryTestSuite.scala +++ b/src/test/scala/io/joern/suites/GhidraQueryTestSuite.scala @@ -8,6 +8,7 @@ import io.shiftleft.codepropertygraph.generated.nodes import io.shiftleft.console.{Query, QueryBundle} import io.shiftleft.utils.ProjectRoot import io.shiftleft.semanticcpg.language._ +import overflowdb.traversal.iterableToTraversal class GhidraQueryTestSuite extends DataFlowBinToCpgSuite { val argumentProvider = new QDBArgumentProvider(3) @@ -22,21 +23,22 @@ class GhidraQueryTestSuite extends DataFlowBinToCpgSuite { def allQueries = QueryUtil.allQueries(queryBundle, argumentProvider) - def findMatchingMethodParam(query: Query): Set[String] = { + def findMatchingCalls(query: Query): Set[String] = { query(cpg) .flatMap(_.evidence) - .collect { case methodParam: nodes.MethodParameterIn => methodParam } + .collect { case call: nodes.Call => call } .method .name .toSetImmutable } - def findMatchingCalls(query: Query): Set[String] = { + def methodNamesForMatchedPoints(query: Query): Set[String] = { + nodes.MethodParameterIn query(cpg) .flatMap(_.evidence) - .collect { case call: nodes.Call => call } - .method - .name + .collect { + case cfgNode: nodes.CfgNode => cfgNode.method.name + } .toSetImmutable } }