Do not reject control message with trailing newlines

The previous fix to reject invalid control message was a bit too aggressive as scripts often accidentally include an extra newline at the end of the control message. Jira: OVPN3-1225 Signed-off-by: Arne Schwabe <[email protected]>
OpenVPN · Jul 5, 2024 · b201027 · b201027
1 parent 5022f30
commit b201027
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 3 deletions.
diff --git a/openvpn/ssl/proto.hpp b/openvpn/ssl/proto.hpp
@@ -1527,12 +1527,19 @@ class ProtoContext : public logging::LoggingMixin<OPENVPN_DEBUG_PROTO, logging::
         size_t size = buf.size();
         if (size)
         {
-            if (buf[size - 1] == 0)
+            /* Trim any trailing \n or \r or 0x00 characters. Scripts plugin sometimes accidentally include a \n or \r\n in AUTH_FAILED
+             * or similar messages */
+            while (size > 0 && (buf[size - 1] == 0 || buf[size - 1] == '\r' || buf[size - 1] == '\n'))
+            {
                 --size;
+            }
+
             if (size)
-                return S((const char *)buf.c_data(), size);
+            {
+                return S{reinterpret_cast<const char *>(buf.c_data()), size};
+            }
         }
-        return S();
+        return {};
     }
 
     template <typename S>

diff --git a/test/unittests/test_proto.cpp b/test/unittests/test_proto.cpp
@@ -1327,3 +1327,44 @@ TEST(proto, iv_ciphers_legacy)
 
     EXPECT_EQ(ivciphers, expectedstr);
 }
+
+TEST(proto, controlmessage_invalidchar)
+{
+    std::string valid_auth_fail{"AUTH_FAILED: go away"};
+    std::string valid_auth_fail_newline_end{"AUTH_FAILED: go away\n"};
+    std::string invalid_auth_fail{"AUTH_FAILED: go\n away\n"};
+    std::string lot_of_whitespace{"AUTH_FAILED: a lot of white space\n\n\r\n\r\n\r\n"};
+    std::string only_whitespace{"\n\n\r\n\r\n\r\n"};
+    std::string empty{""};
+
+    BufferAllocated valid_auth_fail_buf{reinterpret_cast<const unsigned char *>(valid_auth_fail.c_str()), valid_auth_fail.size(), BufferAllocated::GROW};
+    BufferAllocated valid_auth_fail_newline_end_buf{reinterpret_cast<const unsigned char *>(valid_auth_fail_newline_end.c_str()), valid_auth_fail_newline_end.size(), BufferAllocated::GROW};
+    BufferAllocated invalid_auth_fail_buf{reinterpret_cast<const unsigned char *>(invalid_auth_fail.c_str()), invalid_auth_fail.size(), BufferAllocated::GROW};
+    BufferAllocated lot_of_whitespace_buf{reinterpret_cast<const unsigned char *>(lot_of_whitespace.c_str()), lot_of_whitespace.size(), BufferAllocated::GROW};
+    BufferAllocated only_whitespace_buf{reinterpret_cast<const unsigned char *>(only_whitespace.c_str()), only_whitespace.size(), BufferAllocated::GROW};
+    BufferAllocated empty_buf{reinterpret_cast<const unsigned char *>(empty.c_str()), empty.size(), BufferAllocated::GROW};
+
+    auto msg = ProtoContext::read_control_string<std::string>(valid_auth_fail_buf);
+    EXPECT_EQ(msg, valid_auth_fail);
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg, Unicode::UTF8_NO_CTRL));
+
+    auto msg2 = ProtoContext::read_control_string<std::string>(valid_auth_fail_newline_end_buf);
+    EXPECT_EQ(msg2, valid_auth_fail);
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg2, Unicode::UTF8_NO_CTRL));
+
+    auto msg3 = ProtoContext::read_control_string<std::string>(invalid_auth_fail_buf);
+    EXPECT_EQ(msg3, "AUTH_FAILED: go\n away");
+    EXPECT_FALSE(Unicode::is_valid_utf8(msg3, Unicode::UTF8_NO_CTRL));
+
+    auto msg4 = ProtoContext::read_control_string<std::string>(lot_of_whitespace_buf);
+    EXPECT_EQ(msg4, "AUTH_FAILED: a lot of white space");
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg4, Unicode::UTF8_NO_CTRL));
+
+    auto msg5 = ProtoContext::read_control_string<std::string>(only_whitespace_buf);
+    EXPECT_EQ(msg5, "");
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg5, Unicode::UTF8_NO_CTRL));
+
+    auto msg6 = ProtoContext::read_control_string<std::string>(empty_buf);
+    EXPECT_EQ(msg6, "");
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg5, Unicode::UTF8_NO_CTRL));
+}