Apply security audit findings

Signed-off-by: Christian Parpart <christian@parpart.family>

Author: christianparpart

src/vtbackend/Sequence.h

@@ -128,7 +128,7 @@ class SequenceParameterBuilder
 
     void nextParameter()
     {
-        if (_currentParameter != _parameters->_values.end())
+        if (std::next(_currentParameter) != _parameters->_values.end())
         {
             ++_currentParameter;
             *_currentParameter = 0;
@@ -138,7 +138,7 @@ class SequenceParameterBuilder
 
     void nextSubParameter()
     {
-        if (_currentParameter != _parameters->_values.end())
+        if (std::next(_currentParameter) != _parameters->_values.end())
         {
             ++_currentParameter;
             *_currentParameter = 0;
@@ -148,7 +148,11 @@ class SequenceParameterBuilder
 
     constexpr void multiplyBy10AndAdd(uint8_t value) noexcept
     {
-        *_currentParameter = static_cast<uint16_t>((*_currentParameter * 10) + value);
+        unsigned const newValue = (*_currentParameter * 10) + value;
+        if (newValue > 0xFFFF)
+            *_currentParameter = 0xFFFF;
+        else
+            *_currentParameter = static_cast<uint16_t>(newValue);
     }
 
     constexpr void apply(uint16_t value) noexcept

src/vtbackend/Sequence_test.cpp

@@ -124,3 +124,30 @@ TEST_CASE("SequenceParameterBuilder.complex.2")
     INFO(parameters.subParameterBitString());
     CHECK(parameters.str() == "0;12::34:56;7;89");
 }
+
+TEST_CASE("SequenceParameterBuilder.max_parameters_exceeded")
+{
+    auto parameters = SequenceParameters {};
+    auto builder = SequenceParameterBuilder { parameters };
+
+    // Fill up to 16 parameters: "1;2;...;16"
+    for (int i = 1; i <= 16; ++i)
+    {
+        if (i > 1)
+            builder.nextParameter();
+        builder.apply(i);
+    }
+
+    // Try to add a 17th parameter: ";17"
+    // This should be ignored (or merged) and NOT cause an OOB write.
+    builder.nextParameter();
+    builder.apply(17);
+
+    builder.fixiate();
+
+    CHECK(parameters.count() == 16);
+
+    // Verify behavior: since advancement was blocked, the value '17' was appended to the last parameter '16'.
+    // 16 -> 1617
+    CHECK(parameters.at(15) == 1617);
+}

src/vtpty/SshSession.cpp

@@ -502,11 +502,17 @@ void SshSession::processState()
             }
             case State::VerifyHostKey: {
                 if (!verifyHostKey())
+                {
+                    if (_state == State::VerifyHostKeyWaitForInput)
+                        return;
+
                     setState(State::Failure);
+                }
                 else
                     setState(State::AuthenticateAgent);
                 break;
             }
+            case State::VerifyHostKeyWaitForInput: return;
             case State::AuthenticateAgent: {
                 authenticateWithAgent();
                 break;
@@ -814,6 +820,11 @@ int SshSession::write(std::string_view buf)
         handlePreAuthenticationPasswordInput(buf, State::AuthenticatePrivateKey);
         return static_cast<int>(buf.size()); // Make the caller believe that we have written all bytes.
     }
+    else if (_state == State::VerifyHostKeyWaitForInput)
+    {
+        handlePreAuthenticationPasswordInput(buf, State::VerifyHostKey);
+        return static_cast<int>(buf.size()); // Make the caller believe that we have written all bytes.
+    }
     else if (_state != State::Operational)
     {
         sshLog()("Ignoring write() call in state: {}", _state);
@@ -839,13 +850,22 @@ int SshSession::write(std::string_view buf)
 
     if (ptyOutLog)
     {
-        if (rv >= 0)
-            ptyOutLog()("Sending bytes: \"{}\"", crispy::escape(buf.data(), buf.data() + rv)); // NOLINT
+        bool const sensitive =
+            _state >= State::AuthenticatePrivateKeyStart && _state <= State::AuthenticatePassword;
+        if (sensitive)
+        {
+            ptyOutLog()("Sending {} bytes: (hidden in auth state)", rv);
+        }
+        else
+        {
+            if (rv >= 0)
+                ptyOutLog()("Sending bytes: \"{}\"", crispy::escape(buf.data(), buf.data() + rv)); // NOLINT
 
-        if (0 <= rv && static_cast<size_t>(rv) < buf.size())
-            ptyOutLog()("Partial write. {} bytes written and {} bytes left.",
-                        rv,
-                        buf.size() - static_cast<size_t>(rv));
+            if (0 <= rv && static_cast<size_t>(rv) < buf.size())
+                ptyOutLog()("Partial write. {} bytes written and {} bytes left.",
+                            rv,
+                            buf.size() - static_cast<size_t>(rv));
+        }
     }
 
     return static_cast<int>(rv);
@@ -1088,27 +1108,78 @@ bool SshSession::verifyHostKey()
             logError("Host key verification failed. Host key mismatch.");
             return false;
         case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND: {
-            // TODO: Ask user whether to add host key to known_hosts file
-            auto const comment =
-                std::format("{}@{}:{} (added by Contour)", _config.username, _config.hostname, _config.port);
-            auto const typeMask = LIBSSH2_KNOWNHOST_TYPE_PLAIN | LIBSSH2_KNOWNHOST_KEYENC_RAW | knownhostType;
-            libssh2_knownhost_addc(knownHosts,
-                                   _config.hostname.c_str(),
-                                   nullptr /* salt */,
-                                   hostkeyRaw,
-                                   hostkeyLength,
-                                   comment.data(),
-                                   comment.size(),
-                                   typeMask,
-                                   nullptr /* store */);
-            rc = libssh2_knownhost_writefile(
-                knownHosts, _config.knownHostsFile.string().c_str(), LIBSSH2_KNOWNHOST_FILE_OPENSSH);
-            if (rc != LIBSSH2_ERROR_NONE)
+            if (!_injectedWrite.empty())
             {
-                logErrorWithDetails(rc, "Failed to write known_hosts file");
-                return false;
+                // We've got a user response.
+                auto const trim = [](std::string_view s) {
+                    auto const start = s.find_first_not_of(" \t\r\n");
+                    if (start == std::string_view::npos)
+                        return std::string_view {};
+                    auto const end = s.find_last_not_of(" \t\r\n");
+                    return s.substr(start, end - start + 1);
+                };
+                auto const response = trim(_injectedWrite);
+                _injectedWrite.clear();
+                if (response == "yes")
+                {
+                    auto const comment = std::format(
+                        "{}@{}:{} (added by Contour)", _config.username, _config.hostname, _config.port);
+                    auto const typeMask =
+                        LIBSSH2_KNOWNHOST_TYPE_PLAIN | LIBSSH2_KNOWNHOST_KEYENC_RAW | knownhostType;
+                    libssh2_knownhost_addc(knownHosts,
+                                           _config.hostname.c_str(),
+                                           nullptr /* salt */,
+                                           hostkeyRaw,
+                                           hostkeyLength,
+                                           comment.data(),
+                                           comment.size(),
+                                           typeMask,
+                                           nullptr /* store */);
+                    rc = libssh2_knownhost_writefile(
+                        knownHosts, _config.knownHostsFile.string().c_str(), LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+                    if (rc != LIBSSH2_ERROR_NONE)
+                    {
+                        logErrorWithDetails(rc, "Failed to write known_hosts file");
+                        return false;
+                    }
+                    injectRead("\r\nHost key verified and added.\r\n");
+                    return true;
+                }
+                else
+                {
+                    injectRead("\r\nHost key verification failed. Connection aborted.\r\n");
+                    return false;
+                }
             }
-            return true;
+            // Ask user whether to add host key to known_hosts file
+            libssh2_knownhost_get(knownHosts, &knownHost, knownHost); // wait, knownHost is null?
+            // libssh2_knownhost_checkp returns the known host if found, but here it is NOTFOUND.
+            // We need to generate fingerprint from hostkeyRaw.
+            libssh2_session_hostkey(
+                _p->sshSession, &hostkeyLength, &knownhostType); // Use session function to get fingerprint?
+            // Actually libssh2_hostkey_hash is what we want.
+            auto const fingerprint = libssh2_hostkey_hash(_p->sshSession, LIBSSH2_HOSTKEY_HASH_SHA256);
+
+            auto fingerprintHex = std::string {};
+            if (fingerprint)
+            {
+                // SHA256 is 32 bytes
+                for (int i = 0; i < 32; ++i)
+                    fingerprintHex += std::format("{:02X}:", (unsigned char) fingerprint[i]);
+                if (!fingerprintHex.empty())
+                    fingerprintHex.pop_back();
+            }
+            else
+                fingerprintHex = "UNKNOWN";
+
+            auto const message = std::format("\r\nThe authenticity of host '{}' cannot be established.\r\n"
+                                             "Fingerprint: {}\r\n"
+                                             "Are you sure you want to continue connecting (yes/no)? ",
+                                             _config.hostname,
+                                             fingerprintHex);
+            injectRead(message);
+            setState(State::VerifyHostKeyWaitForInput);
+            return false;
         }
         case LIBSSH2_KNOWNHOST_CHECK_FAILURE: {
             logErrorWithDetails(rc, "Host key verification failed");

src/vtpty/SshSession.h

@@ -78,6 +78,7 @@ class SshSession final: public Pty
         Connect,                            // connect to SSH server (usually TCP/IPv4 or TCP/IPv6)
         Handshake,                          // SSH handshake
         VerifyHostKey,                      // verify host key against known_hosts file
+        VerifyHostKeyWaitForInput,          // wait for user confirmation of host key
         AuthenticateAgent,                  // authenticate with SSH agent
         AuthenticatePrivateKeyStart,        // start private key authentication
         AuthenticatePrivateKeyRequest,      // request private key's password
@@ -185,6 +186,7 @@ struct std::formatter<vtpty::SshSession::State>: std::formatter<std::string_view
             case vtpty::SshSession::State::Connect: name = "Connect"; break;
             case vtpty::SshSession::State::Handshake: name = "Handshake"; break;
             case vtpty::SshSession::State::VerifyHostKey: name = "VerifyHostKey"; break;
+            case vtpty::SshSession::State::VerifyHostKeyWaitForInput: name = "VerifyHostKeyWaitForInput"; break;
             case vtpty::SshSession::State::AuthenticateAgent: name = "AuthenticateAgent"; break;
             case vtpty::SshSession::State::AuthenticatePrivateKeyStart: name = "AuthenticatePrivateKeyStart"; break;
             case vtpty::SshSession::State::AuthenticatePrivateKeyRequest: name = "AuthenticatePrivateKeyRequest"; break;