Merge pull request #129 from max-schaefer/fix-argument-post-update-nodes

Fix and improve taint-tracking through function arguments

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.

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.
  */

ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll

@@ -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
+  )
 }
 
 /**

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
@@ -54,15 +46,15 @@ module EmailData {
   /** Gets the package name `github.com/sendgrid/sendgrid-go/helpers/mail`. */
   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. */
@@ -76,18 +68,45 @@ 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()
       )
     }
   }
 }
+
+/**
+ * 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)
+  }
+}

ql/src/semmle/go/frameworks/Stdlib.qll

@@ -67,45 +67,96 @@ 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() }
   }
+
+  /** 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)
+    }
+  }
+
+  /** 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
+      exists(int i | if getName() = "Sscanf" then i > 1 else i > 0 | output.isParameter(i))
+    }
+  }
 }
 
 /** 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 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.isResult() and output.isParameter(0)
+    }
+  }
 }
 
 /** Provides models of commonly used functions in the `io/ioutil` package. */

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 |

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 |

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 |

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 |

ql/test/library-tests/semmle/go/frameworks/Email/MailData.expected

@@ -1,9 +0,0 @@
-| mail.go:16:56:16:77 | type conversion |
-| mail.go:22:24:22:37 | untrustedInput |
-| 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:40:35:40:39 | alert |

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)
 }