From c4c8bfd0270f3d7009f0fb7c953a26e2cb65442d Mon Sep 17 00:00:00 2001
From: Pestdoktor <jonas@fentker.eu>
Date: Fri, 9 Oct 2020 10:15:51 +0200
Subject: [PATCH] reject invalid UTF-8 (#1472)

* reject invalid UTF-8

Signed-off-by: Jonas Fentker <jonas@fentker.eu>

* update sytest-whitelist

Signed-off-by: Jonas Fentker <jonas@fentker.eu>

Co-authored-by: Kegsay <kegan@matrix.org>
---
 clientapi/httputil/httputil.go | 20 +++++++++++++++++++-
 clientapi/routing/device.go    | 12 +++++-------
 clientapi/routing/routing.go   |  6 ++++--
 sytest-whitelist               |  1 +
 4 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/clientapi/httputil/httputil.go b/clientapi/httputil/httputil.go
index b0fe6a6cb..29d7b0b37 100644
--- a/clientapi/httputil/httputil.go
+++ b/clientapi/httputil/httputil.go
@@ -16,7 +16,9 @@ package httputil
 
 import (
 	"encoding/json"
+	"io/ioutil"
 	"net/http"
+	"unicode/utf8"
 
 	"github.com/matrix-org/dendrite/clientapi/jsonerror"
 	"github.com/matrix-org/util"
@@ -25,7 +27,23 @@ import (
 // UnmarshalJSONRequest into the given interface pointer. Returns an error JSON response if
 // there was a problem unmarshalling. Calling this function consumes the request body.
 func UnmarshalJSONRequest(req *http.Request, iface interface{}) *util.JSONResponse {
-	if err := json.NewDecoder(req.Body).Decode(iface); err != nil {
+	// encoding/json allows invalid utf-8, matrix does not
+	// https://matrix.org/docs/spec/client_server/r0.6.1#api-standards
+	body, err := ioutil.ReadAll(req.Body)
+	if err != nil {
+		util.GetLogger(req.Context()).WithError(err).Error("ioutil.ReadAll failed")
+		resp := jsonerror.InternalServerError()
+		return &resp
+	}
+
+	if !utf8.Valid(body) {
+		return &util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: jsonerror.NotJSON("Body contains invalid UTF-8"),
+		}
+	}
+
+	if err := json.Unmarshal(body, iface); err != nil {
 		// TODO: We may want to suppress the Error() return in production? It's useful when
 		// debugging because an error will be produced for both invalid/malformed JSON AND
 		// valid JSON with incorrect types for values.
diff --git a/clientapi/routing/device.go b/clientapi/routing/device.go
index 56886d57f..d50c73b35 100644
--- a/clientapi/routing/device.go
+++ b/clientapi/routing/device.go
@@ -15,11 +15,11 @@
 package routing
 
 import (
-	"encoding/json"
 	"io/ioutil"
 	"net/http"
 
 	"github.com/matrix-org/dendrite/clientapi/auth"
+	"github.com/matrix-org/dendrite/clientapi/httputil"
 	"github.com/matrix-org/dendrite/clientapi/jsonerror"
 	"github.com/matrix-org/dendrite/userapi/api"
 	userapi "github.com/matrix-org/dendrite/userapi/api"
@@ -121,9 +121,8 @@ func UpdateDeviceByID(
 
 	payload := deviceUpdateJSON{}
 
-	if err := json.NewDecoder(req.Body).Decode(&payload); err != nil {
-		util.GetLogger(req.Context()).WithError(err).Error("json.NewDecoder.Decode failed")
-		return jsonerror.InternalServerError()
+	if resErr := httputil.UnmarshalJSONRequest(req, &payload); resErr != nil {
+		return *resErr
 	}
 
 	var performRes api.PerformDeviceUpdateResponse
@@ -211,9 +210,8 @@ func DeleteDevices(
 	ctx := req.Context()
 	payload := devicesDeleteJSON{}
 
-	if err := json.NewDecoder(req.Body).Decode(&payload); err != nil {
-		util.GetLogger(ctx).WithError(err).Error("json.NewDecoder.Decode failed")
-		return jsonerror.InternalServerError()
+	if resErr := httputil.UnmarshalJSONRequest(req, &payload); resErr != nil {
+		return *resErr
 	}
 
 	defer req.Body.Close() // nolint: errcheck
diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go
index b547efb4b..4f99237f5 100644
--- a/clientapi/routing/routing.go
+++ b/clientapi/routing/routing.go
@@ -23,6 +23,7 @@ import (
 	appserviceAPI "github.com/matrix-org/dendrite/appservice/api"
 	"github.com/matrix-org/dendrite/clientapi/api"
 	"github.com/matrix-org/dendrite/clientapi/auth"
+	clientutil "github.com/matrix-org/dendrite/clientapi/httputil"
 	"github.com/matrix-org/dendrite/clientapi/jsonerror"
 	"github.com/matrix-org/dendrite/clientapi/producers"
 	eduServerAPI "github.com/matrix-org/dendrite/eduserver/api"
@@ -659,8 +660,9 @@ func Setup(
 				SearchString string `json:"search_term"`
 				Limit        int    `json:"limit"`
 			}{}
-			if err := json.NewDecoder(req.Body).Decode(&postContent); err != nil {
-				return util.ErrorResponse(err)
+
+			if resErr := clientutil.UnmarshalJSONRequest(req, &postContent); resErr != nil {
+				return *resErr
 			}
 			return *SearchUserDirectory(
 				req.Context(),
diff --git a/sytest-whitelist b/sytest-whitelist
index 420acb22c..099fc6cbd 100644
--- a/sytest-whitelist
+++ b/sytest-whitelist
@@ -477,5 +477,6 @@ Inbound federation rejects invite rejections which include invalid JSON for room
 GET /capabilities is present and well formed for registered user 
 m.room.history_visibility == "joined" allows/forbids appropriately for Guest users
 m.room.history_visibility == "joined" allows/forbids appropriately for Real users
+POST rejects invalid utf-8 in JSON
 Users cannot kick users who have already left a room
 A prev_batch token from incremental sync can be used in the v1 messages API