From aeb0b05242de29eec76789d9079599f805a36b42 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 6 Jan 2025 14:19:41 -0500 Subject: [PATCH 1/7] string_format.h: fix use-after-free bug due to insufficient lifetime extension of the StringifySink object --- Firestore/core/src/util/string_format.h | 39 ++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/Firestore/core/src/util/string_format.h b/Firestore/core/src/util/string_format.h index 2322dba0138..92676c3c7c8 100644 --- a/Firestore/core/src/util/string_format.h +++ b/Firestore/core/src/util/string_format.h @@ -65,8 +65,11 @@ struct FormatChoice<5> {}; class FormatArg : public absl::AlphaNum { public: template - FormatArg(T&& value) // NOLINT(runtime/explicit) - : FormatArg{std::forward(value), internal::FormatChoice<0>{}} { + FormatArg(T&& value, + absl::strings_internal::StringifySink&& sink = + {}) // NOLINT(runtime/explicit) + : FormatArg{std::forward(value), std::move(sink), + internal::FormatChoice<0>{}} { } private: @@ -79,7 +82,9 @@ class FormatArg : public absl::AlphaNum { */ template {}>::type> - FormatArg(T bool_value, internal::FormatChoice<0>) + FormatArg(T bool_value, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<0>) : AlphaNum(bool_value ? "true" : "false") { } @@ -90,7 +95,9 @@ class FormatArg : public absl::AlphaNum { template < typename T, typename = typename std::enable_if{}>::type> - FormatArg(T object, internal::FormatChoice<1>) + FormatArg(T object, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<1>) : AlphaNum(MakeStringView([object description])) { } @@ -98,7 +105,9 @@ class FormatArg : public absl::AlphaNum { * Creates a FormatArg from any Objective-C Class type. Objective-C Class * types are a special struct that aren't of a type derived from NSObject. */ - FormatArg(Class object, internal::FormatChoice<1>) + FormatArg(Class object, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<1>) : AlphaNum(MakeStringView(NSStringFromClass(object))) { } #endif @@ -108,7 +117,10 @@ class FormatArg : public absl::AlphaNum { * handled specially to avoid ambiguity with generic pointers, which are * handled differently. */ - FormatArg(std::nullptr_t, internal::FormatChoice<2>) : AlphaNum("null") { + FormatArg(std::nullptr_t, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<2>) + : AlphaNum("null") { } /** @@ -116,7 +128,9 @@ class FormatArg : public absl::AlphaNum { * handled specially to avoid ambiguity with generic pointers, which are * handled differently. */ - FormatArg(const char* string_value, internal::FormatChoice<3>) + FormatArg(const char* string_value, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<3>) : AlphaNum(string_value == nullptr ? "null" : string_value) { } @@ -125,8 +139,11 @@ class FormatArg : public absl::AlphaNum { * hexadecimal integer literal. */ template - FormatArg(T* pointer_value, internal::FormatChoice<4>) - : AlphaNum(absl::Hex(reinterpret_cast(pointer_value))) { + FormatArg(T* pointer_value, + absl::strings_internal::StringifySink&& sink, + internal::FormatChoice<4>) + : AlphaNum(absl::Hex(reinterpret_cast(pointer_value)), + std::move(sink)) { } /** @@ -134,7 +151,9 @@ class FormatArg : public absl::AlphaNum { * absl::AlphaNum accepts. */ template - FormatArg(T&& value, internal::FormatChoice<5>) + FormatArg(T&& value, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<5>) : AlphaNum(std::forward(value)) { } }; From 82e5289b97ba16b386644fb71d1865f2cbc7882d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 6 Jan 2025 20:03:27 +0000 Subject: [PATCH 2/7] string_format_test.cc: strengthen Pointer test to match a regex rather than just ASSERT_NE --- .../core/test/unit/util/string_format_test.cc | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/Firestore/core/test/unit/util/string_format_test.cc b/Firestore/core/test/unit/util/string_format_test.cc index b9236017a98..1d7b05585b8 100644 --- a/Firestore/core/test/unit/util/string_format_test.cc +++ b/Firestore/core/test/unit/util/string_format_test.cc @@ -17,6 +17,7 @@ #include "Firestore/core/src/util/string_format.h" #include "absl/strings/string_view.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" namespace firebase { @@ -72,14 +73,18 @@ TEST(StringFormatTest, Bool) { EXPECT_EQ("Hello false", StringFormat("Hello %s", false)); } -TEST(StringFormatTest, Pointer) { - // pointers implicitly convert to bool. Make sure this doesn't happen in - // this API. - int value = 4; - EXPECT_NE("Hello true", StringFormat("Hello %s", &value)); +TEST(StringFormatTest, NullPointer) { + // pointers implicitly convert to bool. Make sure this doesn't happen here. EXPECT_EQ("Hello null", StringFormat("Hello %s", nullptr)); } +TEST(StringFormatTest, NonNullPointer) { + // pointers implicitly convert to bool. Make sure this doesn't happen here. + int value = 4; + EXPECT_THAT(StringFormat("Hello %s", &value), + testing::MatchesRegex("Hello (0x)?[0123456789abcdefABCDEF]+")); +} + TEST(StringFormatTest, Mixed) { EXPECT_EQ("string=World, bool=true, int=42, float=1.5", StringFormat("string=%s, bool=%s, int=%s, float=%s", "World", true, From 24066f570fbc54081fd1b1486bc9fc721687f151 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 6 Jan 2025 20:06:08 +0000 Subject: [PATCH 3/7] Firestore/CHANGELOG.md entry added --- Firestore/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index ee7ea12f87f..ad57793f111 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,3 +1,6 @@ +# Unreleased +- [fixed] Fixed use-after-free bug when internally formatting strings. (#14306) + # 11.6.0 - [fixed] Add conditional `Sendable` conformance so `ServerTimestamp` is `Sendable` if `T` is `Sendable`. (#14042) From 64d14b8eb3cb17da986ef0ee30329e9c9d930c29 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Jan 2025 03:37:30 +0000 Subject: [PATCH 4/7] string_format_test.cc: improve robustness of NonNullPointer test to also check that the expected hex address is indeed contained in the actual formatted string. --- .../core/test/unit/util/string_format_test.cc | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/Firestore/core/test/unit/util/string_format_test.cc b/Firestore/core/test/unit/util/string_format_test.cc index 1d7b05585b8..dfbcc303b99 100644 --- a/Firestore/core/test/unit/util/string_format_test.cc +++ b/Firestore/core/test/unit/util/string_format_test.cc @@ -16,6 +16,10 @@ #include "Firestore/core/src/util/string_format.h" +#include +#include +#include + #include "absl/strings/string_view.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -24,6 +28,27 @@ namespace firebase { namespace firestore { namespace util { +namespace { + +using testing::HasSubstr; +using testing::MatchesRegex; + +std::string lowercase(const std::string& str) { + std::string lower_str = str; + std::transform(lower_str.begin(), lower_str.end(), lower_str.begin(), + [](auto c) { return std::tolower(c); }); + return lower_str; +} + +template +std::string hex_address(const T* ptr) { + std::ostringstream os; + os << std::hex << reinterpret_cast(ptr); + return os.str(); +} + +} // namespace + TEST(StringFormatTest, Empty) { EXPECT_EQ("", StringFormat("")); EXPECT_EQ("", StringFormat("%s", std::string().c_str())); @@ -81,8 +106,13 @@ TEST(StringFormatTest, NullPointer) { TEST(StringFormatTest, NonNullPointer) { // pointers implicitly convert to bool. Make sure this doesn't happen here. int value = 4; - EXPECT_THAT(StringFormat("Hello %s", &value), - testing::MatchesRegex("Hello (0x)?[0123456789abcdefABCDEF]+")); + + const std::string formatted_string = StringFormat("Hello %s", &value); + + const std::string hex_address_regex = "(0x)?[0123456789abcdefABCDEF]+"; + EXPECT_THAT(formatted_string, MatchesRegex("Hello " + hex_address_regex)); + const std::string expected_hex_address = lowercase(hex_address(&value)); + EXPECT_THAT(lowercase(formatted_string), HasSubstr(expected_hex_address)); } TEST(StringFormatTest, Mixed) { From 172111724c3c186e1111f03a63efb0112d98c196 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Jan 2025 21:24:01 +0000 Subject: [PATCH 5/7] string_format.h: make FormatArg final, to strongly discourage others from encountering the use-after-free bug. --- Firestore/core/src/util/string_format.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/core/src/util/string_format.h b/Firestore/core/src/util/string_format.h index 92676c3c7c8..cc4e9e40ea0 100644 --- a/Firestore/core/src/util/string_format.h +++ b/Firestore/core/src/util/string_format.h @@ -62,7 +62,7 @@ struct FormatChoice<5> {}; * formatting of the value as an unsigned integer. * * Otherwise the value is interpreted as anything absl::AlphaNum accepts. */ -class FormatArg : public absl::AlphaNum { +class FormatArg final : public absl::AlphaNum { public: template FormatArg(T&& value, From 72833cac1a9589cf89201ccc0cb1e21b9897712c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Jan 2025 21:43:33 +0000 Subject: [PATCH 6/7] add comment about b/388888512 --- Firestore/core/src/util/string_format.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Firestore/core/src/util/string_format.h b/Firestore/core/src/util/string_format.h index cc4e9e40ea0..c440e35911f 100644 --- a/Firestore/core/src/util/string_format.h +++ b/Firestore/core/src/util/string_format.h @@ -66,6 +66,11 @@ class FormatArg final : public absl::AlphaNum { public: template FormatArg(T&& value, + // TODO(b/388888512) Remove the usage of StringifySink since it is not part + // of absl's public API. Moreover, subclassing AlphaNum is not supported + // either, so find a way to do this without these two caveats. + // See https://github.com/firebase/firebase-ios-sdk/pull/14331 for a + // partial proposal. absl::strings_internal::StringifySink&& sink = {}) // NOLINT(runtime/explicit) : FormatArg{std::forward(value), std::move(sink), From 68ccc04ae18176dd4567d7be5a8dcb10c7544912 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Jan 2025 17:12:30 -0500 Subject: [PATCH 7/7] format code --- Firestore/core/src/util/string_format.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/Firestore/core/src/util/string_format.h b/Firestore/core/src/util/string_format.h index c440e35911f..ae905787f2a 100644 --- a/Firestore/core/src/util/string_format.h +++ b/Firestore/core/src/util/string_format.h @@ -65,14 +65,15 @@ struct FormatChoice<5> {}; class FormatArg final : public absl::AlphaNum { public: template - FormatArg(T&& value, - // TODO(b/388888512) Remove the usage of StringifySink since it is not part - // of absl's public API. Moreover, subclassing AlphaNum is not supported - // either, so find a way to do this without these two caveats. - // See https://github.com/firebase/firebase-ios-sdk/pull/14331 for a - // partial proposal. - absl::strings_internal::StringifySink&& sink = - {}) // NOLINT(runtime/explicit) + FormatArg( + T&& value, + // TODO(b/388888512) Remove the usage of StringifySink since it is not + // part of absl's public API. Moreover, subclassing AlphaNum is not + // supported either, so find a way to do this without these two caveats. + // See https://github.com/firebase/firebase-ios-sdk/pull/14331 for a + // partial proposal. + absl::strings_internal::StringifySink&& sink = + {}) // NOLINT(runtime/explicit) : FormatArg{std::forward(value), std::move(sink), internal::FormatChoice<0>{}} { }