From cb06f9e3307cc2aa936b5bb7e91725b83354933c Mon Sep 17 00:00:00 2001 From: Frank Lichtenheld Date: Fri, 27 Oct 2023 15:19:06 +0200 Subject: [PATCH] SplitLines/UserPass: Review fixes Signed-off-by: Frank Lichtenheld --- openvpn/common/splitlines.hpp | 40 ++++++++++++++++------------ openvpn/common/userpass.hpp | 17 +++++++----- openvpn/transport/client/httpcli.hpp | 2 +- test/unittests/test_splitlines.cpp | 4 +-- 4 files changed, 37 insertions(+), 26 deletions(-) diff --git a/openvpn/common/splitlines.hpp b/openvpn/common/splitlines.hpp index 12b68a7f..1ea9626c 100644 --- a/openvpn/common/splitlines.hpp +++ b/openvpn/common/splitlines.hpp @@ -4,7 +4,7 @@ // packet encryption, packet authentication, and // packet compression. // -// Copyright (C) 2012-2022 OpenVPN Inc. +// Copyright (C) 2012-2023 OpenVPN Inc. // // This program is free software: you can redistribute it and/or modify // it under the terms of the GNU Affero General Public License Version 3 @@ -33,18 +33,18 @@ template class SplitLinesType { public: - OPENVPN_EXCEPTION(splitlines_overflow_error); - OPENVPN_EXCEPTION(splitlines_moved_error); + OPENVPN_EXCEPTION(overflow_error); + OPENVPN_EXCEPTION(moved_error); /** - * Initialises SplitLinesType object with pointer to str. + * Initialises SplitLinesType object with pointer to str * * Note: string/buffer passed to constructor is not locally stored, * so it must remain in scope and not be modified during the lifetime * of the SplitLines object. * - * @param max_line_len_arg If not 0, specifies line length that will trigger - * overflow error. + * @param max_line_len_arg If not 0, specifies line length that + * will trigger overflow error. */ SplitLinesType(const STRING &str, const size_t max_line_len_arg = 0) : data((const char *)str.c_str()), @@ -54,7 +54,9 @@ class SplitLinesType } /** - * Read next line so that it can be accessed with line_ref or line_move + * Read next line so that it can be accessed with line_ref or line_move. + * + * If max_line_len is greater zero, read at most max_line_len characters. * * @param trim If true, remove trailing \n or \r\n * @return Returns true if any characters were read. @@ -86,7 +88,8 @@ class SplitLinesType } /** - * Returns true if current line triggered an overflow error. + * Returns true if max_line_len is greater zero and the current line was + * longer than max_line_len characters. */ bool line_overflow() const { @@ -97,6 +100,7 @@ class SplitLinesType * Returns reference to current line. * * Throws an exception if there is no line available currently. + * Throws an exception if line_overflow() returns true. */ std::string &line_ref() { @@ -108,6 +112,7 @@ class SplitLinesType * Returns const reference to current line. * * Throws an exception if there is no line available currently. + * Throws an exception if line_overflow() returns true. */ const std::string &line_ref() const { @@ -119,7 +124,8 @@ class SplitLinesType * Returns the moved current line. * * Throws an exception if there is no line available currently. - * Further calls to line_ref or line_moved will throw an exception until + * Throws an exception if line_overflow() returns true. + * Further calls to line_ref() or line_moved() will throw an exception until * operator() is called again. */ std::string line_move() @@ -131,9 +137,9 @@ class SplitLinesType enum Status { - S_OKAY, - S_EOF, - S_ERROR + S_OKAY, //!< next line was successfully read + S_EOF, //!< no further characters are available + S_ERROR //!< line was longer than allowed }; /** @@ -143,13 +149,13 @@ class SplitLinesType * returns S_ERROR. If nothing could be read, returns * S_EOF. * Since the line is moved into the argument, you can't - * use line_ref or line_moved on the object afterwards. - * In general calls to operator()+line_ref and next() + * use line_ref() or line_moved() on the object afterwards. + * In general calls to operator()+line_ref() and next() * are not intended to be mixed. * * @param ln string to move the line into. * @param trim If true, remove trailing \n or \r\n - * @return Returns S_OKAY if a line was moved. + * @return Returns S_OKAY if a line was moved into ln. */ Status next(std::string &ln, const bool trim = true) { @@ -167,9 +173,9 @@ class SplitLinesType void validate() { if (!line_valid) - throw splitlines_moved_error(); + throw moved_error(); if (overflow) - throw splitlines_overflow_error(line); + throw overflow_error(line); } const char *data; diff --git a/openvpn/common/userpass.hpp b/openvpn/common/userpass.hpp index bbcc0f5c..5665c648 100644 --- a/openvpn/common/userpass.hpp +++ b/openvpn/common/userpass.hpp @@ -59,13 +59,16 @@ enum Flags * password * * - * The multiline argument is allowed to be 1024 characters in - * length. + * The multiline argument is allowed to be 1024 UTF-8 characters in + * length. If it is longer, the function will throw an exception. * * If the TRY_FILE flag is set and the argument is not multiline, * then it is interpreted as a filepath and the contents of the file * will replace the argument. * + * Lines in the file are only allowed to be 1024 bytes in length. + * Longer lines will cause an exception to be thrown. + * * If the argument contains a newline, then the first line is used as the * username and the second line is used as the password, otherwise the argument * is the username. Note that no empty entry will be appended to the vector if @@ -119,14 +122,15 @@ inline bool parse(const OptionList &options, * password * * - * The multiline argument is allowed to be 1024 characters in - * length. + * The multiline argument is allowed to be 1024 UTF-8 characters in + * length. If it is longer, the function will throw an exception. * * If the TRY_FILE flag is set and the argument is not multiline, * then it is interpreted as a filepath and the contents of the file * will replace the argument. * - * Lines in the file are only allowed to be 1024 characters in length. + * Lines in the file are only allowed to be 1024 bytes in length. + * Longer lines will cause an exception to be thrown. * * If the argument contains a newline, then the first line is used as the * username and the second line is used as the password, otherwise the argument @@ -175,7 +179,8 @@ inline void parse(const OptionList &options, * username and the second line is used as the password, otherwise the content * is the username. * - * Lines in the file are only allowed to be 1024 characters in length. + * Lines in the file are only allowed to be 1024 bytes in length. + * Longer lines will cause an exception to be thrown. * * If USERNAME_REQUIRED and/or PASSWORD_REQUIRED flag is set, then it will throw * creds_error instead of returning empty values. diff --git a/openvpn/transport/client/httpcli.hpp b/openvpn/transport/client/httpcli.hpp index 6ccfd4c6..ccd04850 100644 --- a/openvpn/transport/client/httpcli.hpp +++ b/openvpn/transport/client/httpcli.hpp @@ -132,7 +132,7 @@ class Options : public RC set_proxy_server(hp->get(1, 256), hp->get(2, 16)); // get creds - UserPass::parse(opt, "http-proxy-user-pass", UserPass::OPT_OPTIONAL, username, password); + UserPass::parse(opt, "http-proxy-user-pass", 0, username, password); const std::string auth = hp->get_optional(3, 16); if (!auth.empty()) diff --git a/test/unittests/test_splitlines.cpp b/test/unittests/test_splitlines.cpp index a20d4c40..74748af2 100644 --- a/test/unittests/test_splitlines.cpp +++ b/test/unittests/test_splitlines.cpp @@ -104,7 +104,7 @@ TEST(SplitLines, max_length_overflow) SplitLines in(short_text, 3); ASSERT_TRUE(in(true)); ASSERT_TRUE(in.line_overflow()); - ASSERT_THROW(in.line_ref(), SplitLines::splitlines_overflow_error); + ASSERT_THROW(in.line_ref(), SplitLines::overflow_error); } TEST(SplitLines, next_max_length_overflow) @@ -120,5 +120,5 @@ TEST(SplitLines, moved_error) ASSERT_TRUE(in(true)); ASSERT_FALSE(in.line_overflow()); std::string line = in.line_move(); - ASSERT_THROW(in.line_ref(), SplitLines::splitlines_moved_error); + ASSERT_THROW(in.line_ref(), SplitLines::moved_error); }