feat: check validity of trace- and span-id in context update from hea…

…der (#1046) * Added draft test scenario for invalid trace_id * test: invalid span id from header test cases * feat: trace- and span-id checks in context update from header * chore: format code * fix: free memory of string on invalid input * test: add empty span-id test * fix: free memory of parent_span_id on invalid input * chore: update CHANGELOG.md * chore: add warning on invalid input * test: use correct string to compare expected span_id with * Apply suggestions from code review * chore: format updated code * chore: update CHANGELOG.md
getsentry · Oct 7, 2024 · 65bfb62 · 65bfb62
1 parent dbb9580
commit 65bfb62
Show file tree

Hide file tree

Showing 4 changed files with 182 additions and 0 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+**Fixes**:
+
+- Reject invalid trace- and span-ids in context update from header ([#1046](https://github.com/getsentry/sentry-native/pull/1046))
+
 ## 0.7.10
 
 **Fixes**:

diff --git a/src/sentry_tracing.c b/src/sentry_tracing.c
@@ -151,6 +151,50 @@ sentry_transaction_context_remove_sampled(sentry_transaction_context_t *tx_cxt)
     }
 }
 
+/*
+ * Checks whether the string is a valid hex string over the given length and
+ * contains at least one non-zero character.
+ */
+static bool
+is_valid_nonzero_hexstring(const char *s, size_t len)
+{
+    bool has_nonzero = false;
+    for (size_t i = 0; i < len; i++) {
+        if (!isxdigit(s[i])) {
+            return false;
+        }
+        if (s[i] != '0') {
+            has_nonzero = true;
+        }
+    }
+    return has_nonzero;
+}
+
+static bool
+is_valid_id(const char *id, size_t expected_len, const char *id_type)
+{
+    const bool is_valid = id != NULL && strlen(id) == expected_len
+        && is_valid_nonzero_hexstring(id, expected_len);
+
+    if (!is_valid) {
+        SENTRY_WARNF("invalid %s format in given header", id_type);
+    }
+
+    return is_valid;
+}
+
+static bool
+is_valid_trace_id(const char *trace_id)
+{
+    return is_valid_id(trace_id, 32, "trace id");
+}
+
+static bool
+is_valid_span_id(const char *span_id)
+{
+    return is_valid_id(span_id, 16, "span id");
+}
+
 void
 sentry_transaction_context_update_from_header_n(
     sentry_transaction_context_t *tx_cxt, const char *key, size_t key_len,
@@ -177,13 +221,18 @@ sentry_transaction_context_update_from_header_n(
     const char *trace_id_start = value;
     const char *trace_id_end = memchr(trace_id_start, '-', value_len);
     if (!trace_id_end) {
+        SENTRY_WARN("invalid trace id format in given header");
         return;
     }
 
     sentry_value_t inner = tx_cxt->inner;
 
     char *s
         = sentry__string_clone_n(trace_id_start, trace_id_end - trace_id_start);
+    if (!is_valid_trace_id(s)) {
+        sentry_free(s);
+        return;
+    }
     sentry_value_t trace_id = sentry__value_new_string_owned(s);
     sentry_value_set_by_key(inner, "trace_id", trace_id);
 
@@ -192,12 +241,20 @@ sentry_transaction_context_update_from_header_n(
     if (!span_id_end) {
         // no sampled flag
         sentry_value_t parent_span_id = sentry_value_new_string(span_id_start);
+        if (!is_valid_span_id(sentry_value_as_string(parent_span_id))) {
+            sentry_value_decref(parent_span_id);
+            return;
+        }
         sentry_value_set_by_key(inner, "parent_span_id", parent_span_id);
         return;
     }
     // else: we have a sampled flag
 
     s = sentry__string_clone_n(span_id_start, span_id_end - span_id_start);
+    if (!is_valid_span_id(s)) {
+        sentry_free(s);
+        return;
+    }
     sentry_value_t parent_span_id = sentry__value_new_string_owned(s);
     sentry_value_set_by_key(inner, "parent_span_id", parent_span_id);
 

diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c
@@ -748,6 +748,123 @@ SENTRY_TEST(update_from_header_no_sampled_flag)
     sentry_close();
 }
 
+SENTRY_TEST(distributed_headers_invalid_traceid)
+{
+    sentry_options_t *options = sentry_options_new();
+    sentry_options_set_dsn(options, "https://foo@sentry.invalid/42");
+
+    sentry_init(options);
+
+    sentry_transaction_context_t *tx_ctx
+        = sentry_transaction_context_new("sanity_check", NULL);
+
+    const char *valid_trace_header
+        = "2674eb52d5874b13b560236d6c79ce8a-a0f9fdf04f1a63df-1";
+    // expected should match the valid trace_id from the header
+    const char *expected_trace_id = "2674eb52d5874b13b560236d6c79ce8a";
+
+    // sanity check test case
+    sentry_transaction_context_update_from_header(
+        tx_ctx, "sentry-trace", valid_trace_header);
+    const char *valid_trace_id = sentry_value_as_string(
+        sentry_value_get_by_key(tx_ctx->inner, "trace_id"));
+    TEST_CHECK_STRING_EQUAL(valid_trace_id, expected_trace_id);
+
+    // case 1: string with two dashes (nothing inbetween)
+    const char *trace_header = "--";
+    sentry_transaction_context_update_from_header(
+        tx_ctx, "sentry-trace", trace_header);
+    const char *new_trace_id = sentry_value_as_string(
+        sentry_value_get_by_key(tx_ctx->inner, "trace_id"));
+    // expect to have the trace_id remain unchanged
+    TEST_CHECK_STRING_EQUAL(new_trace_id, expected_trace_id);
+
+    // case 2: string with two dashes (trace_id too short)
+    const char *trace_header_short = "2-a0f9fdf04f1a63df-1";
+    sentry_transaction_context_update_from_header(
+        tx_ctx, "sentry-trace", trace_header_short);
+    const char *new_trace_id_short = sentry_value_as_string(
+        sentry_value_get_by_key(tx_ctx->inner, "trace_id"));
+    // expect to have the trace_id remain unchanged
+    TEST_CHECK_STRING_EQUAL(new_trace_id_short, expected_trace_id);
+
+    // case 3: string with two dashes (trace_id too long)
+    const char *trace_header_long = "2674eb52d5874b13b560236d6c79ce8a2674eb52d5"
+                                    "874b13b560236d6c79ce8a-a0f9fdf04f1a63df-1";
+    sentry_transaction_context_update_from_header(
+        tx_ctx, "sentry-trace", trace_header_long);
+    const char *new_trace_id_long = sentry_value_as_string(
+        sentry_value_get_by_key(tx_ctx->inner, "trace_id"));
+    // expect to have the trace_id remain unchanged
+    TEST_CHECK_STRING_EQUAL(new_trace_id_long, expected_trace_id);
+
+    sentry__transaction_context_free(tx_ctx);
+    sentry_close();
+}
+
+SENTRY_TEST(distributed_headers_invalid_spanid)
+{
+    sentry_options_t *options = sentry_options_new();
+    sentry_options_set_dsn(options, "https://foo@sentry.invalid/42");
+
+    sentry_init(options);
+
+    sentry_transaction_context_t *tx_ctx
+        = sentry_transaction_context_new("wow!", NULL);
+
+    const char *valid_trace_header
+        = "2674eb52d5874b13b560236d6c79ce8a-a0f9fdf04f1a63df-1";
+    // expected should match the valid parent_span_id from the header
+    const char *expected_parent_span_id = "a0f9fdf04f1a63df";
+
+    // sanity check test case
+    sentry_transaction_context_update_from_header(
+        tx_ctx, "sentry-trace", valid_trace_header);
+    const char *valid_parent_span_id = sentry_value_as_string(
+        sentry_value_get_by_key(tx_ctx->inner, "parent_span_id"));
+    TEST_CHECK_STRING_EQUAL(valid_parent_span_id, expected_parent_span_id);
+
+    // case 1: string with two dashes (nothing inbetween)
+    const char *trace_header = "--";
+    sentry_transaction_context_update_from_header(
+        tx_ctx, "sentry-trace", trace_header);
+    const char *new_parent_span_id = sentry_value_as_string(
+        sentry_value_get_by_key(tx_ctx->inner, "parent_span_id"));
+    // expect to have the parent_span_id remain unchanged
+    TEST_CHECK_STRING_EQUAL(new_parent_span_id, expected_parent_span_id);
+
+    // case 2: string with two dashes (parent_span_id too short)
+    const char *trace_header_short = "2674eb52d5874b13b560236d6c79ce8a-a-1";
+    sentry_transaction_context_update_from_header(
+        tx_ctx, "sentry-trace", trace_header_short);
+    const char *new_parent_span_id_short = sentry_value_as_string(
+        sentry_value_get_by_key(tx_ctx->inner, "parent_span_id"));
+    // expect to have the parent_span_id remain unchanged
+    TEST_CHECK_STRING_EQUAL(new_parent_span_id_short, expected_parent_span_id);
+
+    // case 3: string with two dashes (parent_span_id too long)
+    const char *trace_header_long
+        = "2674eb52d5874b13b560236d6c79ce8a-a0f9fdf04f1a63dfa0f9fdf04f1a63df-1";
+    sentry_transaction_context_update_from_header(
+        tx_ctx, "sentry-trace", trace_header_long);
+    const char *new_parent_span_id_long = sentry_value_as_string(
+        sentry_value_get_by_key(tx_ctx->inner, "parent_span_id"));
+    // expect to have the parent_span_id remain unchanged
+    TEST_CHECK_STRING_EQUAL(new_parent_span_id_long, expected_parent_span_id);
+
+    // case 4: string with one dash (span_id empty)
+    const char *trace_header_empty_span = "2674eb52d5874b13b560236d6c79ce8a-";
+    sentry_transaction_context_update_from_header(
+        tx_ctx, "sentry-trace", trace_header_empty_span);
+    const char *new_parent_span_id_empty = sentry_value_as_string(
+        sentry_value_get_by_key(tx_ctx->inner, "parent_span_id"));
+    // expect to have the parent_span_id remain unchanged
+    TEST_CHECK_STRING_EQUAL(new_parent_span_id_empty, expected_parent_span_id);
+
+    sentry__transaction_context_free(tx_ctx);
+    sentry_close();
+}
+
 SENTRY_TEST(distributed_headers)
 {
     sentry_options_t *options = sentry_options_new();

diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc
@@ -27,6 +27,8 @@ XX(crashed_last_run)
 XX(custom_logger)
 XX(discarding_before_send)
 XX(distributed_headers)
+XX(distributed_headers_invalid_spanid)
+XX(distributed_headers_invalid_traceid)
 XX(drop_unfinished_spans)
 XX(dsn_auth_header_custom_user_agent)
 XX(dsn_auth_header_invalid_dsn)