From f66983155634c8552fe88abdeefbac5a218ef781 Mon Sep 17 00:00:00 2001
From: James Yonan <james@openvpn.net>
Date: Sun, 8 Mar 2020 17:39:03 -0600
Subject: [PATCH] Base64: fixed issue where decode() method would ignore bad
 chars rather than throwing base64_decode_error()

Added a unit test to confirm the fix.

Other changes:

* In Base64 decode(), avoid the use of std::strlen() in favor
  of std::string length() method since a std::string could
  conceivably contain embedded null chars.

* In Base64 unit test, renamed b64_test_bad() to
  b64_test_bad_decode() for clarity.

Signed-off-by: James Yonan <james@openvpn.net>
---
 openvpn/common/base64.hpp   | 13 ++++++++-----
 test/unittests/.gitignore   |  1 +
 test/unittests/test_b64.cpp | 19 ++++++++++---------
 3 files changed, 19 insertions(+), 14 deletions(-)
 create mode 100644 test/unittests/.gitignore

diff --git a/openvpn/common/base64.hpp b/openvpn/common/base64.hpp
index 85b02d74..20e7a829 100644
--- a/openvpn/common/base64.hpp
+++ b/openvpn/common/base64.hpp
@@ -25,7 +25,9 @@
 #define OPENVPN_COMMON_BASE64_H
 
 #include <string>
-#include <cstring> // for std::memset, std::strlen
+#include <cstring>   // for std::memset, std::strlen
+#include <algorithm> // for std::min
+#include <cstddef>   // for ptrdiff_t
 
 #include <openvpn/common/size.hpp>
 #include <openvpn/common/exception.hpp>
@@ -195,10 +197,11 @@ namespace openvpn {
     template <typename V>
     void decode(V& dest, const std::string& str) const
     {
-      for (const char *p = str.c_str(); *p != '\0' && (*p == equal || is_base64_char(*p)); p += 4)
+      const char *endp = str.c_str() + str.length();
+      for (const char *p = str.c_str(); p < endp; p += 4)
 	{
 	  unsigned int marker;
-	  const unsigned int val = token_decode(p, marker);
+	  const unsigned int val = token_decode(p, std::min(endp - p, ptrdiff_t(4)), marker);
 	  dest.push_back((val >> 16) & 0xff);
 	  if (marker < 2)
 	    dest.push_back((val >> 8) & 0xff);
@@ -249,12 +252,12 @@ namespace openvpn {
       return v;
     }
 
-    unsigned int token_decode(const char *token, unsigned int& marker) const
+    unsigned int token_decode(const char *token, const ptrdiff_t len, unsigned int& marker) const
     {
       size_t i;
       unsigned int val = 0;
       marker = 0; // number of equal chars seen
-      if (std::strlen(token) < 4)
+      if (len < 4)
 	throw base64_decode_error();
       for (i = 0; i < 4; i++)
 	{
diff --git a/test/unittests/.gitignore b/test/unittests/.gitignore
new file mode 100644
index 00000000..ead83a23
--- /dev/null
+++ b/test/unittests/.gitignore
@@ -0,0 +1 @@
+test_b64
diff --git a/test/unittests/test_b64.cpp b/test/unittests/test_b64.cpp
index aa0e8ce5..9abac8f4 100644
--- a/test/unittests/test_b64.cpp
+++ b/test/unittests/test_b64.cpp
@@ -117,22 +117,23 @@ TEST(Base64, tooshortdest)
     EXPECT_THROW(b64.decode(buf, 2, enc), Base64::base64_decode_out_of_bound_error);
 }
 
-void b64_test_bad(const Base64& b64, const std::string& text)
+void b64_test_bad_decode(const Base64& b64, const std::string& text)
 {
     std::string dec;
     EXPECT_THROW(b64.decode(dec, text), Base64::base64_decode_error);
 }
 
-TEST(Base64, badencode)
+TEST(Base64, baddecode)
 {
     const Base64 b64;
 
-    b64_test_bad(b64, "plausible deniability");
-    b64_test_bad(b64, "plausible != deniability");
-    b64_test_bad(b64, "x");
-    b64_test_bad(b64, "====");
-    b64_test_bad(b64, "xxxx=");
-    b64_test_bad(b64, "01*=");
+    b64_test_bad_decode(b64, "!@#$%^&*()_");
+    b64_test_bad_decode(b64, "plausible deniability");
+    b64_test_bad_decode(b64, "plausible != deniability");
+    b64_test_bad_decode(b64, "x");
+    b64_test_bad_decode(b64, "====");
+    b64_test_bad_decode(b64, "xxxx=");
+    b64_test_bad_decode(b64, "01*=");
 }
 
 TEST(Base64, encode)
@@ -178,4 +179,4 @@ TEST(Base64, binary_data)
         b64_test_binary(b64, data, i);
         delete[] data;
     }
-}
\ No newline at end of file
+}