From d0e8d6efda101d1bbae846e4927e04d54f0f0045 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Mon, 4 May 2020 09:47:34 +0100
Subject: [PATCH 01/10] Fix post-update nodes for function arguments.

---
 ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll             | 2 +-
 .../FunctionOutput_getExitNode.expected                         | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
index 3645073cf..6ccd6338b 100644
--- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
+++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
@@ -408,7 +408,7 @@ class PostUpdateNode extends Node {
       )
       or
       preupd instanceof ArgumentNode and
-      mutableType(preupd.getType())
+      mutableType(preupd.getType().getUnderlyingType())
     ) and
     (
       preupd = this.(SsaNode).getAUse()
diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected
index f84d4616a..cf0fe4752 100644
--- a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected
+++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected
@@ -1,3 +1,4 @@
+| parameter 0 | tst.go:10:2:10:29 | call to ReadFrom | tst.go:8:12:8:17 | definition of reader |
 | receiver | tst.go:10:2:10:29 | call to ReadFrom | tst.go:9:2:9:12 | definition of bytesBuffer |
 | result | main.go:51:2:51:14 | call to op | main.go:51:2:51:14 | call to op |
 | result | main.go:53:2:53:22 | call to op2 | main.go:53:2:53:22 | call to op2 |

From 5b0c48e332e6129b3d1258bd74d2303df66b744c Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Mon, 4 May 2020 09:48:41 +0100
Subject: [PATCH 02/10] Add taint models for `fmt.Fprintf` and
 `io.WriteString`.

---
 ql/src/semmle/go/frameworks/Stdlib.qll | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll
index ec496aca0..82c7d51f8 100644
--- a/ql/src/semmle/go/frameworks/Stdlib.qll
+++ b/ql/src/semmle/go/frameworks/Stdlib.qll
@@ -93,6 +93,15 @@ module Fmt {
 
     override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
   }
+
+  private class FprintfModel extends TaintTracking::FunctionModel {
+    FprintfModel() { this.hasQualifiedName("fmt", "Fprintf") }
+
+    override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+      input.isParameter(any(int i | i > 0)) and
+      output.isParameter(0)
+    }
+  }
 }
 
 /** Provides models of commonly used functions in the `io` package. */
@@ -106,6 +115,15 @@ module Io {
       inp.isReceiver() and outp.isParameter(0)
     }
   }
+
+  private class WriteStringModel extends TaintTracking::FunctionModel {
+    WriteStringModel() { this.hasQualifiedName("io", "WriteString") }
+
+    override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+      input.isParameter(1) and
+      output.isParameter(0)
+    }
+  }
 }
 
 /** Provides models of commonly used functions in the `io/ioutil` package. */

From 5e8e51993e1469a8db23e4b48ed87a761a3d518c Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Mon, 4 May 2020 09:48:55 +0100
Subject: [PATCH 03/10] Simplify `SmtpData`.

---
 ql/src/semmle/go/frameworks/Email.qll | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/ql/src/semmle/go/frameworks/Email.qll b/ql/src/semmle/go/frameworks/Email.qll
index 90cbb523e..8bcb90c7d 100644
--- a/ql/src/semmle/go/frameworks/Email.qll
+++ b/ql/src/semmle/go/frameworks/Email.qll
@@ -30,17 +30,9 @@ module EmailData {
   private class SmtpData extends Range {
     SmtpData() {
       // func (c *Client) Data() (io.WriteCloser, error)
-      exists(Method data, DataFlow::CallNode write, DataFlow::Node writer, int i |
+      exists(Method data |
         data.hasQualifiedName("net/smtp", "Client", "Data") and
-        writer = data.getACall().getResult(0) and
-        (
-          write.getTarget().hasQualifiedName("fmt", "Fprintf")
-          or
-          write.getTarget().hasQualifiedName("io", "WriteString")
-        ) and
-        writer.getASuccessor*() = write.getArgument(0) and
-        i > 0 and
-        write.getArgument(i) = this
+        this.(DataFlow::SsaNode).getInit() = data.getACall().getResult(0)
       )
       or
       // func SendMail(addr string, a Auth, from string, to []string, msg []byte) error

From e632c75de33e1f196b0428fe30e0c68fbf3ba749 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Mon, 4 May 2020 16:11:23 +0100
Subject: [PATCH 04/10] Add support for taint models involving "backwards"
 taint propagation from results to arguments.

---
 .../go/dataflow/FunctionInputsAndOutputs.qll  | 56 ++++++++++++++++++-
 .../FunctionInput_getEntryNode.expected       |  1 +
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/ql/src/semmle/go/dataflow/FunctionInputsAndOutputs.qll b/ql/src/semmle/go/dataflow/FunctionInputsAndOutputs.qll
index 1cf73dad3..922aee529 100644
--- a/ql/src/semmle/go/dataflow/FunctionInputsAndOutputs.qll
+++ b/ql/src/semmle/go/dataflow/FunctionInputsAndOutputs.qll
@@ -12,7 +12,14 @@ private import semmle.go.dataflow.internal.DataFlowPrivate
  */
 private newtype TFunctionInput =
   TInParameter(int i) { exists(SignatureType s | exists(s.getParameterType(i))) } or
-  TInReceiver()
+  TInReceiver() or
+  TInResult(int index) {
+    // the one and only result
+    index = -1
+    or
+    // one among several results
+    exists(SignatureType s | exists(s.getResultType(index)))
+  }
 
 /**
  * An abstract representation of an input to a function, which is either a parameter
@@ -25,6 +32,12 @@ class FunctionInput extends TFunctionInput {
   /** Holds if this represents the receiver of a function. */
   predicate isReceiver() { none() }
 
+  /** Holds if this represents the result of a function. */
+  predicate isResult() { none() }
+
+  /** Holds if this represents the `i`th result of a function. */
+  predicate isResult(int i) { none() }
+
   /** Gets the data-flow node corresponding to this input for the call `c`. */
   final DataFlow::Node getNode(DataFlow::CallNode c) { result = getEntryNode(c) }
 
@@ -70,6 +83,47 @@ private class ReceiverInput extends FunctionInput, TInReceiver {
   override string toString() { result = "receiver" }
 }
 
+/**
+ * A result position of a function, viewed as an input.
+ *
+ * Results are usually outputs rather than inputs, but for taint tracking it can be useful to
+ * think of taint propagating backwards from a result of a function to its arguments. For instance,
+ * the function `bufio.NewWriter` returns a writer `bw` that buffers write operations to an
+ * underlying writer `w`. If tainted data is written to `bw`, then it makes sense to propagate
+ * that taint back to the underlying writer `w`, which can be modeled by saying that
+ * `bufio.NewWriter` propagates taint from its result to its first argument.
+ */
+private class ResultInput extends FunctionInput, TInResult {
+  int index;
+
+  ResultInput() { this = TInResult(index) }
+
+  override predicate isResult() { index = -1 }
+
+  override predicate isResult(int i) { i = index and i >= 0 }
+
+  override DataFlow::Node getEntryNode(DataFlow::CallNode c) {
+    exists(DataFlow::PostUpdateNode pun, DataFlow::Node init |
+      pun = result and
+      init = pun.(DataFlow::SsaNode).getInit()
+    |
+      index = -1 and
+      init = c.getResult()
+      or
+      index >= 0 and
+      init = c.getResult(index)
+    )
+  }
+
+  override DataFlow::Node getExitNode(FuncDef f) { none() }
+
+  override string toString() {
+    index = -1 and result = "result"
+    or
+    index >= 0 and result = "result " + index
+  }
+}
+
 /**
  * An abstract representation of an output of a function, which is one of its results.
  */
diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected
index 2989f4ef3..649f5fd62 100644
--- a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected
+++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected
@@ -13,3 +13,4 @@
 | parameter 2 | main.go:57:2:57:27 | call to Printf | main.go:57:26:57:26 | y |
 | receiver | main.go:53:14:53:21 | call to bump | main.go:53:14:53:14 | c |
 | receiver | tst.go:10:2:10:29 | call to ReadFrom | tst.go:10:2:10:12 | bytesBuffer |
+| result | tst.go:9:17:9:33 | call to new | tst.go:9:2:9:12 | definition of bytesBuffer |

From be94f2b9e6e6b5dc593e75f862f606855ae94104 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Mon, 4 May 2020 16:12:23 +0100
Subject: [PATCH 05/10] Improve and extend various standard-library function
 models.

---
 ql/src/semmle/go/frameworks/Stdlib.qll | 78 ++++++++++++++++++--------
 1 file changed, 55 insertions(+), 23 deletions(-)

diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll
index 82c7d51f8..86c8a3909 100644
--- a/ql/src/semmle/go/frameworks/Stdlib.qll
+++ b/ql/src/semmle/go/frameworks/Stdlib.qll
@@ -67,39 +67,55 @@ module PathFilePath {
   }
 }
 
+/** Provides models of commonly used functions in the `bytes` package. */
+private module Bytes {
+  private class BufferBytes extends TaintTracking::FunctionModel, Method {
+    BufferBytes() { this.hasQualifiedName("bytes", "Buffer", ["Bytes", "String"]) }
+
+    override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+      input.isReceiver() and output.isResult()
+    }
+  }
+}
+
 /** Provides models of commonly used functions in the `fmt` package. */
 module Fmt {
   /** The `Sprint` function or one of its variants. */
   class Sprinter extends TaintTracking::FunctionModel {
-    Sprinter() {
-      exists(string sprint | sprint.matches("Sprint%") | hasQualifiedName("fmt", sprint))
-    }
+    Sprinter() { this.hasQualifiedName("fmt", ["Sprint", "Sprintf", "Sprintln"]) }
 
     override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {
       inp.isParameter(_) and outp.isResult()
     }
   }
 
+  /** The `Print` function or one of its variants. */
+  private class Printer extends Function {
+    Printer() { this.hasQualifiedName("fmt", ["Print", "Printf", "Println"]) }
+  }
+
+  /** A call to `Print`, `Fprint`, or similar. */
   private class PrintCall extends LoggerCall::Range, DataFlow::CallNode {
-    PrintCall() {
-      exists(string fn |
-        fn = "Print%"
-        or
-        fn = "Fprint%"
-      |
-        this.getTarget().hasQualifiedName("fmt", fn)
-      )
-    }
+    PrintCall() { this.getTarget() instanceof Printer or this.getTarget() instanceof Fprinter }
 
     override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
   }
 
-  private class FprintfModel extends TaintTracking::FunctionModel {
-    FprintfModel() { this.hasQualifiedName("fmt", "Fprintf") }
+  /** The `Fprint` function or one of its variants. */
+  private class Fprinter extends TaintTracking::FunctionModel {
+    Fprinter() { this.hasQualifiedName("fmt", ["Fprint", "Fprintf", "Fprintln"]) }
 
     override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
-      input.isParameter(any(int i | i > 0)) and
-      output.isParameter(0)
+      input.isParameter(any(int i | i > 0)) and output.isParameter(0)
+    }
+  }
+
+  /** The `Sscan` function or one of its variants. */
+  private class Sscanner extends TaintTracking::FunctionModel {
+    Sscanner() { this.hasQualifiedName("fmt", ["Sscan", "Sscanf", "Sscanln"]) }
+
+    override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+      input.isParameter(0) and output.isParameter(any(int i | i > 0))
     }
   }
 }
@@ -107,21 +123,37 @@ module Fmt {
 /** Provides models of commonly used functions in the `io` package. */
 module Io {
   private class ReaderRead extends TaintTracking::FunctionModel, Method {
-    ReaderRead() {
-      exists(Method im | im.hasQualifiedName("io", "Reader", "Read") | this.implements(im))
-    }
+    ReaderRead() { this.implements("io", "Reader", "Read") }
 
     override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
       inp.isReceiver() and outp.isParameter(0)
     }
   }
 
-  private class WriteStringModel extends TaintTracking::FunctionModel {
-    WriteStringModel() { this.hasQualifiedName("io", "WriteString") }
+  private class WriterWrite extends TaintTracking::FunctionModel, Method {
+    WriterWrite() { this.implements("io", "Writer", "Write") }
+
+    override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+      input.isParameter(0) and output.isReceiver()
+    }
+  }
+
+  private class WriteString extends TaintTracking::FunctionModel {
+    WriteString() { this.hasQualifiedName("io", "WriteString") }
+
+    override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+      input.isParameter(1) and output.isParameter(0)
+    }
+  }
+}
+
+/** Provides models of commonly used functions in the `bufio` package. */
+module Bufio {
+  private class NewWriter extends TaintTracking::FunctionModel {
+    NewWriter() { this.hasQualifiedName("bufio", "NewWriter") }
 
     override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
-      input.isParameter(1) and
-      output.isParameter(0)
+      input.isResult() and output.isParameter(0)
     }
   }
 }

From 5a96b0e8acc898dd9c89d2c9f04034f942f6cade Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Mon, 4 May 2020 16:13:53 +0100
Subject: [PATCH 06/10] Add two function models for handling MIME APIs.

---
 ql/src/semmle/go/frameworks/Email.qll | 30 +++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/ql/src/semmle/go/frameworks/Email.qll b/ql/src/semmle/go/frameworks/Email.qll
index 8bcb90c7d..b867dfb98 100644
--- a/ql/src/semmle/go/frameworks/Email.qll
+++ b/ql/src/semmle/go/frameworks/Email.qll
@@ -84,3 +84,33 @@ module EmailData {
     }
   }
 }
+
+/**
+ * A taint model of the `Writer.CreatePart` method from `mime/multipart`.
+ *
+ * If tainted data is written to the multipart section created by this method, the underlying writer
+ * should be considered tainted as well.
+ */
+private class MultipartWriterCreatePartModel extends TaintTracking::FunctionModel, Method {
+  MultipartWriterCreatePartModel() {
+    this.hasQualifiedName("mime/multipart", "Writer", "CreatePart")
+  }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    input.isResult(0) and output.isReceiver()
+  }
+}
+
+/**
+ * A taint model of the `NewWriter` function from `mime/multipart`.
+ *
+ * If tainted data is written to the writer created by this function, the underlying writer
+ * should be considered tainted as well.
+ */
+private class MultipartNewWriterModel extends TaintTracking::FunctionModel {
+  MultipartNewWriterModel() { this.hasQualifiedName("mime/multipart", "NewWriter") }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    input.isResult() and output.isParameter(0)
+  }
+}

From 60a6c9686310e2a58623feba34a65011402562f1 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Mon, 4 May 2020 16:22:39 +0100
Subject: [PATCH 07/10] Simplify modeling of `NewContent`.

---
 ql/src/semmle/go/frameworks/Email.qll         | 25 +++++++---------
 .../CWE-640/EmailInjection.expected           | 30 +++++++++++--------
 .../go/frameworks/Email/MailData.expected     |  8 ++---
 3 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/ql/src/semmle/go/frameworks/Email.qll b/ql/src/semmle/go/frameworks/Email.qll
index b867dfb98..50eb34625 100644
--- a/ql/src/semmle/go/frameworks/Email.qll
+++ b/ql/src/semmle/go/frameworks/Email.qll
@@ -47,15 +47,15 @@ module EmailData {
   bindingset[result]
   private string sendgridMail() { result = "github.com/sendgrid/sendgrid-go/helpers/mail" }
 
-  /* Gets the value of the `i`th content parameter of the given `call` */
-  private DataFlow::Node getContent(DataFlow::CallNode call, int i) {
-    exists(DataFlow::CallNode cn, DataFlow::Node content |
+  private class NewContent extends TaintTracking::FunctionModel {
+    NewContent() {
       // func NewContent(contentType string, value string) *Content
-      cn.getTarget().hasQualifiedName(sendgridMail(), "NewContent") and
-      cn.getResult() = content and
-      content.getASuccessor*() = call.getArgument(i) and
-      result = cn.getArgument(1)
-    )
+      this.hasQualifiedName(sendgridMail(), "NewContent")
+    }
+
+    override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+      input.isParameter(1) and output.isResult()
+    }
   }
 
   /** A data-flow node that is written to an email using the sendgrid/sendgrid-go package. */
@@ -69,17 +69,14 @@ module EmailData {
       or
       // func NewV3MailInit(from *Email, subject string, to *Email, content ...*Content) *SGMailV3
       exists(Function newv3MailInit |
-        newv3MailInit.hasQualifiedName(sendgridMail(), "NewV3MailInit")
-      |
-        this = getContent(newv3MailInit.getACall(), any(int i | i >= 3))
-        or
-        this = newv3MailInit.getACall().getArgument(1)
+        newv3MailInit.hasQualifiedName(sendgridMail(), "NewV3MailInit") and
+        this = newv3MailInit.getACall().getArgument(any(int i | i = 1 or i >= 3))
       )
       or
       // func (s *SGMailV3) AddContent(c ...*Content) *SGMailV3
       exists(Method addContent |
         addContent.hasQualifiedName(sendgridMail(), "SGMailV3", "AddContent") and
-        this = getContent(addContent.getACall(), _)
+        this = addContent.getACall().getAnArgument()
       )
     }
   }
diff --git a/ql/test/experimental/CWE-640/EmailInjection.expected b/ql/test/experimental/CWE-640/EmailInjection.expected
index 7d7e2988d..37a3af5ae 100644
--- a/ql/test/experimental/CWE-640/EmailInjection.expected
+++ b/ql/test/experimental/CWE-640/EmailInjection.expected
@@ -1,37 +1,43 @@
 edges
 | email.go:24:10:24:17 | selection of Header : Header | email.go:27:56:27:67 | type conversion |
 | email.go:34:21:34:31 | call to Referer : string | email.go:36:57:36:78 | type conversion |
-| email.go:42:21:42:31 | call to Referer : string | email.go:46:25:46:38 | untrustedInput |
+| email.go:42:21:42:31 | call to Referer : string | email.go:45:3:45:7 | definition of write |
 | email.go:51:21:51:31 | call to Referer : string | email.go:57:46:57:59 | untrustedInput |
 | email.go:51:21:51:31 | call to Referer : string | email.go:58:52:58:65 | untrustedInput |
-| email.go:63:21:63:31 | call to Referer : string | email.go:65:47:65:60 | untrustedInput |
-| email.go:73:21:73:31 | call to Referer : string | email.go:79:47:79:60 | untrustedInput |
+| email.go:63:21:63:31 | call to Referer : string | email.go:68:16:68:22 | content |
+| email.go:73:21:73:31 | call to Referer : string | email.go:81:50:81:56 | content |
+| email.go:73:21:73:31 | call to Referer : string | email.go:81:59:81:65 | content |
+| email.go:73:21:73:31 | call to Referer : string | email.go:82:16:82:22 | content |
 | email.go:87:21:87:31 | call to Referer : string | email.go:94:37:94:50 | untrustedInput |
-| email.go:87:21:87:31 | call to Referer : string | email.go:96:48:96:61 | untrustedInput |
+| email.go:87:21:87:31 | call to Referer : string | email.go:98:16:98:23 | content2 |
 nodes
 | email.go:24:10:24:17 | selection of Header : Header | semmle.label | selection of Header : Header |
 | email.go:27:56:27:67 | type conversion | semmle.label | type conversion |
 | email.go:34:21:34:31 | call to Referer : string | semmle.label | call to Referer : string |
 | email.go:36:57:36:78 | type conversion | semmle.label | type conversion |
 | email.go:42:21:42:31 | call to Referer : string | semmle.label | call to Referer : string |
-| email.go:46:25:46:38 | untrustedInput | semmle.label | untrustedInput |
+| email.go:45:3:45:7 | definition of write | semmle.label | definition of write |
 | email.go:51:21:51:31 | call to Referer : string | semmle.label | call to Referer : string |
 | email.go:57:46:57:59 | untrustedInput | semmle.label | untrustedInput |
 | email.go:58:52:58:65 | untrustedInput | semmle.label | untrustedInput |
 | email.go:63:21:63:31 | call to Referer : string | semmle.label | call to Referer : string |
-| email.go:65:47:65:60 | untrustedInput | semmle.label | untrustedInput |
+| email.go:68:16:68:22 | content | semmle.label | content |
 | email.go:73:21:73:31 | call to Referer : string | semmle.label | call to Referer : string |
-| email.go:79:47:79:60 | untrustedInput | semmle.label | untrustedInput |
+| email.go:81:50:81:56 | content | semmle.label | content |
+| email.go:81:59:81:65 | content | semmle.label | content |
+| email.go:82:16:82:22 | content | semmle.label | content |
 | email.go:87:21:87:31 | call to Referer : string | semmle.label | call to Referer : string |
 | email.go:94:37:94:50 | untrustedInput | semmle.label | untrustedInput |
-| email.go:96:48:96:61 | untrustedInput | semmle.label | untrustedInput |
+| email.go:98:16:98:23 | content2 | semmle.label | content2 |
 #select
 | email.go:27:56:27:67 | type conversion | email.go:24:10:24:17 | selection of Header : Header | email.go:27:56:27:67 | type conversion | Email content may contain $@. | email.go:24:10:24:17 | selection of Header | untrusted input |
 | email.go:36:57:36:78 | type conversion | email.go:34:21:34:31 | call to Referer : string | email.go:36:57:36:78 | type conversion | Email content may contain $@. | email.go:34:21:34:31 | call to Referer | untrusted input |
-| email.go:46:25:46:38 | untrustedInput | email.go:42:21:42:31 | call to Referer : string | email.go:46:25:46:38 | untrustedInput | Email content may contain $@. | email.go:42:21:42:31 | call to Referer | untrusted input |
+| email.go:45:3:45:7 | definition of write | email.go:42:21:42:31 | call to Referer : string | email.go:45:3:45:7 | definition of write | Email content may contain $@. | email.go:42:21:42:31 | call to Referer | untrusted input |
 | email.go:57:46:57:59 | untrustedInput | email.go:51:21:51:31 | call to Referer : string | email.go:57:46:57:59 | untrustedInput | Email content may contain $@. | email.go:51:21:51:31 | call to Referer | untrusted input |
 | email.go:58:52:58:65 | untrustedInput | email.go:51:21:51:31 | call to Referer : string | email.go:58:52:58:65 | untrustedInput | Email content may contain $@. | email.go:51:21:51:31 | call to Referer | untrusted input |
-| email.go:65:47:65:60 | untrustedInput | email.go:63:21:63:31 | call to Referer : string | email.go:65:47:65:60 | untrustedInput | Email content may contain $@. | email.go:63:21:63:31 | call to Referer | untrusted input |
-| email.go:79:47:79:60 | untrustedInput | email.go:73:21:73:31 | call to Referer : string | email.go:79:47:79:60 | untrustedInput | Email content may contain $@. | email.go:73:21:73:31 | call to Referer | untrusted input |
+| email.go:68:16:68:22 | content | email.go:63:21:63:31 | call to Referer : string | email.go:68:16:68:22 | content | Email content may contain $@. | email.go:63:21:63:31 | call to Referer | untrusted input |
+| email.go:81:50:81:56 | content | email.go:73:21:73:31 | call to Referer : string | email.go:81:50:81:56 | content | Email content may contain $@. | email.go:73:21:73:31 | call to Referer | untrusted input |
+| email.go:81:59:81:65 | content | email.go:73:21:73:31 | call to Referer : string | email.go:81:59:81:65 | content | Email content may contain $@. | email.go:73:21:73:31 | call to Referer | untrusted input |
+| email.go:82:16:82:22 | content | email.go:73:21:73:31 | call to Referer : string | email.go:82:16:82:22 | content | Email content may contain $@. | email.go:73:21:73:31 | call to Referer | untrusted input |
 | email.go:94:37:94:50 | untrustedInput | email.go:87:21:87:31 | call to Referer : string | email.go:94:37:94:50 | untrustedInput | Email content may contain $@. | email.go:87:21:87:31 | call to Referer | untrusted input |
-| email.go:96:48:96:61 | untrustedInput | email.go:87:21:87:31 | call to Referer : string | email.go:96:48:96:61 | untrustedInput | Email content may contain $@. | email.go:87:21:87:31 | call to Referer | untrusted input |
+| email.go:98:16:98:23 | content2 | email.go:87:21:87:31 | call to Referer : string | email.go:98:16:98:23 | content2 | Email content may contain $@. | email.go:87:21:87:31 | call to Referer | untrusted input |
diff --git a/ql/test/library-tests/semmle/go/frameworks/Email/MailData.expected b/ql/test/library-tests/semmle/go/frameworks/Email/MailData.expected
index 9e9801a28..160f7a0ea 100644
--- a/ql/test/library-tests/semmle/go/frameworks/Email/MailData.expected
+++ b/ql/test/library-tests/semmle/go/frameworks/Email/MailData.expected
@@ -1,9 +1,9 @@
 | mail.go:16:56:16:77 | type conversion |
-| mail.go:22:24:22:37 | untrustedInput |
+| mail.go:19:2:19:6 | definition of write |
 | mail.go:29:32:29:36 | alert |
 | mail.go:29:43:29:47 | alert |
 | mail.go:29:50:29:54 | alert |
-| mail.go:32:46:32:50 | alert |
-| mail.go:36:47:36:51 | alert |
-| mail.go:37:47:37:51 | alert |
+| mail.go:34:15:34:21 | content |
 | mail.go:40:35:40:39 | alert |
+| mail.go:40:46:40:53 | content2 |
+| mail.go:40:56:40:63 | content3 |

From b177d58c88eb4486947692651a090bd572098c54 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Mon, 4 May 2020 17:07:15 +0100
Subject: [PATCH 08/10] Tweak test.

The query under test isn't a `@problem` query, so we should refer to "alerts".
---
 .../go/frameworks/Email/EmailData.expected    |  9 +++++++
 .../Email/{MailData.ql => EmailData.ql}       |  0
 .../go/frameworks/Email/MailData.expected     |  9 -------
 .../semmle/go/frameworks/Email/mail.go        | 26 ++++++++-----------
 4 files changed, 20 insertions(+), 24 deletions(-)
 create mode 100644 ql/test/library-tests/semmle/go/frameworks/Email/EmailData.expected
 rename ql/test/library-tests/semmle/go/frameworks/Email/{MailData.ql => EmailData.ql} (100%)
 delete mode 100644 ql/test/library-tests/semmle/go/frameworks/Email/MailData.expected

diff --git a/ql/test/library-tests/semmle/go/frameworks/Email/EmailData.expected b/ql/test/library-tests/semmle/go/frameworks/Email/EmailData.expected
new file mode 100644
index 000000000..99b33b4a7
--- /dev/null
+++ b/ql/test/library-tests/semmle/go/frameworks/Email/EmailData.expected
@@ -0,0 +1,9 @@
+| mail.go:15:73:15:94 | type conversion |
+| mail.go:18:19:18:23 | definition of write |
+| mail.go:26:49:26:52 | text |
+| mail.go:26:76:26:79 | text |
+| mail.go:27:20:27:23 | text |
+| mail.go:31:33:31:39 | content |
+| mail.go:36:52:36:55 | text |
+| mail.go:36:79:36:86 | content2 |
+| mail.go:37:20:37:27 | content3 |
diff --git a/ql/test/library-tests/semmle/go/frameworks/Email/MailData.ql b/ql/test/library-tests/semmle/go/frameworks/Email/EmailData.ql
similarity index 100%
rename from ql/test/library-tests/semmle/go/frameworks/Email/MailData.ql
rename to ql/test/library-tests/semmle/go/frameworks/Email/EmailData.ql
diff --git a/ql/test/library-tests/semmle/go/frameworks/Email/MailData.expected b/ql/test/library-tests/semmle/go/frameworks/Email/MailData.expected
deleted file mode 100644
index 160f7a0ea..000000000
--- a/ql/test/library-tests/semmle/go/frameworks/Email/MailData.expected
+++ /dev/null
@@ -1,9 +0,0 @@
-| mail.go:16:56:16:77 | type conversion |
-| mail.go:19:2:19:6 | definition of write |
-| mail.go:29:32:29:36 | alert |
-| mail.go:29:43:29:47 | alert |
-| mail.go:29:50:29:54 | alert |
-| mail.go:34:15:34:21 | content |
-| mail.go:40:35:40:39 | alert |
-| mail.go:40:46:40:53 | content2 |
-| mail.go:40:56:40:63 | content3 |
diff --git a/ql/test/library-tests/semmle/go/frameworks/Email/mail.go b/ql/test/library-tests/semmle/go/frameworks/Email/mail.go
index 823ec92ac..fe5565c23 100644
--- a/ql/test/library-tests/semmle/go/frameworks/Email/mail.go
+++ b/ql/test/library-tests/semmle/go/frameworks/Email/mail.go
@@ -12,31 +12,27 @@ import (
 func main() {
 	untrustedInput := "test"
 
-	// Not OK - 1 alert
-	smtp.SendMail("test.test", nil, "from@from.com", nil, []byte(untrustedInput))
+	smtp.SendMail("test.test", nil, "from@from.com", nil /* email data */, []byte(untrustedInput))
 
 	s, _ := smtp.Dial("test.test")
-	write, _ := s.Data()
+	/* email data */ write, _ := s.Data()
 
-	// Not OK - 1 alert
 	io.WriteString(write, untrustedInput)
 
 	from := sendgrid.NewEmail("from", "from@from.com")
 	to := sendgrid.NewEmail("to", "to@to.com")
-	alert := "sub"
+	text := "sub"
 
-	// Not OK - 3 alerts
-	sendgrid.NewSingleEmail(from, alert, to, alert, alert)
+	sendgrid.NewSingleEmail(from /* email data */, text, to /* email data */, text,
+		/* email data */ text)
 
-	// Not OK - 1 alert
-	content := sendgrid.NewContent("text/html", alert)
+	content := sendgrid.NewContent("text/html", text)
 	v := sendgrid.NewV3Mail()
-	v.AddContent(content)
+	v.AddContent( /* email data */ content)
 
-	content2 := sendgrid.NewContent("text/html", alert)
-	content3 := sendgrid.NewContent("text/html", alert)
-
-	// Not OK - 3 alerts
-	v = sendgrid.NewV3MailInit(from, alert, to, content2, content3)
+	content2 := sendgrid.NewContent("text/html", text)
+	content3 := sendgrid.NewContent("text/html", text)
 
+	v = sendgrid.NewV3MailInit(from /* email data */, text, to /* email data */, content2,
+		/* email data */ content3)
 }

From a79f2b4f44b0fea1e9416744c45386c65d690757 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Tue, 5 May 2020 12:05:01 +0100
Subject: [PATCH 09/10] Add change note for `CleartextLogging`.

---
 change-notes/2020-05-05-clear-text-logging.md | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 change-notes/2020-05-05-clear-text-logging.md

diff --git a/change-notes/2020-05-05-clear-text-logging.md b/change-notes/2020-05-05-clear-text-logging.md
new file mode 100644
index 000000000..ad9e97455
--- /dev/null
+++ b/change-notes/2020-05-05-clear-text-logging.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The query "Clear-text logging of sensitive information" has been improved to recognize more logging APIs, which may lead to more alerts.
\ No newline at end of file

From 08f5451fce22c18b9a06b284e79a7d5c224c17aa Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Wed, 6 May 2020 07:32:15 +0100
Subject: [PATCH 10/10] Address review comments.

---
 .../semmle/go/dataflow/internal/DataFlowUtil.qll   | 14 ++++++++------
 ql/src/semmle/go/frameworks/Stdlib.qll             |  3 ++-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
index 6ccd6338b..d01829a1f 100644
--- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
+++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
@@ -408,7 +408,7 @@ class PostUpdateNode extends Node {
       )
       or
       preupd instanceof ArgumentNode and
-      mutableType(preupd.getType().getUnderlyingType())
+      mutableType(preupd.getType())
     ) and
     (
       preupd = this.(SsaNode).getAUse()
@@ -463,11 +463,13 @@ class ArgumentNode extends Node {
  * mutate it or something it points to.
  */
 predicate mutableType(Type tp) {
-  tp instanceof ArrayType or
-  tp instanceof SliceType or
-  tp instanceof MapType or
-  tp instanceof PointerType or
-  tp instanceof InterfaceType
+  exists(Type underlying | underlying = tp.getUnderlyingType() |
+    underlying instanceof ArrayType or
+    underlying instanceof SliceType or
+    underlying instanceof MapType or
+    underlying instanceof PointerType or
+    underlying instanceof InterfaceType
+  )
 }
 
 /**
diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll
index 86c8a3909..36c9516a4 100644
--- a/ql/src/semmle/go/frameworks/Stdlib.qll
+++ b/ql/src/semmle/go/frameworks/Stdlib.qll
@@ -115,7 +115,8 @@ module Fmt {
     Sscanner() { this.hasQualifiedName("fmt", ["Sscan", "Sscanf", "Sscanln"]) }
 
     override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
-      input.isParameter(0) and output.isParameter(any(int i | i > 0))
+      input.isParameter(0) and
+      exists(int i | if getName() = "Sscanf" then i > 1 else i > 0 | output.isParameter(i))
     }
   }
 }