From 46323d25beef3e16c92d6175a1cdd98de0e53b18 Mon Sep 17 00:00:00 2001
From: Jacob Hoffman-Andrews <jsha+github@letsencrypt.org>
Date: Mon, 21 Nov 2022 11:05:21 -0800
Subject: [PATCH] va: filter invalid UTF-8 from ProblemDetails (#6506)

This avoids serialization errors passing through gRPC.

Also, add a pass-through path in replaceInvalidUTF8 that saves an
allocation in the trivial case.

Fixes #6490
---
 va/dns.go             |  2 +-
 va/http.go            |  2 +-
 va/http_test.go       | 17 -----------------
 va/utf8filter.go      | 24 +++++++++++++++++++++++-
 va/utf8filter_test.go | 23 ++++++++++++++++++++++-
 va/va.go              | 11 ++++++++---
 6 files changed, 55 insertions(+), 24 deletions(-)

diff --git a/va/dns.go b/va/dns.go
index cabbf989f..fd8558c7a 100644
--- a/va/dns.go
+++ b/va/dns.go
@@ -89,5 +89,5 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident iden
 		andMore = fmt.Sprintf(" (and %d more)", len(txts)-1)
 	}
 	return nil, probs.Unauthorized(fmt.Sprintf("Incorrect TXT record %q%s found at %s",
-		replaceInvalidUTF8([]byte(invalidRecord)), andMore, challengeSubdomain))
+		invalidRecord, andMore, challengeSubdomain))
 }
diff --git a/va/http.go b/va/http.go
index d8ce0b331..2dfa2daec 100644
--- a/va/http.go
+++ b/va/http.go
@@ -643,7 +643,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation(
 	// resulting payload is the same size as maxResponseSize fail
 	if len(body) >= maxResponseSize {
 		return nil, records, newIPError(target, berrors.UnauthorizedError("Invalid response from %s: %q",
-			records[len(records)-1].URL, replaceInvalidUTF8(body)))
+			records[len(records)-1].URL, body))
 	}
 	return body, records, nil
 }
diff --git a/va/http_test.go b/va/http_test.go
index 808b43ee6..258b2fd05 100644
--- a/va/http_test.go
+++ b/va/http_test.go
@@ -1083,23 +1083,6 @@ func TestFetchHTTP(t *testing.T) {
 	}
 }
 
-func TestFetchHTTPInvalidUTF8(t *testing.T) {
-	testSrv := httpTestSrv(t)
-	defer testSrv.Close()
-	va, _ := setup(testSrv, 0, "", nil)
-	ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500)
-	defer cancel()
-	_, _, prob := va.fetchHTTP(ctx, "example.com", "/invalid-utf8-body")
-	expectedResult := "f\ufffdoo"
-	// If the body of the http response is larger than the maxResponseSize
-	// a truncated body is returned as part of the error detail. If the
-	// body contains invalid UTF-8 the invalid characters must be replaced
-	// before the error is marshalled for grpc. This tests that the
-	// invalid string "f\xffoo" is expected to be converted to
-	// "f\ufffdoo".
-	test.AssertContains(t, prob.Detail, expectedResult)
-}
-
 // All paths that get assigned to tokens MUST be valid tokens
 const pathWrongToken = "i6lNAC4lOOLYCl-A08VJt9z_tKYvVk63Dumo8icsBjQ"
 const path404 = "404"
diff --git a/va/utf8filter.go b/va/utf8filter.go
index 422442262..3d0f1ec8a 100644
--- a/va/utf8filter.go
+++ b/va/utf8filter.go
@@ -1,10 +1,19 @@
 package va
 
-import "strings"
+import (
+	"strings"
+	"unicode/utf8"
+
+	"github.com/letsencrypt/boulder/probs"
+)
 
 // replaceInvalidUTF8 replaces all invalid UTF-8 encodings with
 // Unicode REPLACEMENT CHARACTER.
 func replaceInvalidUTF8(input []byte) string {
+	if utf8.Valid(input) {
+		return string(input)
+	}
+
 	var b strings.Builder
 
 	// Ranging over a string in Go produces runes. When the range keyword
@@ -14,3 +23,16 @@ func replaceInvalidUTF8(input []byte) string {
 	}
 	return b.String()
 }
+
+// Call replaceInvalidUTF8 on all string fields of a ProblemDetails
+// and return the result.
+func filterProblemDetails(prob *probs.ProblemDetails) *probs.ProblemDetails {
+	if prob == nil {
+		return nil
+	}
+	return &probs.ProblemDetails{
+		Type:       probs.ProblemType(replaceInvalidUTF8([]byte(prob.Type))),
+		Detail:     replaceInvalidUTF8([]byte(prob.Detail)),
+		HTTPStatus: prob.HTTPStatus,
+	}
+}
diff --git a/va/utf8filter_test.go b/va/utf8filter_test.go
index 2443259a8..5c8cfff0e 100644
--- a/va/utf8filter_test.go
+++ b/va/utf8filter_test.go
@@ -1,6 +1,11 @@
 package va
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/letsencrypt/boulder/probs"
+	"github.com/letsencrypt/boulder/test"
+)
 
 func TestReplaceInvalidUTF8(t *testing.T) {
 	input := "f\xffoo"
@@ -10,3 +15,19 @@ func TestReplaceInvalidUTF8(t *testing.T) {
 		t.Errorf("replaceInvalidUTF8(%q): got %q, expected %q", input, result, expected)
 	}
 }
+
+func TestFilterProblemDetails(t *testing.T) {
+	test.Assert(t, filterProblemDetails(nil) == nil, "nil should filter to nil")
+	result := filterProblemDetails(&probs.ProblemDetails{
+		Type:       probs.ProblemType([]byte{0xff, 0xfe, 0xfd}),
+		Detail:     "seems okay so far whoah no \xFF\xFE\xFD",
+		HTTPStatus: 999,
+	})
+
+	expected := &probs.ProblemDetails{
+		Type:       "���",
+		Detail:     "seems okay so far whoah no ���",
+		HTTPStatus: 999,
+	}
+	test.AssertDeepEquals(t, result, expected)
+}
diff --git a/va/va.go b/va/va.go
index c8d9fc9fa..73d52a5a4 100644
--- a/va/va.go
+++ b/va/va.go
@@ -374,9 +374,14 @@ func (va *ValidationAuthorityImpl) validate(
 	}()
 
 	// TODO(#1292): send into another goroutine
-	validationRecords, err := va.validateChallenge(ctx, baseIdentifier, challenge)
-	if err != nil {
-		return validationRecords, err
+	validationRecords, prob := va.validateChallenge(ctx, baseIdentifier, challenge)
+	if prob != nil {
+		// The ProblemDetails will be serialized through gRPC, which requires UTF-8.
+		// It will also later be serialized in JSON, which defaults to UTF-8. Make
+		// sure it is UTF-8 clean now.
+		prob = filterProblemDetails(prob)
+
+		return validationRecords, prob
 	}
 
 	for i := 0; i < cap(ch); i++ {