diff --git a/CHANGELOG.md b/CHANGELOG.md index 96bed61b4..6462f03ed 100644 --- 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 index 4d6195a0d..2a63511ce 100644 --- 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,6 +221,7 @@ 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; } @@ -184,6 +229,10 @@ sentry_transaction_context_update_from_header_n( 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 index bbc23107c..5d86ee48a 100644 --- 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 index cc7920bc1..97f032fca 100644 --- 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)