Merge commit from fork

* Fix baggage metadata injection and reject unsafe curl headers

* Avoid noexcept substring warnings and clarify security release notes

* Preserve existing baggage when Set receives invalid metadata

* Reuse baggage tokenizer while preserving whitespace compatibility

* Add direct test includes required by IWYU

Author: lalitb

CHANGELOG.md

@@ -15,6 +15,18 @@ Increment the:
 
 ## [Unreleased]
 
+* [SECURITY] Reject invalid baggage metadata during extraction and injection.
+  After the existing trimming of leading and trailing whitespace, members
+  containing raw control bytes other than permitted HTAB are dropped.
+  Members with empty metadata properties (`;;` or a trailing `;`), non-token
+  property keys, or non-ASCII metadata are also dropped. `Baggage::Set()` rejects
+  invalid metadata without replacing existing entries or adding new ones.
+  The curl client now fails the whole request for non-token header names or
+  header values containing control bytes other than HTAB, or DEL. This
+  includes configured exporter headers. Applications must recompile to
+  receive the header-only baggage fix.
+  [GHSA-j5cq-83xw-rx48](https://github.com/open-telemetry/opentelemetry-cpp/security/advisories/GHSA-j5cq-83xw-rx48)
+
 * [API/SDK] Add ASCII helpers and remove regex from InstrumentMetaDataValidator
   [#4715](https://github.com/open-telemetry/opentelemetry-cpp/pull/4715)
 

api/include/opentelemetry/baggage/baggage.h

@@ -55,7 +55,7 @@ class OPENTELEMETRY_EXPORT Baggage
   {
 
     nostd::shared_ptr<Baggage> baggage(new Baggage(kv_properties_->Size() + 1));
-    const bool valid_kv = IsValidKey(key) && IsValidValue(value);
+    const bool valid_kv = IsValidKey(key) && IsValidValue(value) && IsValidValueMetadata(value);
 
     if (valid_kv)
     {
@@ -125,9 +125,14 @@ class OPENTELEMETRY_EXPORT Baggage
 
     while (kv_str_tokenizer.next(kv_valid, key, value) && baggage->kv_properties_->Size() < cnt)
     {
-      if (!kv_valid || (key.size() + value.size() > kMaxKeyValueSize))
+      if (!kv_valid || key.size() + value.size() > kMaxKeyValueSize)
+      {
+        continue;
+      }
+      // Preserve the tokenizer's edge-whitespace normalization, but reject
+      // unsafe bytes before decoding or separating metadata from the value.
+      if (!IsValidWireString(key) || !IsValidWireString(value))
       {
-        // if kv pair is not valid, skip it
         continue;
       }
 
@@ -145,7 +150,8 @@ class OPENTELEMETRY_EXPORT Baggage
       auto key_str   = UrlDecode(common::StringUtil::Trim(key), err);
       auto value_str = UrlDecode(common::StringUtil::Trim(value), err);
 
-      if (err == false && IsValidKey(key_str) && IsValidValue(value_str))
+      if (err == false && IsValidKey(key_str) && IsValidValue(value_str) &&
+          IsValidMetadata(metadata))
       {
         if (!metadata.empty())
         {
@@ -164,6 +170,12 @@ class OPENTELEMETRY_EXPORT Baggage
     std::string header_s;
     bool first = true;
     kv_properties_->GetAllEntries([&](nostd::string_view key, nostd::string_view value) {
+      // Constructors can populate baggage without going through FromHeader or Set.
+      auto metadata_separator = value.find(kMetadataSeparator);
+      if (!IsValidValueMetadata(value))
+      {
+        return true;
+      }
       if (!first)
       {
         header_s.push_back(kMembersSeparator);
@@ -176,11 +188,10 @@ class OPENTELEMETRY_EXPORT Baggage
       header_s.push_back(kKeyValueSeparator);
 
       // extracting metadata from value. We do not encode metadata
-      auto metadata_separator = value.find(kMetadataSeparator);
       if (metadata_separator != std::string::npos)
       {
-        header_s.append(UrlEncode(value.substr(0, metadata_separator)));
-        auto metadata = value.substr(metadata_separator);
+        header_s.append(UrlEncode(common::StringUtil::Substr(value, 0, metadata_separator)));
+        auto metadata = common::StringUtil::Substr(value, metadata_separator);
         header_s.append(std::string(metadata.data(), metadata.size()));
       }
       else
@@ -193,6 +204,93 @@ class OPENTELEMETRY_EXPORT Baggage
   }
 
 private:
+  static bool IsValidValueMetadata(nostd::string_view value) noexcept
+  {
+    auto separator = value.find(kMetadataSeparator);
+    return separator == nostd::string_view::npos ||
+           IsValidMetadata(common::StringUtil::Substr(value, separator));
+  }
+
+  static nostd::string_view TrimOws(nostd::string_view value) noexcept
+  {
+    while (!value.empty() && (value[0] == ' ' || value[0] == '\t'))
+    {
+      value = common::StringUtil::Substr(value, 1);
+    }
+    while (!value.empty() && (value[value.size() - 1] == ' ' || value[value.size() - 1] == '\t'))
+    {
+      value = common::StringUtil::Substr(value, 0, value.size() - 1);
+    }
+    return value;
+  }
+
+  static bool IsValidWireString(nostd::string_view value) noexcept
+  {
+    for (unsigned char ch : value)
+    {
+      if ((ch < 0x20 && ch != '\t') || ch > 0x7e)
+      {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  static bool IsToken(nostd::string_view value) noexcept
+  {
+    if (value.empty())
+    {
+      return false;
+    }
+    for (char ch : value)
+    {
+      if (!((ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') ||
+            nostd::string_view("!#$%&'*+-.^_`|~").find(ch) != nostd::string_view::npos))
+      {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  // Metadata is stored in wire form. Validate properties without decoding or
+  // re-encoding them, so existing percent escapes retain their meaning.
+  static bool IsValidMetadata(nostd::string_view metadata) noexcept
+  {
+    while (!metadata.empty())
+    {
+      if (metadata[0] != kMetadataSeparator)
+      {
+        return false;
+      }
+      metadata      = common::StringUtil::Substr(metadata, 1);
+      auto end      = metadata.find(kMetadataSeparator);
+      auto property = TrimOws(common::StringUtil::Substr(metadata, 0, end));
+      auto equals   = property.find(kKeyValueSeparator);
+      if (!IsToken(TrimOws(common::StringUtil::Substr(property, 0, equals))))
+      {
+        return false;
+      }
+      if (equals != nostd::string_view::npos)
+      {
+        for (unsigned char ch : TrimOws(common::StringUtil::Substr(property, equals + 1)))
+        {
+          // W3C baggage-octet; SP and HTAB are only allowed around properties.
+          if (ch < 0x21 || ch > 0x7e || ch == '"' || ch == ',' || ch == '\\')
+          {
+            return false;
+          }
+        }
+      }
+      if (end == nostd::string_view::npos)
+      {
+        return true;
+      }
+      metadata = common::StringUtil::Substr(metadata, end);
+    }
+    return true;
+  }
+
   static bool IsPrintableString(nostd::string_view str)
   {
     for (const auto ch : str)

api/test/baggage/baggage_test.cc

@@ -4,7 +4,9 @@
 #include <gtest/gtest.h>
 #include <cassert>
 #include <cstddef>
+#include <initializer_list>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "opentelemetry/baggage/baggage.h"
@@ -15,6 +17,135 @@
 
 using namespace opentelemetry::baggage;
 
+TEST(BaggageTest, RejectInvalidMetadataOnExtractAndInject)
+{
+  const std::vector<std::string> invalid = {
+      ";a\r\nX-Injected: 1",
+      ";a\r\n\r\nGET /smuggled HTTP/1.1\r\nHost: localhost\r\nX-Smuggled: 1",
+      ";a\rb",
+      ";a\nb",
+      std::string(";a\0b", 4),
+      ";a=\x7f",
+      ";a=\xc3\xa9",
+      ";",
+      ";a;;b",
+      ";=value",
+      ";bad key=value",
+      ";a=bad value",
+      ";a=bad\tvalue",
+      ";a=\"quoted\"",
+      ";a=back\\slash"};
+  for (const auto &metadata : invalid)
+  {
+    SCOPED_TRACE(metadata);
+    auto baggage = Baggage::FromHeader("before=ok,bad=value" + metadata + ",after=ok");
+    std::string value;
+    EXPECT_FALSE(baggage->GetValue("bad", value));
+    EXPECT_TRUE(baggage->GetValue("before", value));
+    EXPECT_TRUE(baggage->GetValue("after", value));
+    EXPECT_EQ(baggage->ToHeader(), "before=ok,after=ok");
+
+    // Direct construction does not validate input. Output must still be safe.
+    const std::vector<std::pair<std::string, std::string>> entries = {
+        {"before", "ok"}, {"bad", "value" + metadata}, {"after", "ok"}};
+    // KeyValueProperties exposes NUL-terminated strings, so direct construction
+    // already truncates at an embedded NUL. Extraction must reject it above.
+    if (metadata.find('\0') == std::string::npos)
+    {
+      EXPECT_EQ(Baggage(entries).ToHeader(), "before=ok,after=ok");
+    }
+    EXPECT_EQ(Baggage().Set("bad", "value" + metadata)->ToHeader(), "");
+  }
+}
+
+TEST(BaggageTest, RejectMemberInjectionFromSet)
+{
+  auto baggage = Baggage::FromHeader("good=ok")->Set("k", "v;a,evil=1");
+  EXPECT_EQ(baggage->ToHeader(), "good=ok");
+}
+
+TEST(BaggageTest, SetWithInvalidMetadataPreservesExistingEntry)
+{
+  auto original = Baggage::FromHeader("k=good;flag,other=kept");
+  for (const auto *invalid : {"v;a,evil=1", "v;", "v;;x", "v;p:x", "v;p=bad value"})
+  {
+    SCOPED_TRACE(invalid);
+    auto updated = original->Set("k", invalid);
+    std::string value;
+    ASSERT_TRUE(updated->GetValue("k", value));
+    EXPECT_EQ(value, "good;flag");
+    EXPECT_EQ(updated->ToHeader(), "k=good;flag,other=kept");
+    EXPECT_EQ(original->ToHeader(), "k=good;flag,other=kept");
+
+    auto inserted = original->Set("new", invalid);
+    EXPECT_FALSE(inserted->GetValue("new", value));
+    EXPECT_EQ(inserted->ToHeader(), "k=good;flag,other=kept");
+  }
+
+  // Valid metadata must still replace the existing value.
+  auto updated = original->Set("k", "replacement;p=100%");
+  std::string value;
+  ASSERT_TRUE(updated->GetValue("k", value));
+  EXPECT_EQ(value, "replacement;p=100%");
+  EXPECT_EQ(updated->ToHeader(), "k=replacement;p=100%,other=kept");
+}
+
+TEST(BaggageTest, RejectMemberInjectionFromDecodedDelimiters)
+{
+  // Decoding the value introduces a semicolon that output treats as metadata.
+  auto baggage = Baggage::FromHeader("k=a%3B%2Cx%3Dy,good=ok");
+  EXPECT_EQ(baggage->ToHeader(), "good=ok");
+}
+
+TEST(BaggageTest, PreserveValidMetadataWireRepresentation)
+{
+  for (const std::string metadata :
+       {";flag", ";empty=", ";a=b=c", ";p=100%", ";a=%20%0D%0A%25", "; flag \t; key \t=\t value",
+        ";a=!#$&'()*+-./:<=>?@[]^_`{|}~"})
+  {
+    SCOPED_TRACE(metadata);
+    const auto header = "key=value" + metadata;
+    EXPECT_EQ(Baggage::FromHeader(header)->ToHeader(), header);
+    const std::vector<std::pair<std::string, std::string>> entries = {{"key", "value" + metadata}};
+    EXPECT_EQ(Baggage(entries).ToHeader(), header);
+  }
+}
+
+TEST(BaggageTest, RejectUnsafeBytesAfterTokenization)
+{
+  for (int byte = 0; byte <= 255; ++byte)
+  {
+    if ((byte >= 0x20 && byte <= 0x7e) || byte == '\t')
+    {
+      continue;
+    }
+    SCOPED_TRACE(byte);
+    const std::string control(1, static_cast<char>(byte));
+    // Interior bytes must be checked in both the key and the complete value.
+    for (const auto &member :
+         {"bad" + control + "key=value;flag", "bad=value;flag=" + control + "value"})
+    {
+      EXPECT_EQ(Baggage::FromHeader(member + ",good=ok")->ToHeader(), "good=ok");
+    }
+    // The shared tokenizer has always normalized these whitespace bytes at edges.
+    const bool trimmed = byte >= 0x0a && byte <= 0x0d;
+    for (const auto &member : {control + "bad=value;flag", "bad=value;flag" + control})
+    {
+      EXPECT_EQ(Baggage::FromHeader(member + ",good=ok")->ToHeader(),
+                trimmed ? "bad=value;flag,good=ok" : "good=ok");
+    }
+  }
+}
+
+TEST(BaggageTest, PreserveTokenizerEdgeWhitespaceNormalization)
+{
+  EXPECT_EQ(Baggage::FromHeader(" \t\r\n\v\fk=v;flag\r\n\v\f \t,other=ok")->ToHeader(),
+            "k=v;flag,other=ok");
+  // Directly constructed metadata is not tokenizer-normalized and must remain rejected.
+  const std::vector<std::pair<std::string, std::string>> entries = {{"k", "v;flag\r\n"}};
+  EXPECT_EQ(Baggage(entries).ToHeader(), "");
+}
+
 static std::string header_with_custom_entries(size_t num_entries)
 {
   std::string header;
@@ -127,8 +258,8 @@ TEST(BaggageTest, ValidateInjectHeader)
                     {"bar%1", "bar 4"},
                     "a%2A%2Ffoo-_%2Fbar=bar+4,1a-2f%40foo=bar%251"},  // encoding is done properly
                    {{"foo 1"},
-                    {"bar 1;  metadata ; ;;"},
-                    "foo+1=bar+1;  metadata ; ;;"}};  // metadata is added without encoding
+                    {"bar 1;  metadata ; flag=encoded%20value"},
+                    "foo+1=bar+1;  metadata ; flag=encoded%20value"}};
 
   for (auto &testcase : testcases)
   {

api/test/baggage/propagation/baggage_propagator_test.cc

@@ -47,6 +47,21 @@ class BaggageCarrierTest : public context::propagation::TextMapCarrier
 
 static BaggagePropagator format;
 
+TEST(BaggagePropagatorTest, DoesNotPropagateHeaderInjection)
+{
+  for (const auto *payload : {"bad=value;a\r\nX-Injected: 1",
+                              "bad=value;a\r\n\r\nGET /smuggled HTTP/1.1\r\nHost: localhost"})
+  {
+    BaggageCarrierTest inbound;
+    inbound.Set("baggage", std::string(payload) + ",good=ok;flag");
+    context::Context base;
+    auto extracted = format.Extract(inbound, base);
+    BaggageCarrierTest outbound;
+    format.Inject(outbound, extracted);
+    EXPECT_EQ(outbound.Get("baggage"), "good=ok;flag");
+  }
+}
+
 TEST(BaggagePropagatorTest, ExtractNoBaggageHeader)
 {
   BaggageCarrierTest carrier;

api/test/common/kv_properties_test.cc

@@ -73,6 +73,23 @@ TEST(EntryTest, SetValue)
 using opentelemetry::common::KeyValueStringTokenizer;
 using opentelemetry::common::KeyValueStringTokenizerOptions;
 
+TEST(KVStringTokenizer, PreserveExistingWhitespaceNormalization)
+{
+  // OTLP headers and resource attributes also rely on member-edge trimming.
+  KeyValueStringTokenizer tk(" \t\r\n\v\fAuthorization=token\r\n,resource=value\v\f");
+  bool valid = false;
+  nostd::string_view key, value;
+  ASSERT_TRUE(tk.next(valid, key, value));
+  EXPECT_TRUE(valid);
+  EXPECT_EQ(key, "Authorization");
+  EXPECT_EQ(value, "token");
+  ASSERT_TRUE(tk.next(valid, key, value));
+  EXPECT_TRUE(valid);
+  EXPECT_EQ(key, "resource");
+  EXPECT_EQ(value, "value");
+  EXPECT_FALSE(tk.next(valid, key, value));
+}
+
 TEST(KVStringTokenizer, SinglePair)
 {
   bool valid_kv{};