Allow only valid trace ID characters when decoding

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

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...)

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
 			}