From dad7dc252aa9628bd1cdd905b566e43d6b0af037 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Thu, 19 Dec 2024 12:27:19 -0800 Subject: [PATCH] Add more validation to integral/floating default values during descriptor processing. protoc already does these validations, but descriptor building should do them too. Checks added: - Verify that parsing the value consumes the whole string, even in the presence of NULL characters. - Verify that there are no overflows for integral types. PiperOrigin-RevId: 707992634 --- src/google/protobuf/descriptor.cc | 63 ++++-- src/google/protobuf/descriptor_unittest.cc | 216 +++++++++++++++++++++ 2 files changed, 265 insertions(+), 14 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 26581320e09d0..f63e133ddf492 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -6591,6 +6592,28 @@ void DescriptorBuilder::CheckFieldJsonNameUniqueness( } } +template +std::optional ParseIntegralDefault(const std::string& value, + T& out) { + static_assert(sizeof(int64_t) <= sizeof(long long)); // NOLINT + static_assert(sizeof(uint64_t) <= sizeof(unsigned long long)); // NOLINT + char* end_pos; + using Large = std::conditional_t, int64_t, uint64_t>; + errno = 0; + Large parsed = std::is_signed_v + ? std::strtoll(value.c_str(), &end_pos, 0) // NOLINT + : std::strtoull(value.c_str(), &end_pos, 0); // NOLINT + out = static_cast(parsed); + if (errno == ERANGE || parsed != out || + (value[0] == '-' && !std::is_signed_v)) { + return absl::StrCat("Value \"", value, "\" out of range."); + } + if (value.empty() || end_pos != &value[value.size()]) { + return absl::StrCat("Couldn't parse default value \"", value, "\"."); + } + return absl::nullopt; +} + void DescriptorBuilder::BuildFieldOrExtension(const FieldDescriptorProto& proto, Descriptor* parent, FieldDescriptor* result, @@ -6667,25 +6690,36 @@ void DescriptorBuilder::BuildFieldOrExtension(const FieldDescriptorProto& proto, char* end_pos = nullptr; switch (result->cpp_type()) { case FieldDescriptor::CPPTYPE_INT32: - result->default_value_int32_t_ = - std::strtol(proto.default_value().c_str(), &end_pos, 0); + if (auto error = ParseIntegralDefault( + proto.default_value(), result->default_value_int32_t_)) { + AddError(result->full_name(), proto, + DescriptorPool::ErrorCollector::DEFAULT_VALUE, + [&] { return *error; }); + } break; case FieldDescriptor::CPPTYPE_INT64: - static_assert(sizeof(int64_t) == sizeof(long long), // NOLINT - "sizeof int64_t is not sizeof long long"); - result->default_value_int64_t_ = - std::strtoll(proto.default_value().c_str(), &end_pos, 0); + if (auto error = ParseIntegralDefault( + proto.default_value(), result->default_value_int64_t_)) { + AddError(result->full_name(), proto, + DescriptorPool::ErrorCollector::DEFAULT_VALUE, + [&] { return *error; }); + } break; case FieldDescriptor::CPPTYPE_UINT32: - result->default_value_uint32_t_ = - std::strtoul(proto.default_value().c_str(), &end_pos, 0); + if (auto error = ParseIntegralDefault( + proto.default_value(), result->default_value_uint32_t_)) { + AddError(result->full_name(), proto, + DescriptorPool::ErrorCollector::DEFAULT_VALUE, + [&] { return *error; }); + } break; case FieldDescriptor::CPPTYPE_UINT64: - static_assert( - sizeof(uint64_t) == sizeof(unsigned long long), // NOLINT - "sizeof uint64_t is not sizeof unsigned long long"); - result->default_value_uint64_t_ = - std::strtoull(proto.default_value().c_str(), &end_pos, 0); + if (auto error = ParseIntegralDefault( + proto.default_value(), result->default_value_uint64_t_)) { + AddError(result->full_name(), proto, + DescriptorPool::ErrorCollector::DEFAULT_VALUE, + [&] { return *error; }); + } break; case FieldDescriptor::CPPTYPE_FLOAT: if (proto.default_value() == "inf") { @@ -6760,7 +6794,8 @@ void DescriptorBuilder::BuildFieldOrExtension(const FieldDescriptorProto& proto, // end_pos is only set non-null by the parsers for numeric types, // above. This checks that the default was non-empty and had no extra // junk after the end of the number. - if (proto.default_value().empty() || *end_pos != '\0') { + if (proto.default_value().empty() || + end_pos != &proto.default_value()[proto.default_value().size()]) { AddError(result->full_name(), proto, DescriptorPool::ErrorCollector::DEFAULT_VALUE, [&] { return absl::StrCat("Couldn't parse default value \"", diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index c15f3a9b6be79..7f175d75cc311 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -5079,6 +5079,222 @@ TEST_F(ValidationErrorTest, InvalidDefaults) { "values.\n"); } +static constexpr absl::string_view kNullChar("\0", 1); + +TEST_F(ValidationErrorTest, InvalidDefaultIntegerValues) { + // A null char in the middle + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_INT64 + default_value: "1\0" + } + } + )pb", + absl::StrCat( + "foo.proto: bar.foo: DEFAULT_VALUE: Couldn't parse default value \"1", + kNullChar, "\".\n")); + + // Other chars after + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_INT64 + default_value: "1a" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Couldn't parse default value " + "\"1a\".\n"); + + // Overflow of int32_t + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_INT32 + default_value: "3000000000" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Value \"3000000000\" out of " + "range.\n"); + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_INT32 + default_value: "-3000000000" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Value \"-3000000000\" out of " + "range.\n"); + + // Overflow of uint32_t + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_UINT32 + default_value: "5000000000" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Value \"5000000000\" out of " + "range.\n"); + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_UINT32 + default_value: "-1" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Value \"-1\" out of range.\n"); + + // Overflow of int64_t + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_INT64 + default_value: "9999999999999999999" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Value \"9999999999999999999\" out of " + "range.\n"); + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_INT64 + default_value: "-9999999999999999999" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Value \"-9999999999999999999\" out " + "of range.\n"); + + // Overflow of uint64_t + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_UINT64 + default_value: "19999999999999999999" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Value \"19999999999999999999\" out " + "of " + "range.\n"); + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_UINT64 + default_value: "-1" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Value \"-1\" out " + "of range.\n"); +} + +TEST_F(ValidationErrorTest, InvalidDefaultFloatingValues) { + // A null char in the middle + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_FLOAT + default_value: "1\0" + } + } + )pb", + absl::StrCat( + "foo.proto: bar.foo: DEFAULT_VALUE: Couldn't parse default value \"1", + kNullChar, "\".\n")); + + // Other chars after + BuildFileWithErrors( + R"pb( + name: "foo.proto" + message_type { + name: "bar" + field { + name: "foo" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_DOUBLE + default_value: "1j" + } + } + )pb", + "foo.proto: bar.foo: DEFAULT_VALUE: Couldn't parse default value " + "\"1j\".\n"); +} + TEST_F(ValidationErrorTest, NegativeFieldNumber) { BuildFileWithErrors( "name: \"foo.proto\" "