From 285f392a12b0d797d771744788b610a96f60a85e Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Fri, 21 Feb 2020 10:14:26 +0000
Subject: [PATCH] Sharpen the sources for `StringBreak`.

`json.Marshal` returns two results, we only want to consider the first one as a source.
---
 .../go/security/StringBreakCustomizations.qll      |  3 ++-
 .../Security/CWE-089/StringBreak.expected          |  6 +++---
 ql/test/query-tests/Security/CWE-089/tst.go        | 14 ++++++++++++++
 3 files changed, 19 insertions(+), 4 deletions(-)
 create mode 100644 ql/test/query-tests/Security/CWE-089/tst.go

diff --git a/ql/src/semmle/go/security/StringBreakCustomizations.qll b/ql/src/semmle/go/security/StringBreakCustomizations.qll
index 29769cb0139..bf948fe825b 100644
--- a/ql/src/semmle/go/security/StringBreakCustomizations.qll
+++ b/ql/src/semmle/go/security/StringBreakCustomizations.qll
@@ -49,7 +49,8 @@ module StringBreak {
   class JsonMarshalAsSource extends Source {
     JsonMarshalAsSource() {
       exists(Function jsonMarshal | jsonMarshal.hasQualifiedName("encoding/json", "Marshal") |
-        this = jsonMarshal.getACall()
+        // we are only interested in the first result (the second result is an error)
+        this = DataFlow::extractTupleElement(jsonMarshal.getACall(), 0)
       )
     }
   }
diff --git a/ql/test/query-tests/Security/CWE-089/StringBreak.expected b/ql/test/query-tests/Security/CWE-089/StringBreak.expected
index 37b1862708e..783bbd0a247 100644
--- a/ql/test/query-tests/Security/CWE-089/StringBreak.expected
+++ b/ql/test/query-tests/Security/CWE-089/StringBreak.expected
@@ -1,7 +1,7 @@
 edges
-| StringBreak.go:10:20:10:40 | call to Marshal : tuple type | StringBreak.go:14:47:14:57 | versionJSON |
+| StringBreak.go:10:2:10:40 | ... := ...[0] : slice type | StringBreak.go:14:47:14:57 | versionJSON |
 nodes
-| StringBreak.go:10:20:10:40 | call to Marshal : tuple type | semmle.label | call to Marshal : tuple type |
+| StringBreak.go:10:2:10:40 | ... := ...[0] : slice type | semmle.label | ... := ...[0] : slice type |
 | StringBreak.go:14:47:14:57 | versionJSON | semmle.label | versionJSON |
 #select
-| StringBreak.go:14:47:14:57 | versionJSON | StringBreak.go:10:20:10:40 | call to Marshal : tuple type | StringBreak.go:14:47:14:57 | versionJSON | If this $@ contains a single quote, it could break out of the enclosing quotes. | StringBreak.go:10:20:10:40 | call to Marshal | JSON value |
+| StringBreak.go:14:47:14:57 | versionJSON | StringBreak.go:10:2:10:40 | ... := ...[0] : slice type | StringBreak.go:14:47:14:57 | versionJSON | If this $@ contains a single quote, it could break out of the enclosing quotes. | StringBreak.go:10:2:10:40 | ... := ...[0] | JSON value |
diff --git a/ql/test/query-tests/Security/CWE-089/tst.go b/ql/test/query-tests/Security/CWE-089/tst.go
new file mode 100644
index 00000000000..af0d7e6cf79
--- /dev/null
+++ b/ql/test/query-tests/Security/CWE-089/tst.go
@@ -0,0 +1,14 @@
+package main
+
+import (
+	"encoding/json"
+	"fmt"
+)
+
+func marshal(version interface{}) string {
+	versionJSON, err := json.Marshal(version)
+	if err != nil {
+		return fmt.Sprintf("error: '%v'", err) // OK
+	}
+	return versionJSON
+}