Repository: randombit/botan
Pull request #2731 (2021-04-27 20:55:57)

Fix sstream integer formatting
 As a stopgap of the issues raised in #2237 and #2730, use `std::to_string` instead of `ostringstream`'s native integer formatting. `std::to_string` is supposed to respect locale, thus having the same problem as `ostringstream`, but (likely for performance reasons), GCC, Clang and MSVC all ignore locale in their implementation. Also add a test so that if there is a compiler that implements `std::to_string` in a way that respects locale, and the test is run in a locale with thousands separators, then we see the failure instead of having a very mysterious error as in #2237 

If this test ever fails we'll probably have to give up on `std::to_string` and just implement something directly :cry: 


Patch:
diff --git a/src/lib/asn1/asn1_oid.cpp b/src/lib/asn1/asn1_oid.cpp
index ecedd100c..a184f6698 100644
--- a/src/lib/asn1/asn1_oid.cpp
+++ b/src/lib/asn1/asn1_oid.cpp
@@ -12,7 +12,6 @@
 #include <botan/internal/parsing.h>
 #include <botan/oids.h>
 #include <algorithm>
-#include <sstream>
 
 namespace Botan {
 
@@ -97,15 +96,14 @@ OID::OID(const std::string& oid_str)
 */
 std::string OID::to_string() const
    {
-   std::ostringstream oss;
-   oss.imbue(std::locale("C"));
+   std::string out;
    for(size_t i = 0; i != m_id.size(); ++i)
       {
-      oss << m_id[i];
+      out += std::to_string(m_id[i]);
       if(i != m_id.size() - 1)
-         oss << ".";
+         out += ".";
       }
-   return oss.str();
+   return out;
    }
 
 std::string OID::to_formatted_string() const
diff --git a/src/lib/modes/aead/aead.cpp b/src/lib/modes/aead/aead.cpp
index 772b88ccd..a09aedf00 100644
--- a/src/lib/modes/aead/aead.cpp
+++ b/src/lib/modes/aead/aead.cpp
@@ -82,17 +82,16 @@ std::unique_ptr<AEAD_Mode> AEAD_Mode::create(const std::string& algo,
       if(mode_info.empty())
          return std::unique_ptr<AEAD_Mode>();
 
-      std::ostringstream alg_args;
+      std::ostringstream mode_name;
 
-      alg_args << '(' << cipher_name;
+      mode_name << mode_info[0] << '(' << cipher_name;
       for(size_t i = 1; i < mode_info.size(); ++i)
-         alg_args << ',' << mode_info[i];
+         mode_name << ',' << mode_info[i];
       for(size_t i = 2; i < algo_parts.size(); ++i)
-         alg_args << ',' << algo_parts[i];
-      alg_args << ')';
+         mode_name << ',' << algo_parts[i];
+      mode_name << ')';
 
-      const std::string mode_name = mode_info[0] + alg_args.str();
-      return AEAD_Mode::create(mode_name, dir);
+      return AEAD_Mode::create(mode_name.str(), dir);
       }
 
 #if defined(BOTAN_HAS_BLOCK_CIPHER)
diff --git a/src/lib/modes/cipher_mode.cpp b/src/lib/modes/cipher_mode.cpp
index a1d80a586..b3fe5e86a 100644
--- a/src/lib/modes/cipher_mode.cpp
+++ b/src/lib/modes/cipher_mode.cpp
@@ -104,17 +104,16 @@ std::unique_ptr<Cipher_Mode> Cipher_Mode::create(const std::string& algo,
       if(mode_info.empty())
          return std::unique_ptr<Cipher_Mode>();
 
-      std::ostringstream alg_args;
+      std::ostringstream mode_name;
 
-      alg_args << '(' << cipher_name;
+      mode_name << mode_info[0] << '(' << cipher_name;
       for(size_t i = 1; i < mode_info.size(); ++i)
-         alg_args << ',' << mode_info[i];
+         mode_name << ',' << mode_info[i];
       for(size_t i = 2; i < algo_parts.size(); ++i)
-         alg_args << ',' << algo_parts[i];
-      alg_args << ')';
+         mode_name << ',' << algo_parts[i];
+      mode_name << ')';
 
-      const std::string mode_name = mode_info[0] + alg_args.str();
-      return Cipher_Mode::create(mode_name, direction, provider);
+      return Cipher_Mode::create(mode_name.str(), direction, provider);
       }
 
 #if defined(BOTAN_HAS_BLOCK_CIPHER)
diff --git a/src/lib/passhash/argon2fmt/argon2fmt.cpp b/src/lib/passhash/argon2fmt/argon2fmt.cpp
index 022499505..cfd3d4cbd 100644
--- a/src/lib/passhash/argon2fmt/argon2fmt.cpp
+++ b/src/lib/passhash/argon2fmt/argon2fmt.cpp
@@ -62,7 +62,9 @@ std::string argon2_generate_pwhash(const char* password, size_t password_len,
    else
       oss << "$argon2id$";
 
-   oss << "v=19$m=" << M << ",t=" << t << ",p=" << p << "$";
+   oss << "v=19$m=" << std::to_string(M)
+       << ",t=" << std::to_string(t)
+       << ",p=" << std::to_string(p) << "$";
    oss << strip_padding(base64_encode(salt)) << "$" << strip_padding(base64_encode(output));
 
    return oss.str();
diff --git a/src/lib/pbkdf/scrypt/scrypt.cpp b/src/lib/pbkdf/scrypt/scrypt.cpp
index 3c3eba95e..0e8322056 100644
--- a/src/lib/pbkdf/scrypt/scrypt.cpp
+++ b/src/lib/pbkdf/scrypt/scrypt.cpp
@@ -12,7 +12,6 @@
 #include <botan/internal/loadstor.h>
 #include <botan/internal/bit_ops.h>
 #include <botan/internal/timer.h>
-#include <sstream>
 
 namespace Botan {
 
@@ -153,9 +152,15 @@ Scrypt::Scrypt(size_t N, size_t r, size_t p) :
 
 std::string Scrypt::to_string() const
    {
-   std::ostringstream oss;
-   oss << "Scrypt(" << m_N << "," << m_r << "," << m_p << ")";
-   return oss.str();
+   std::string out;
+   out = "Scrypt(";
+   out += std::to_string(m_N);
+   out += ",";
+   out += std::to_string(m_r);
+   out += ",";
+   out += std::to_string(m_p);
+   out += ")";
+   return out;
    }
 
 size_t Scrypt::total_memory_usage() const
diff --git a/src/lib/utils/version.cpp b/src/lib/utils/version.cpp
index aa82d7cc9..60a5de5ec 100644
--- a/src/lib/utils/version.cpp
+++ b/src/lib/utils/version.cpp
@@ -90,7 +90,9 @@ std::string runtime_version_check(uint32_t major,
       std::ostringstream oss;
       oss << "Warning: linked version (" << short_version_string() << ")"
           << " does not match version built against "
-          << "(" << major << '.' << minor << '.' << patch << ")\n";
+          << "(" << std::to_string(major)
+          << '.' << std::to_string(minor)
+          << '.' << std::to_string(patch) << ")\n";
       return oss.str();
       }
 


Test patch:
diff --git a/src/tests/test_oid.cpp b/src/tests/test_oid.cpp
index 430d4fe4b..f803b2d05 100644
--- a/src/tests/test_oid.cpp
+++ b/src/tests/test_oid.cpp
@@ -16,6 +16,41 @@ namespace {
 
 #if defined(BOTAN_HAS_ASN1)
 
+Test::Result test_OID_to_string()
+   {
+   /*
+   See #2730 and #2237
+
+   Certain locales format integers with thousands seperators.  This
+   caused a subtle bug which caused OID comparisons to fail because
+   OID::to_string(), which used ostringstream, introduced a thousands
+   seperator when the OID component had a value >= 1000. But this
+   only failed in certain locales (pt_BR was reported).
+
+   Nominally C++ requires std::to_string to also be locale-respecting.
+   But, libc++, libstdc++, and MSVC's STL library all implement
+   std::to_string in a way that ignores locales, because adding locale
+   support means std::to_string will be both slow and a serialization
+   point. So as a stopgap we assume this behavior from std::to_string.
+
+   Here we test the original issue of #2237 to verify it works. If
+   the compiler implements std::to_string in a way that respects locale,
+   *and* this test is run in a locale that uses thousands seperators,
+   then it will fail. Which is much better than a very subtle failure.
+   However if it ever does fail then we must replace nearly every
+   call to std::to_string with something else that ignores locale.
+   */
+
+   Botan::OID oid{1,2,1000,1001,1002000};
+
+   Test::Result result("OID::to_string");
+
+   result.test_eq("OID::to_string behaves as we expect",
+                  oid.to_string(), "1.2.1000.1001.1002000");
+
+   return result;
+   }
+
 Test::Result test_add_have_OID()
    {
    Test::Result result("OID add");
@@ -83,6 +118,7 @@ class OID_Tests final : public Test
 
          std::vector<std::function<Test::Result()>> fns =
             {
+            test_OID_to_string,
             test_add_have_OID,
             test_add_have_OID_str,
             test_add_and_lookup,