From 5b24e3daaddc238614cfbe71d25d2b2ebd51a5dd Mon Sep 17 00:00:00 2001
From: Zach Leslie <88330524+zalegrala@users.noreply.github.com>
Date: Thu, 5 Aug 2021 08:48:09 -0600
Subject: [PATCH] Allow only valid trace ID characters when decoding (#854)

Without this change, invalid characters received from the user input causes a
bad error message to the user.  Here we check that all of the characters that
are to be decoded are valid hex characters and improve the error to the user in
the event the trace ID submitted has invalid characters.
---
 CHANGELOG.md          |  1 +
 pkg/util/http.go      | 15 ++++++++++++++-
 pkg/util/http_test.go | 14 ++++++++++----
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 19a2e326738..43d352174aa 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,6 @@
 ## main / unreleased
 
+* [BUGFIX] Allow only valid trace ID characters when decoding [#854](https://github.com/grafana/tempo/pull/854) (@zalegrala)
 * [CHANGE] Upgrade Cortex from v1.9.0 to v1.9.0-131-ga4bf10354 [#841](https://github.com/grafana/tempo/pull/841) (@aknuds1)
 * [CHANGE] Change default tempo port from 3100 to 3200 [#770](https://github.com/grafana/tempo/pull/809) (@MurzNN)
 * [ENHANCEMENT] Added hedged request metric `tempodb_backend_hedged_roundtrips_total` and a new storage agnostic `tempodb_backend_request_duration_seconds` metric that
diff --git a/pkg/util/http.go b/pkg/util/http.go
index 5d8d041430c..a342b1bef93 100644
--- a/pkg/util/http.go
+++ b/pkg/util/http.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/gorilla/mux"
 )
@@ -32,6 +33,18 @@ func ParseTraceID(r *http.Request) ([]byte, error) {
 }
 
 func hexStringToTraceID(id string) ([]byte, error) {
+	// The encoding/hex package does not handle non-hex characters.
+	// Ensure the ID has only the proper characters
+	for pos, idChar := range strings.Split(id, "") {
+		if (idChar >= "a" && idChar <= "f") ||
+			(idChar >= "A" && idChar <= "F") ||
+			(idChar >= "0" && idChar <= "9") {
+			continue
+		} else {
+			return nil, fmt.Errorf("trace IDs can only contain hex characters: invalid character '%s' at position %d", idChar, pos+1)
+		}
+	}
+
 	// the encoding/hex package does not like odd length strings.
 	// just append a bit here
 	if len(id)%2 == 1 {
@@ -45,7 +58,7 @@ func hexStringToTraceID(id string) ([]byte, error) {
 
 	size := len(byteID)
 	if size > 16 {
-		return nil, errors.New("trace ids can't be larger than 128 bits")
+		return nil, errors.New("trace IDs can't be larger than 128 bits")
 	}
 	if size < 16 {
 		byteID = append(make([]byte, 16-size), byteID...)
diff --git a/pkg/util/http_test.go b/pkg/util/http_test.go
index 7d5b871e859..ba21e294a95 100644
--- a/pkg/util/http_test.go
+++ b/pkg/util/http_test.go
@@ -1,6 +1,7 @@
 package util
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/grafana/tempo/cmd/tempo-query/tempo"
@@ -13,7 +14,7 @@ func TestHexStringToTraceID(t *testing.T) {
 	tc := []struct {
 		id          string
 		expected    []byte
-		expectError bool
+		expectError error
 	}{
 		{
 			id:       "12",
@@ -30,20 +31,25 @@ func TestHexStringToTraceID(t *testing.T) {
 		{
 			id:          "121234567890abcdef1234567890abcdef", // value too long
 			expected:    []byte{0x12, 0x34, 0x56, 0x78, 0x90, 0xab, 0xcd, 0xef, 0x12, 0x34, 0x56, 0x78, 0x90, 0xab, 0xcd, 0xef},
-			expectError: true,
+			expectError: errors.New("trace IDs can't be larger than 128 bits"),
 		},
 		{
 			id:       "234567890abcdef", // odd length
 			expected: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x34, 0x56, 0x78, 0x90, 0xab, 0xcd, 0xef},
 		},
+		{
+			id:          "1234567890abcdef ", // trailing space
+			expected:    nil,
+			expectError: errors.New("trace IDs can only contain hex characters: invalid character ' ' at position 17"),
+		},
 	}
 
 	for _, tt := range tc {
 		t.Run(tt.id, func(t *testing.T) {
 			actual, err := hexStringToTraceID(tt.id)
 
-			if tt.expectError {
-				assert.Error(t, err)
+			if tt.expectError != nil {
+				assert.Equal(t, tt.expectError, err)
 				assert.Nil(t, actual)
 				return
 			}