Security Audit Fixes and Enhancements

[christianparpart]

Security Audit Remediation Walkthrough

This document outlines the security vulnerabilities identified during the audit of the Contour terminal emulator and the remediation steps taken.

Identified Vulnerabilities

1. Critical Out-of-Bounds Write (CWE-787)

Component: vtbackend/Sequence.h (SequenceParameterBuilder)
Description: The nextParameter() method in SequenceParameterBuilder allowed incrementing the parameter iterator beyond the bounds of the static array (size 16), leading to memory corruption.
Fix Implemented: Added a check std::next(_currentParameter) != _parameters->_values.end() before incrementing the iterator.

2. High - SSH Blind Trust-On-First-Use (TOFU) (CWE-295)

Component: vtpty/SshSession.cpp
Description: The SSH implementation automatically added unknown host keys to the known_hosts file without user confirmation, making the client susceptible to Man-in-the-Middle (MITM) attacks.
Fix Implemented: Modified verifyHostKey to:

• Pause the connection process if the host key is unknown (NOTFOUND).
• Generate and display the host key fingerprint.
• Prompt the user for confirmation (yes/no).
• Only add the key and proceed if the user explicitly confirms.

3. Medium - Integer Overflow (CWE-190)

Component: vtbackend/Sequence.h
Description: Parameter accumulation (multiplyBy10AndAdd) lacked overflow checks, allowing values to wrap around 65535.
Fix Implemented: Added logic to saturate the accumulated value at 65535 (maximum safe value for uint16_t) if the calculation would overflow.

4. Low - Sensitive Data Exposure (Logging) (CWE-532)

Component: vtpty/SshSession.cpp
Description: Debug logs (ptyOutLog) captured all input sent to the SSH channel, including potential passwords.
Fix Implemented: Modified logging logic to detect authentication states (AuthenticatePassword, AuthenticatePrivateKey) and redact the log output during these sensitive phases.

Verification

Code Inspection

• OOB Write: The fix prevents the iterator from advancing to the invalid end() position. If nextParameter() is called when the array is full, it silently ignores the increment, preventing the OOB write.
• SSH TOFU: The new state VerifyHostKeyWaitForInput ensures the state machine waits for user input. The logic explicitly checks for "yes" before calling libssh2_knownhost_addc.
• Logging: The explicit check for authentication states ensures sensitive input is replaced with a redacted message in debug logs.

Proof of Concept (PoC)

A PoC reproduce_oob.cpp was updated to use the fixed Sequence.h. While full compilation was limited by dependency availability in the audit environment, the logic was verified by direct code analysis.

[Copilot]

Pull request overview

This PR addresses four security vulnerabilities identified in a security audit of the Contour terminal emulator. The fixes prevent memory corruption, strengthen SSH security, prevent integer overflow, and protect sensitive data in logs.

Key Changes:

• Fixed critical out-of-bounds write vulnerability in VT sequence parameter handling
• Implemented interactive SSH host key verification to prevent MITM attacks
• Added integer overflow protection with saturation for parameter parsing

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/vtbackend/Sequence.h	Fixed OOB write by checking iterator bounds before increment; added overflow saturation to 0xFFFF
src/vtbackend/Sequence_test.cpp	Added test case for maximum parameter limit boundary condition
src/vtpty/SshSession.h	Added new state VerifyHostKeyWaitForInput for interactive host key confirmation
src/vtpty/SshSession.cpp	Implemented user prompt for unknown host keys with fingerprint display; added log redaction for auth states
metainfo.xml	Documented security fixes in release notes for version 0.6.2

Comments suppressed due to low confidence (1)

src/vtbackend/Sequence.h:162

• The apply function has a logical bug in how it processes multi-digit values. When value >= 10, it recursively calls multiplyBy10AndAdd(value / 10) which can pass values greater than 9 to a function that expects single digits (0-9) based on its parameter type uint8_t.

For example, apply(123) would:

1. Call multiplyBy10AndAdd(123 / 10) = multiplyBy10AndAdd(12) (not a single digit!)
2. Call multiplyBy10AndAdd(123 % 10) = multiplyBy10AndAdd(3)

This defeats the purpose of the overflow check since multiplyBy10AndAdd(12) multiplies the current value by 10 and adds 12, which is not the intended digit-by-digit accumulation.

The function should recursively break down the number into individual digits:

constexpr void apply(uint16_t value) noexcept
{
    if (value >= 10)
        apply(value / 10);
    multiplyBy10AndAdd(static_cast<uint8_t>(value % 10));
}

    constexpr void apply(uint16_t value) noexcept
    {
        if (value >= 10)
            multiplyBy10AndAdd(static_cast<uint8_t>(value / 10));
        multiplyBy10AndAdd(static_cast<uint8_t>(value % 10));

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Dead code and confusing comments should be removed. Lines 1155-1160 contain commented-out code that attempts to call libssh2_knownhost_get and libssh2_session_hostkey but serve no purpose. These appear to be leftover debugging or experimentation code. The actual fingerprint generation on line 1161 is the correct implementation.

Suggested change

	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.

[Copilot]

[nitpick] The sensitive data logging check doesn't cover the VerifyHostKeyWaitForInput state. When the user types "yes" or "no" to confirm the host key, this input is logged in clear text on line 862. While not as critical as passwords, user confirmation inputs should also be redacted for consistency and to avoid cluttering logs with user interaction details.

Consider extending the sensitive check to include State::VerifyHostKeyWaitForInput:

bool const sensitive =
    (_state >= State::AuthenticatePrivateKeyStart && _state <= State::AuthenticatePassword) ||
    _state == State::VerifyHostKeyWaitForInput;

Suggested change

	_state >= State::AuthenticatePrivateKeyStart && _state <= State::AuthenticatePassword;
	(_state >= State::AuthenticatePrivateKeyStart && _state <= State::AuthenticatePassword) ||
	_state == State::VerifyHostKeyWaitForInput;

[Copilot]

The release entry for version 0.6.2 is missing the date attribute. According to AppStream metadata standards and the comment on line 94 ("TODO: Don't forget to add date attribute to it"), release entries should include a date.

Add a date attribute to the release tag:

<release version="0.6.2" urgency="medium" type="development" date="YYYY-MM-DD">

Replace YYYY-MM-DD with the actual release date in ISO 8601 format (e.g., "2025-01-15").

Suggested change

	<release version="0.6.2" urgency="medium" type="development">
	<release version="0.6.2" urgency="medium" type="development" date="2024-06-15">

[Copilot]

The test expectation doesn't accurately reflect the actual behavior due to the bug in the apply() function (see related comment on lines 158-162).

When apply(17) is called:

1. It calls multiplyBy10AndAdd(17 / 10) = multiplyBy10AndAdd(1), which does: 16 * 10 + 1 = 161
2. Then calls multiplyBy10AndAdd(17 % 10) = multiplyBy10AndAdd(7), which does: 161 * 10 + 7 = 1617

So the test expectation of 1617 happens to be correct, but for the wrong reasons. Once the bug in apply() is fixed to recursively process digits correctly, this test will fail because:

• With the fixed apply(), it would process digits 1 and 7 individually
• Since we're already at parameter 16, nextParameter() will be blocked
• The digits 1 and 7 would be applied to the current parameter (16): 16 -> 161 -> 1617

The test should be updated to include a comment explaining this behavior, or ideally test with a different value that makes the digit-by-digit processing more obvious.

[Copilot]

[nitpick] The fingerprint display defaults to "UNKNOWN" when libssh2_hostkey_hash() returns NULL (line 1173), but this could obscure the actual reason for failure and make it harder for users to troubleshoot connection issues.

Consider adding debug logging when fingerprint generation fails:

else
{
    fingerprintHex = "UNKNOWN";
    logError("Failed to generate host key fingerprint.");
}

This would help administrators diagnose issues with host key verification while still providing a safe fallback for the user prompt.

Suggested change

	else
	fingerprintHex = "UNKNOWN";
	else
	{
	fingerprintHex = "UNKNOWN";
	logError("Failed to generate host key fingerprint.");
	}