From 4f6ce61de2ff53b89fae51afdd00c24a8cd5bb34 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 15 Jun 2020 16:15:26 +0100 Subject: [PATCH 1/8] Move EmailInjection query out of experimental --- ql/src/{experimental => Security}/CWE-640/EmailBad.go | 0 ql/src/{experimental => Security}/CWE-640/EmailGood.go | 0 ql/src/{experimental => Security}/CWE-640/EmailInjection.qhelp | 0 ql/src/{experimental => Security}/CWE-640/EmailInjection.ql | 0 ql/src/{experimental => Security}/CWE-640/EmailInjection.qll | 0 .../CWE-640/EmailInjectionCustomizations.qll | 0 ql/test/experimental/CWE-640/EmailInjection.qlref | 1 - .../Security}/CWE-640/EmailInjection.expected | 0 ql/test/query-tests/Security/CWE-640/EmailInjection.qlref | 1 + ql/test/{experimental => query-tests/Security}/CWE-640/email.go | 0 ql/test/{experimental => query-tests/Security}/CWE-640/go.mod | 0 .../vendor/github.com/sendgrid/sendgrid-go/helpers/LICENSE | 0 .../vendor/github.com/sendgrid/sendgrid-go/helpers/mail/stub.go | 0 .../Security}/CWE-640/vendor/modules.txt | 0 14 files changed, 1 insertion(+), 1 deletion(-) rename ql/src/{experimental => Security}/CWE-640/EmailBad.go (100%) rename ql/src/{experimental => Security}/CWE-640/EmailGood.go (100%) rename ql/src/{experimental => Security}/CWE-640/EmailInjection.qhelp (100%) rename ql/src/{experimental => Security}/CWE-640/EmailInjection.ql (100%) rename ql/src/{experimental => Security}/CWE-640/EmailInjection.qll (100%) rename ql/src/{experimental => Security}/CWE-640/EmailInjectionCustomizations.qll (100%) delete mode 100644 ql/test/experimental/CWE-640/EmailInjection.qlref rename ql/test/{experimental => query-tests/Security}/CWE-640/EmailInjection.expected (100%) create mode 100644 ql/test/query-tests/Security/CWE-640/EmailInjection.qlref rename ql/test/{experimental => query-tests/Security}/CWE-640/email.go (100%) rename ql/test/{experimental => query-tests/Security}/CWE-640/go.mod (100%) rename ql/test/{experimental => query-tests/Security}/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/LICENSE (100%) rename ql/test/{experimental => query-tests/Security}/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/mail/stub.go (100%) rename ql/test/{experimental => query-tests/Security}/CWE-640/vendor/modules.txt (100%) diff --git a/ql/src/experimental/CWE-640/EmailBad.go b/ql/src/Security/CWE-640/EmailBad.go similarity index 100% rename from ql/src/experimental/CWE-640/EmailBad.go rename to ql/src/Security/CWE-640/EmailBad.go diff --git a/ql/src/experimental/CWE-640/EmailGood.go b/ql/src/Security/CWE-640/EmailGood.go similarity index 100% rename from ql/src/experimental/CWE-640/EmailGood.go rename to ql/src/Security/CWE-640/EmailGood.go diff --git a/ql/src/experimental/CWE-640/EmailInjection.qhelp b/ql/src/Security/CWE-640/EmailInjection.qhelp similarity index 100% rename from ql/src/experimental/CWE-640/EmailInjection.qhelp rename to ql/src/Security/CWE-640/EmailInjection.qhelp diff --git a/ql/src/experimental/CWE-640/EmailInjection.ql b/ql/src/Security/CWE-640/EmailInjection.ql similarity index 100% rename from ql/src/experimental/CWE-640/EmailInjection.ql rename to ql/src/Security/CWE-640/EmailInjection.ql diff --git a/ql/src/experimental/CWE-640/EmailInjection.qll b/ql/src/Security/CWE-640/EmailInjection.qll similarity index 100% rename from ql/src/experimental/CWE-640/EmailInjection.qll rename to ql/src/Security/CWE-640/EmailInjection.qll diff --git a/ql/src/experimental/CWE-640/EmailInjectionCustomizations.qll b/ql/src/Security/CWE-640/EmailInjectionCustomizations.qll similarity index 100% rename from ql/src/experimental/CWE-640/EmailInjectionCustomizations.qll rename to ql/src/Security/CWE-640/EmailInjectionCustomizations.qll diff --git a/ql/test/experimental/CWE-640/EmailInjection.qlref b/ql/test/experimental/CWE-640/EmailInjection.qlref deleted file mode 100644 index a6d8abad1c9..00000000000 --- a/ql/test/experimental/CWE-640/EmailInjection.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/CWE-640/EmailInjection.ql \ No newline at end of file diff --git a/ql/test/experimental/CWE-640/EmailInjection.expected b/ql/test/query-tests/Security/CWE-640/EmailInjection.expected similarity index 100% rename from ql/test/experimental/CWE-640/EmailInjection.expected rename to ql/test/query-tests/Security/CWE-640/EmailInjection.expected diff --git a/ql/test/query-tests/Security/CWE-640/EmailInjection.qlref b/ql/test/query-tests/Security/CWE-640/EmailInjection.qlref new file mode 100644 index 00000000000..98862a2de38 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-640/EmailInjection.qlref @@ -0,0 +1 @@ +Security/CWE-640/EmailInjection.ql \ No newline at end of file diff --git a/ql/test/experimental/CWE-640/email.go b/ql/test/query-tests/Security/CWE-640/email.go similarity index 100% rename from ql/test/experimental/CWE-640/email.go rename to ql/test/query-tests/Security/CWE-640/email.go diff --git a/ql/test/experimental/CWE-640/go.mod b/ql/test/query-tests/Security/CWE-640/go.mod similarity index 100% rename from ql/test/experimental/CWE-640/go.mod rename to ql/test/query-tests/Security/CWE-640/go.mod diff --git a/ql/test/experimental/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/LICENSE b/ql/test/query-tests/Security/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/LICENSE similarity index 100% rename from ql/test/experimental/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/LICENSE rename to ql/test/query-tests/Security/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/LICENSE diff --git a/ql/test/experimental/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/mail/stub.go b/ql/test/query-tests/Security/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/mail/stub.go similarity index 100% rename from ql/test/experimental/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/mail/stub.go rename to ql/test/query-tests/Security/CWE-640/vendor/github.com/sendgrid/sendgrid-go/helpers/mail/stub.go diff --git a/ql/test/experimental/CWE-640/vendor/modules.txt b/ql/test/query-tests/Security/CWE-640/vendor/modules.txt similarity index 100% rename from ql/test/experimental/CWE-640/vendor/modules.txt rename to ql/test/query-tests/Security/CWE-640/vendor/modules.txt From f27ecdabb896e3dd7a5798ac82583679ba40e071 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 15 Jun 2020 16:18:28 +0100 Subject: [PATCH 2/8] Set precision to high --- ql/src/Security/CWE-640/EmailInjection.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/ql/src/Security/CWE-640/EmailInjection.ql b/ql/src/Security/CWE-640/EmailInjection.ql index 7cf11fa1a3e..1ae4d751342 100644 --- a/ql/src/Security/CWE-640/EmailInjection.ql +++ b/ql/src/Security/CWE-640/EmailInjection.ql @@ -8,6 +8,7 @@ * @problem.severity error * @tags security * external/cwe/cwe-640 + * @precision high */ import go From 336eba1be496039ad068c4f680a146ab18f2dd69 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 16 Jun 2020 12:48:43 +0100 Subject: [PATCH 3/8] Add Hash.Write and similar as sanitizers --- ql/src/Security/CWE-640/EmailInjection.qll | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ql/src/Security/CWE-640/EmailInjection.qll b/ql/src/Security/CWE-640/EmailInjection.qll index 4cf8b382c98..726836ed922 100644 --- a/ql/src/Security/CWE-640/EmailInjection.qll +++ b/ql/src/Security/CWE-640/EmailInjection.qll @@ -24,6 +24,19 @@ module EmailInjection { override predicate isSource(DataFlow::Node source) { source instanceof Source } + override predicate isSanitizerOut(DataFlow::Node node) { + exists(DataFlow::CallNode call | + call.getTarget().hasQualifiedName("hash.Hash", "Write") and + ( + call.getReceiver().getType().getName() = "Hash" or + call.getReceiver().getType().getName() = "Hash32" or + call.getReceiver().getType().getName() = "Hash64" + ) + | + node = call.getArgument(0) + ) + } + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } } } From 1b49bcc3b38da4cafe242e43367369144b7aef99 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 16 Jun 2020 15:44:51 +0100 Subject: [PATCH 4/8] Put code snippets from qhelp in test folder --- .../query-tests/Security/CWE-640/EmailBad.go | 13 +++ .../query-tests/Security/CWE-640/EmailGood.go | 13 +++ .../Security/CWE-640/EmailInjection.expected | 80 +++++++++---------- .../Security/CWE-640/{email.go => main.go} | 24 ++---- 4 files changed, 74 insertions(+), 56 deletions(-) create mode 100644 ql/test/query-tests/Security/CWE-640/EmailBad.go create mode 100644 ql/test/query-tests/Security/CWE-640/EmailGood.go rename ql/test/query-tests/Security/CWE-640/{email.go => main.go} (81%) diff --git a/ql/test/query-tests/Security/CWE-640/EmailBad.go b/ql/test/query-tests/Security/CWE-640/EmailBad.go new file mode 100644 index 00000000000..aab8467b340 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-640/EmailBad.go @@ -0,0 +1,13 @@ +package main + +import ( + "net/http" + "net/smtp" +) + +func mail(w http.ResponseWriter, r *http.Request) { + host := r.Header.Get("Host") + token := backend.getUserSecretResetToken(email) + body := "Click to reset password: " + host + "/" + token + smtp.SendMail("test.test", nil, "from@from.com", nil, []byte(body)) +} diff --git a/ql/test/query-tests/Security/CWE-640/EmailGood.go b/ql/test/query-tests/Security/CWE-640/EmailGood.go new file mode 100644 index 00000000000..5a85e543540 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-640/EmailGood.go @@ -0,0 +1,13 @@ +package main + +import ( + "net/http" + "net/smtp" +) + +func mailGood(w http.ResponseWriter, r *http.Request) { + host := config["Host"] + token := backend.getUserSecretResetToken(email) + body := "Click to reset password: " + host + "/" + token + smtp.SendMail("test.test", nil, "from@from.com", nil, []byte(body)) +} diff --git a/ql/test/query-tests/Security/CWE-640/EmailInjection.expected b/ql/test/query-tests/Security/CWE-640/EmailInjection.expected index 37a3af5ae75..9e4eb436872 100644 --- a/ql/test/query-tests/Security/CWE-640/EmailInjection.expected +++ b/ql/test/query-tests/Security/CWE-640/EmailInjection.expected @@ -1,43 +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: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: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:98:16:98:23 | content2 | +| EmailBad.go:9:10:9:17 | selection of Header : Header | EmailBad.go:12:56:12:67 | type conversion | +| main.go:26:21:26:31 | call to Referer : string | main.go:28:57:28:78 | type conversion | +| main.go:34:21:34:31 | call to Referer : string | main.go:37:3:37:7 | definition of write | +| main.go:43:21:43:31 | call to Referer : string | main.go:49:46:49:59 | untrustedInput | +| main.go:43:21:43:31 | call to Referer : string | main.go:50:52:50:65 | untrustedInput | +| main.go:55:21:55:31 | call to Referer : string | main.go:60:16:60:22 | content | +| main.go:65:21:65:31 | call to Referer : string | main.go:73:50:73:56 | content | +| main.go:65:21:65:31 | call to Referer : string | main.go:73:59:73:65 | content | +| main.go:65:21:65:31 | call to Referer : string | main.go:74:16:74:22 | content | +| main.go:79:21:79:31 | call to Referer : string | main.go:86:37:86:50 | untrustedInput | +| main.go:79:21:79:31 | call to Referer : string | main.go:90:16:90: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: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: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: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:98:16:98:23 | content2 | semmle.label | content2 | +| EmailBad.go:9:10:9:17 | selection of Header : Header | semmle.label | selection of Header : Header | +| EmailBad.go:12:56:12:67 | type conversion | semmle.label | type conversion | +| main.go:26:21:26:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:28:57:28:78 | type conversion | semmle.label | type conversion | +| main.go:34:21:34:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:37:3:37:7 | definition of write | semmle.label | definition of write | +| main.go:43:21:43:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:49:46:49:59 | untrustedInput | semmle.label | untrustedInput | +| main.go:50:52:50:65 | untrustedInput | semmle.label | untrustedInput | +| main.go:55:21:55:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:60:16:60:22 | content | semmle.label | content | +| main.go:65:21:65:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:73:50:73:56 | content | semmle.label | content | +| main.go:73:59:73:65 | content | semmle.label | content | +| main.go:74:16:74:22 | content | semmle.label | content | +| main.go:79:21:79:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:86:37:86:50 | untrustedInput | semmle.label | untrustedInput | +| main.go:90:16:90: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: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: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: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 | +| EmailBad.go:12:56:12:67 | type conversion | EmailBad.go:9:10:9:17 | selection of Header : Header | EmailBad.go:12:56:12:67 | type conversion | Email content may contain $@. | EmailBad.go:9:10:9:17 | selection of Header | untrusted input | +| main.go:28:57:28:78 | type conversion | main.go:26:21:26:31 | call to Referer : string | main.go:28:57:28:78 | type conversion | Email content may contain $@. | main.go:26:21:26:31 | call to Referer | untrusted input | +| main.go:37:3:37:7 | definition of write | main.go:34:21:34:31 | call to Referer : string | main.go:37:3:37:7 | definition of write | Email content may contain $@. | main.go:34:21:34:31 | call to Referer | untrusted input | +| main.go:49:46:49:59 | untrustedInput | main.go:43:21:43:31 | call to Referer : string | main.go:49:46:49:59 | untrustedInput | Email content may contain $@. | main.go:43:21:43:31 | call to Referer | untrusted input | +| main.go:50:52:50:65 | untrustedInput | main.go:43:21:43:31 | call to Referer : string | main.go:50:52:50:65 | untrustedInput | Email content may contain $@. | main.go:43:21:43:31 | call to Referer | untrusted input | +| main.go:60:16:60:22 | content | main.go:55:21:55:31 | call to Referer : string | main.go:60:16:60:22 | content | Email content may contain $@. | main.go:55:21:55:31 | call to Referer | untrusted input | +| main.go:73:50:73:56 | content | main.go:65:21:65:31 | call to Referer : string | main.go:73:50:73:56 | content | Email content may contain $@. | main.go:65:21:65:31 | call to Referer | untrusted input | +| main.go:73:59:73:65 | content | main.go:65:21:65:31 | call to Referer : string | main.go:73:59:73:65 | content | Email content may contain $@. | main.go:65:21:65:31 | call to Referer | untrusted input | +| main.go:74:16:74:22 | content | main.go:65:21:65:31 | call to Referer : string | main.go:74:16:74:22 | content | Email content may contain $@. | main.go:65:21:65:31 | call to Referer | untrusted input | +| main.go:86:37:86:50 | untrustedInput | main.go:79:21:79:31 | call to Referer : string | main.go:86:37:86:50 | untrustedInput | Email content may contain $@. | main.go:79:21:79:31 | call to Referer | untrusted input | +| main.go:90:16:90:23 | content2 | main.go:79:21:79:31 | call to Referer : string | main.go:90:16:90:23 | content2 | Email content may contain $@. | main.go:79:21:79:31 | call to Referer | untrusted input | diff --git a/ql/test/query-tests/Security/CWE-640/email.go b/ql/test/query-tests/Security/CWE-640/main.go similarity index 81% rename from ql/test/query-tests/Security/CWE-640/email.go rename to ql/test/query-tests/Security/CWE-640/main.go index 44a54b59fe7..2f57864ad06 100644 --- a/ql/test/query-tests/Security/CWE-640/email.go +++ b/ql/test/query-tests/Security/CWE-640/main.go @@ -11,23 +11,15 @@ import ( sendgrid "github.com/sendgrid/sendgrid-go/helpers/mail" ) -// OK -func mailGood(w http.ResponseWriter, r *http.Request) { - host := config["Host"] - token := backend.getUserSecretResetToken(email) - body := "Click to reset password: " + host + "/" + token - smtp.SendMail("test.test", nil, "from@from.com", nil, []byte(body)) -} - -// Not OK -func mail(w http.ResponseWriter, r *http.Request) { - host := r.Header.Get("Host") - token := backend.getUserSecretResetToken(email) - body := "Click to reset password: " + host + "/" + token - smtp.SendMail("test.test", nil, "from@from.com", nil, []byte(body)) -} - func main() { + var w http.ResponseWriter + var r *http.Request + + // Not OK + mail(w, r) + + // OK + mailGood(w, r) // Not OK http.HandleFunc("/ex0", func(w http.ResponseWriter, r *http.Request) { From a3bc09473186bbc7a07f3570c0d0b14d0de408d8 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 16 Jun 2020 15:48:39 +0100 Subject: [PATCH 5/8] Add change note --- change-notes/2020-06-16-email-injection.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2020-06-16-email-injection.md diff --git a/change-notes/2020-06-16-email-injection.md b/change-notes/2020-06-16-email-injection.md new file mode 100644 index 00000000000..63abd57f2d6 --- /dev/null +++ b/change-notes/2020-06-16-email-injection.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "Email injection" (`go/email-injection`) has been moved out of the experimental folder. The query detects when untrusted input can be incorporated directly into an email. From f926808c8aae56215cd8c7d1b17607ebf8985010 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 17 Jun 2020 10:06:35 +0100 Subject: [PATCH 6/8] Address review comments --- ql/src/Security/CWE-640/EmailInjection.qll | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/ql/src/Security/CWE-640/EmailInjection.qll b/ql/src/Security/CWE-640/EmailInjection.qll index 726836ed922..4cf8b382c98 100644 --- a/ql/src/Security/CWE-640/EmailInjection.qll +++ b/ql/src/Security/CWE-640/EmailInjection.qll @@ -24,19 +24,6 @@ module EmailInjection { override predicate isSource(DataFlow::Node source) { source instanceof Source } - override predicate isSanitizerOut(DataFlow::Node node) { - exists(DataFlow::CallNode call | - call.getTarget().hasQualifiedName("hash.Hash", "Write") and - ( - call.getReceiver().getType().getName() = "Hash" or - call.getReceiver().getType().getName() = "Hash32" or - call.getReceiver().getType().getName() = "Hash64" - ) - | - node = call.getArgument(0) - ) - } - override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } } } From 83697f62ac7a0212d7c508b62c6daf702aa2b0bc Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 17 Jun 2020 14:21:37 +0100 Subject: [PATCH 7/8] Address review comments on qhelp --- ql/src/Security/CWE-640/EmailInjection.qhelp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ql/src/Security/CWE-640/EmailInjection.qhelp b/ql/src/Security/CWE-640/EmailInjection.qhelp index f3ac1dc4bbe..1da839cedb8 100644 --- a/ql/src/Security/CWE-640/EmailInjection.qhelp +++ b/ql/src/Security/CWE-640/EmailInjection.qhelp @@ -4,7 +4,7 @@

Using untrusted input to construct an email can cause multiple security vulnerabilities. For instance, inclusion of an untrusted input in an email body - may allow an attacker to conduct Cross Site Scripting (XSS) attacks, while + may allow an attacker to conduct cross-site scripting (XSS) attacks, while inclusion of an HTTP header may allow a full account compromise as shown in the example below.

@@ -19,13 +19,13 @@ In the following example snippet, the host field is user controlled.

- A malicious user can send an HTTP request to the targeted web site, - but with a Host header that refers to their own web site. This means the + A malicious user can send an HTTP request to the targeted website, + but with a Host header that refers to their own website. This means the emails will be sent out to potential victims, originating from a server - they trust, but with links leading to a malicious web site. + they trust, but with links leading to a malicious website.

- If the email contains a password reset link, and should the victim click + If the email contains a password reset link, and the victim clicks the link, the secret reset token will be leaked to the attacker. Using the leaked token, the attacker can then construct the real reset link and use it to change the victim's password. @@ -38,7 +38,7 @@

  • - OWASP + OWASP: Content Spoofing .
    • From 49abd0b9b1103c37f13c862aad659dfb2eb2c564 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 17 Jun 2020 14:33:26 +0100 Subject: [PATCH 8/8] Add test using hashing --- .../Security/CWE-640/EmailInjection.expected | 72 +++++++++---------- ql/test/query-tests/Security/CWE-640/main.go | 16 +++++ 2 files changed, 52 insertions(+), 36 deletions(-) diff --git a/ql/test/query-tests/Security/CWE-640/EmailInjection.expected b/ql/test/query-tests/Security/CWE-640/EmailInjection.expected index 9e4eb436872..6e84a0466cd 100644 --- a/ql/test/query-tests/Security/CWE-640/EmailInjection.expected +++ b/ql/test/query-tests/Security/CWE-640/EmailInjection.expected @@ -1,43 +1,43 @@ edges | EmailBad.go:9:10:9:17 | selection of Header : Header | EmailBad.go:12:56:12:67 | type conversion | -| main.go:26:21:26:31 | call to Referer : string | main.go:28:57:28:78 | type conversion | -| main.go:34:21:34:31 | call to Referer : string | main.go:37:3:37:7 | definition of write | -| main.go:43:21:43:31 | call to Referer : string | main.go:49:46:49:59 | untrustedInput | -| main.go:43:21:43:31 | call to Referer : string | main.go:50:52:50:65 | untrustedInput | -| main.go:55:21:55:31 | call to Referer : string | main.go:60:16:60:22 | content | -| main.go:65:21:65:31 | call to Referer : string | main.go:73:50:73:56 | content | -| main.go:65:21:65:31 | call to Referer : string | main.go:73:59:73:65 | content | -| main.go:65:21:65:31 | call to Referer : string | main.go:74:16:74:22 | content | -| main.go:79:21:79:31 | call to Referer : string | main.go:86:37:86:50 | untrustedInput | -| main.go:79:21:79:31 | call to Referer : string | main.go:90:16:90:23 | content2 | +| main.go:29:21:29:31 | call to Referer : string | main.go:31:57:31:78 | type conversion | +| main.go:37:21:37:31 | call to Referer : string | main.go:40:3:40:7 | definition of write | +| main.go:46:21:46:31 | call to Referer : string | main.go:52:46:52:59 | untrustedInput | +| main.go:46:21:46:31 | call to Referer : string | main.go:53:52:53:65 | untrustedInput | +| main.go:58:21:58:31 | call to Referer : string | main.go:63:16:63:22 | content | +| main.go:68:21:68:31 | call to Referer : string | main.go:76:50:76:56 | content | +| main.go:68:21:68:31 | call to Referer : string | main.go:76:59:76:65 | content | +| main.go:68:21:68:31 | call to Referer : string | main.go:77:16:77:22 | content | +| main.go:82:21:82:31 | call to Referer : string | main.go:89:37:89:50 | untrustedInput | +| main.go:82:21:82:31 | call to Referer : string | main.go:93:16:93:23 | content2 | nodes | EmailBad.go:9:10:9:17 | selection of Header : Header | semmle.label | selection of Header : Header | | EmailBad.go:12:56:12:67 | type conversion | semmle.label | type conversion | -| main.go:26:21:26:31 | call to Referer : string | semmle.label | call to Referer : string | -| main.go:28:57:28:78 | type conversion | semmle.label | type conversion | -| main.go:34:21:34:31 | call to Referer : string | semmle.label | call to Referer : string | -| main.go:37:3:37:7 | definition of write | semmle.label | definition of write | -| main.go:43:21:43:31 | call to Referer : string | semmle.label | call to Referer : string | -| main.go:49:46:49:59 | untrustedInput | semmle.label | untrustedInput | -| main.go:50:52:50:65 | untrustedInput | semmle.label | untrustedInput | -| main.go:55:21:55:31 | call to Referer : string | semmle.label | call to Referer : string | -| main.go:60:16:60:22 | content | semmle.label | content | -| main.go:65:21:65:31 | call to Referer : string | semmle.label | call to Referer : string | -| main.go:73:50:73:56 | content | semmle.label | content | -| main.go:73:59:73:65 | content | semmle.label | content | -| main.go:74:16:74:22 | content | semmle.label | content | -| main.go:79:21:79:31 | call to Referer : string | semmle.label | call to Referer : string | -| main.go:86:37:86:50 | untrustedInput | semmle.label | untrustedInput | -| main.go:90:16:90:23 | content2 | semmle.label | content2 | +| main.go:29:21:29:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:31:57:31:78 | type conversion | semmle.label | type conversion | +| main.go:37:21:37:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:40:3:40:7 | definition of write | semmle.label | definition of write | +| main.go:46:21:46:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:52:46:52:59 | untrustedInput | semmle.label | untrustedInput | +| main.go:53:52:53:65 | untrustedInput | semmle.label | untrustedInput | +| main.go:58:21:58:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:63:16:63:22 | content | semmle.label | content | +| main.go:68:21:68:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:76:50:76:56 | content | semmle.label | content | +| main.go:76:59:76:65 | content | semmle.label | content | +| main.go:77:16:77:22 | content | semmle.label | content | +| main.go:82:21:82:31 | call to Referer : string | semmle.label | call to Referer : string | +| main.go:89:37:89:50 | untrustedInput | semmle.label | untrustedInput | +| main.go:93:16:93:23 | content2 | semmle.label | content2 | #select | EmailBad.go:12:56:12:67 | type conversion | EmailBad.go:9:10:9:17 | selection of Header : Header | EmailBad.go:12:56:12:67 | type conversion | Email content may contain $@. | EmailBad.go:9:10:9:17 | selection of Header | untrusted input | -| main.go:28:57:28:78 | type conversion | main.go:26:21:26:31 | call to Referer : string | main.go:28:57:28:78 | type conversion | Email content may contain $@. | main.go:26:21:26:31 | call to Referer | untrusted input | -| main.go:37:3:37:7 | definition of write | main.go:34:21:34:31 | call to Referer : string | main.go:37:3:37:7 | definition of write | Email content may contain $@. | main.go:34:21:34:31 | call to Referer | untrusted input | -| main.go:49:46:49:59 | untrustedInput | main.go:43:21:43:31 | call to Referer : string | main.go:49:46:49:59 | untrustedInput | Email content may contain $@. | main.go:43:21:43:31 | call to Referer | untrusted input | -| main.go:50:52:50:65 | untrustedInput | main.go:43:21:43:31 | call to Referer : string | main.go:50:52:50:65 | untrustedInput | Email content may contain $@. | main.go:43:21:43:31 | call to Referer | untrusted input | -| main.go:60:16:60:22 | content | main.go:55:21:55:31 | call to Referer : string | main.go:60:16:60:22 | content | Email content may contain $@. | main.go:55:21:55:31 | call to Referer | untrusted input | -| main.go:73:50:73:56 | content | main.go:65:21:65:31 | call to Referer : string | main.go:73:50:73:56 | content | Email content may contain $@. | main.go:65:21:65:31 | call to Referer | untrusted input | -| main.go:73:59:73:65 | content | main.go:65:21:65:31 | call to Referer : string | main.go:73:59:73:65 | content | Email content may contain $@. | main.go:65:21:65:31 | call to Referer | untrusted input | -| main.go:74:16:74:22 | content | main.go:65:21:65:31 | call to Referer : string | main.go:74:16:74:22 | content | Email content may contain $@. | main.go:65:21:65:31 | call to Referer | untrusted input | -| main.go:86:37:86:50 | untrustedInput | main.go:79:21:79:31 | call to Referer : string | main.go:86:37:86:50 | untrustedInput | Email content may contain $@. | main.go:79:21:79:31 | call to Referer | untrusted input | -| main.go:90:16:90:23 | content2 | main.go:79:21:79:31 | call to Referer : string | main.go:90:16:90:23 | content2 | Email content may contain $@. | main.go:79:21:79:31 | call to Referer | untrusted input | +| main.go:31:57:31:78 | type conversion | main.go:29:21:29:31 | call to Referer : string | main.go:31:57:31:78 | type conversion | Email content may contain $@. | main.go:29:21:29:31 | call to Referer | untrusted input | +| main.go:40:3:40:7 | definition of write | main.go:37:21:37:31 | call to Referer : string | main.go:40:3:40:7 | definition of write | Email content may contain $@. | main.go:37:21:37:31 | call to Referer | untrusted input | +| main.go:52:46:52:59 | untrustedInput | main.go:46:21:46:31 | call to Referer : string | main.go:52:46:52:59 | untrustedInput | Email content may contain $@. | main.go:46:21:46:31 | call to Referer | untrusted input | +| main.go:53:52:53:65 | untrustedInput | main.go:46:21:46:31 | call to Referer : string | main.go:53:52:53:65 | untrustedInput | Email content may contain $@. | main.go:46:21:46:31 | call to Referer | untrusted input | +| main.go:63:16:63:22 | content | main.go:58:21:58:31 | call to Referer : string | main.go:63:16:63:22 | content | Email content may contain $@. | main.go:58:21:58:31 | call to Referer | untrusted input | +| main.go:76:50:76:56 | content | main.go:68:21:68:31 | call to Referer : string | main.go:76:50:76:56 | content | Email content may contain $@. | main.go:68:21:68:31 | call to Referer | untrusted input | +| main.go:76:59:76:65 | content | main.go:68:21:68:31 | call to Referer : string | main.go:76:59:76:65 | content | Email content may contain $@. | main.go:68:21:68:31 | call to Referer | untrusted input | +| main.go:77:16:77:22 | content | main.go:68:21:68:31 | call to Referer : string | main.go:77:16:77:22 | content | Email content may contain $@. | main.go:68:21:68:31 | call to Referer | untrusted input | +| main.go:89:37:89:50 | untrustedInput | main.go:82:21:82:31 | call to Referer : string | main.go:89:37:89:50 | untrustedInput | Email content may contain $@. | main.go:82:21:82:31 | call to Referer | untrusted input | +| main.go:93:16:93:23 | content2 | main.go:82:21:82:31 | call to Referer : string | main.go:93:16:93:23 | content2 | Email content may contain $@. | main.go:82:21:82:31 | call to Referer | untrusted input | diff --git a/ql/test/query-tests/Security/CWE-640/main.go b/ql/test/query-tests/Security/CWE-640/main.go index 2f57864ad06..c6685475a8d 100644 --- a/ql/test/query-tests/Security/CWE-640/main.go +++ b/ql/test/query-tests/Security/CWE-640/main.go @@ -3,6 +3,9 @@ package main //go:generate depstubber -vendor github.com/sendgrid/sendgrid-go/helpers/mail "" NewEmail,NewSingleEmail,NewContent,NewV3Mail,NewV3MailInit import ( + "crypto/hmac" + "crypto/sha256" + "encoding/base64" "io" "log" "net/http" @@ -90,6 +93,19 @@ func main() { v.AddContent(content2) }) + // OK + http.HandleFunc("/ex6", func(w http.ResponseWriter, r *http.Request) { + untrustedInput := r.Referer() + + sha256 := sha256.New + appsecret := "appid" + hash := hmac.New(sha256, []byte(appsecret)) + hash.Write([]byte(untrustedInput)) + signature := base64.StdEncoding.EncodeToString(hash.Sum(nil)) + + smtp.SendMail("test.test", nil, "from@from.com", nil, []byte(signature)) + }) + log.Println(http.ListenAndServe(":80", nil)) }