From 92d3da5e56d8bc2d3668dab4f4e7e430b730c4cd Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Fri, 17 Dec 2021 17:07:58 +0000
Subject: [PATCH] Declassify fmt.Fprintf as a log sink

In future we could try harder to find out whether you're Fprintf'ing to stdout, a file named xyz.log etc, but for now this causes Fprintf'ing to an HTTP writer to be mistaken for log-injection rather than just XSS.
---
 ql/lib/semmle/go/frameworks/stdlib/Fmt.qll         | 14 +++-----------
 .../query-tests/Security/CWE-117/LogInjection.go   | 12 ++++++------
 2 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll b/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll
index e634a82a863..171c4d0ce76 100644
--- a/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll
+++ b/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll
@@ -29,19 +29,11 @@ module Fmt {
     Printer() { this.hasQualifiedName("fmt", ["Print", "Printf", "Println"]) }
   }
 
-  /** A call to `Print`, `Fprint`, or similar. */
+  /** A call to `Print` or similar. */
   private class PrintCall extends LoggerCall::Range, DataFlow::CallNode {
-    int firstPrintedArg;
+    PrintCall() { this.getTarget() instanceof Printer }
 
-    PrintCall() {
-      this.getTarget() instanceof Printer and firstPrintedArg = 0
-      or
-      this.getTarget() instanceof Fprinter and firstPrintedArg = 1
-    }
-
-    override DataFlow::Node getAMessageComponent() {
-      result = this.getArgument(any(int i | i >= firstPrintedArg))
-    }
+    override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
   }
 
   /** The `Fprint` function or one of its variants. */
diff --git a/ql/test/query-tests/Security/CWE-117/LogInjection.go b/ql/test/query-tests/Security/CWE-117/LogInjection.go
index 7beb055ad9a..f386d2fb910 100644
--- a/ql/test/query-tests/Security/CWE-117/LogInjection.go
+++ b/ql/test/query-tests/Security/CWE-117/LogInjection.go
@@ -32,12 +32,12 @@ func handler(req *http.Request, ctx *goproxy.ProxyCtx) {
 	testFlag := req.URL.Query()["testFlag"][0]
 
 	{
-		fmt.Print(username)         // $ hasTaintFlow="username"
-		fmt.Printf(username)        // $ hasTaintFlow="username"
-		fmt.Println(username)       // $ hasTaintFlow="username"
-		fmt.Fprint(nil, username)   // $ hasTaintFlow="username"
-		fmt.Fprintf(nil, username)  // $ hasTaintFlow="username"
-		fmt.Fprintln(nil, username) // $ hasTaintFlow="username"
+		fmt.Print(username)       // $ hasTaintFlow="username"
+		fmt.Printf(username)      // $ hasTaintFlow="username"
+		fmt.Println(username)     // $ hasTaintFlow="username"
+		fmt.Fprint(nil, username) // Fprint functions are only loggers if they target stdout/stderr
+		fmt.Fprintf(nil, username)
+		fmt.Fprintln(nil, username)
 	}
 	// log
 	{