From 58a9e30017b7094c9cf8bfb0739b610ba5bcd450 Mon Sep 17 00:00:00 2001 From: Keith Burzinski Date: Fri, 16 Jan 2026 17:05:19 -0600 Subject: [PATCH 1/4] [helpers] Add `base64_decode_int32_vector` function (#13289) Co-authored-by: J. Nick Koston --- esphome/core/helpers.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ esphome/core/helpers.h | 6 ++++++ 2 files changed, 46 insertions(+) diff --git a/esphome/core/helpers.cpp b/esphome/core/helpers.cpp index 96b2d46d783..5cad2308df2 100644 --- a/esphome/core/helpers.cpp +++ b/esphome/core/helpers.cpp @@ -624,6 +624,46 @@ std::vector base64_decode(const std::string &encoded_string) { return ret; } +/// Decode base64/base64url string directly into vector of little-endian int32 values +/// @param base64 Base64 or base64url encoded string (both +/ and -_ accepted) +/// @param out Output vector (cleared and filled with decoded int32 values) +/// @return true if successful, false if decode failed or invalid size +bool base64_decode_int32_vector(const std::string &base64, std::vector &out) { + // Decode in chunks to minimize stack usage + constexpr size_t chunk_bytes = 48; // 12 int32 values + constexpr size_t chunk_chars = 64; // 48 * 4/3 = 64 chars + uint8_t chunk[chunk_bytes]; + + out.clear(); + + const uint8_t *input = reinterpret_cast(base64.data()); + size_t remaining = base64.size(); + size_t pos = 0; + + while (remaining > 0) { + size_t chars_to_decode = std::min(remaining, chunk_chars); + size_t decoded_len = base64_decode(input + pos, chars_to_decode, chunk, chunk_bytes); + + if (decoded_len == 0) + return false; + + // Parse little-endian int32 values + for (size_t i = 0; i + 3 < decoded_len; i += 4) { + int32_t timing = static_cast(encode_uint32(chunk[i + 3], chunk[i + 2], chunk[i + 1], chunk[i])); + out.push_back(timing); + } + + // Check for incomplete int32 in last chunk + if (remaining <= chunk_chars && (decoded_len % 4) != 0) + return false; + + pos += chars_to_decode; + remaining -= chars_to_decode; + } + + return !out.empty(); +} + /// Encode int32 to 5 base85 characters + null terminator /// Standard ASCII85 alphabet: '!' (33) = 0 through 'u' (117) = 84 inline void base85_encode_int32(int32_t value, std::span output) { diff --git a/esphome/core/helpers.h b/esphome/core/helpers.h index 9dc289c7436..000762c9bfc 100644 --- a/esphome/core/helpers.h +++ b/esphome/core/helpers.h @@ -1137,6 +1137,12 @@ std::vector base64_decode(const std::string &encoded_string); size_t base64_decode(std::string const &encoded_string, uint8_t *buf, size_t buf_len); size_t base64_decode(const uint8_t *encoded_data, size_t encoded_len, uint8_t *buf, size_t buf_len); +/// Decode base64/base64url string directly into vector of little-endian int32 values +/// @param base64 Base64 or base64url encoded string (both +/ and -_ accepted) +/// @param out Output vector (cleared and filled with decoded int32 values) +/// @return true if successful, false if decode failed or invalid size +bool base64_decode_int32_vector(const std::string &base64, std::vector &out); + /// Size of buffer needed for base85 encoded int32 (5 chars + null terminator) static constexpr size_t BASE85_INT32_ENCODED_SIZE = 6; From f7ad324d81175881b3997833a110904d2df1ac0a Mon Sep 17 00:00:00 2001 From: Keith Burzinski Date: Fri, 16 Jan 2026 18:15:27 -0600 Subject: [PATCH 2/4] [infrared, remote_base] Replace `base85` with `base64url` for web server infrared transmissions (#13265) --- esphome/components/infrared/infrared.cpp | 27 ++++++++++++------- esphome/components/infrared/infrared.h | 24 ++++++++--------- .../components/remote_base/remote_base.cpp | 6 ++--- esphome/components/remote_base/remote_base.h | 8 +++--- 4 files changed, 36 insertions(+), 29 deletions(-) diff --git a/esphome/components/infrared/infrared.cpp b/esphome/components/infrared/infrared.cpp index 294d69e5233..44318699511 100644 --- a/esphome/components/infrared/infrared.cpp +++ b/esphome/components/infrared/infrared.cpp @@ -19,12 +19,12 @@ InfraredCall &InfraredCall::set_carrier_frequency(uint32_t frequency) { InfraredCall &InfraredCall::set_raw_timings(const std::vector &timings) { this->raw_timings_ = &timings; this->packed_data_ = nullptr; - this->base85_ptr_ = nullptr; + this->base64url_ptr_ = nullptr; return *this; } -InfraredCall &InfraredCall::set_raw_timings_base85(const std::string &base85) { - this->base85_ptr_ = &base85; +InfraredCall &InfraredCall::set_raw_timings_base64url(const std::string &base64url) { + this->base64url_ptr_ = &base64url; this->raw_timings_ = nullptr; this->packed_data_ = nullptr; return *this; @@ -35,7 +35,7 @@ InfraredCall &InfraredCall::set_raw_timings_packed(const uint8_t *data, uint16_t this->packed_length_ = length; this->packed_count_ = count; this->raw_timings_ = nullptr; - this->base85_ptr_ = nullptr; + this->base64url_ptr_ = nullptr; return *this; } @@ -101,13 +101,22 @@ void Infrared::control(const InfraredCall &call) { call.get_packed_count()); ESP_LOGD(TAG, "Transmitting packed raw timings: count=%u, repeat=%u", call.get_packed_count(), call.get_repeat_count()); - } else if (call.is_base85()) { - // Decode base85 directly into transmit buffer (zero heap allocations) - if (!transmit_data->set_data_from_base85(call.get_base85_data())) { - ESP_LOGE(TAG, "Invalid base85 data"); + } else if (call.is_base64url()) { + // Decode base64url (URL-safe) into transmit buffer + if (!transmit_data->set_data_from_base64url(call.get_base64url_data())) { + ESP_LOGE(TAG, "Invalid base64url data"); return; } - ESP_LOGD(TAG, "Transmitting base85 raw timings: count=%zu, repeat=%u", transmit_data->get_data().size(), + // Sanity check: validate timing values are within reasonable bounds + constexpr int32_t max_timing_us = 500000; // 500ms absolute max + for (int32_t timing : transmit_data->get_data()) { + int32_t abs_timing = timing < 0 ? -timing : timing; + if (abs_timing > max_timing_us) { + ESP_LOGE(TAG, "Invalid timing value: %d µs (max %d)", timing, max_timing_us); + return; + } + } + ESP_LOGD(TAG, "Transmitting base64url raw timings: count=%zu, repeat=%u", transmit_data->get_data().size(), call.get_repeat_count()); } else { // From vector (lambdas/automations) diff --git a/esphome/components/infrared/infrared.h b/esphome/components/infrared/infrared.h index ba426c9daa5..59535f499a0 100644 --- a/esphome/components/infrared/infrared.h +++ b/esphome/components/infrared/infrared.h @@ -40,11 +40,11 @@ class InfraredCall { /// @note Usage: Primarily for lambdas/automations where the vector is in scope. InfraredCall &set_raw_timings(const std::vector &timings); - /// Set the raw timings from base85-encoded int32 data + /// Set the raw timings from base64url-encoded little-endian int32 data /// @note Lifetime: Stores a pointer to the string. The string must outlive perform(). - /// @note Usage: For web_server where the encoded string is on the stack. + /// @note Usage: For web_server - base64url is fully URL-safe (uses '-' and '_'). /// @note Decoding happens at perform() time, directly into the transmit buffer. - InfraredCall &set_raw_timings_base85(const std::string &base85); + InfraredCall &set_raw_timings_base64url(const std::string &base64url); /// Set the raw timings from packed protobuf sint32 data (zigzag + varint encoded) /// @note Lifetime: Stores a pointer to the buffer. The buffer must outlive perform(). @@ -59,18 +59,18 @@ class InfraredCall { /// Get the carrier frequency const optional &get_carrier_frequency() const { return this->carrier_frequency_; } - /// Get the raw timings (only valid if set via set_raw_timings, not packed or base85) + /// Get the raw timings (only valid if set via set_raw_timings) const std::vector &get_raw_timings() const { return *this->raw_timings_; } - /// Check if raw timings have been set (vector, packed, or base85) + /// Check if raw timings have been set (any format) bool has_raw_timings() const { - return this->raw_timings_ != nullptr || this->packed_data_ != nullptr || this->base85_ptr_ != nullptr; + return this->raw_timings_ != nullptr || this->packed_data_ != nullptr || this->base64url_ptr_ != nullptr; } /// Check if using packed data format bool is_packed() const { return this->packed_data_ != nullptr; } - /// Check if using base85 data format - bool is_base85() const { return this->base85_ptr_ != nullptr; } - /// Get the base85 data string - const std::string &get_base85_data() const { return *this->base85_ptr_; } + /// Check if using base64url data format + bool is_base64url() const { return this->base64url_ptr_ != nullptr; } + /// Get the base64url data string + const std::string &get_base64url_data() const { return *this->base64url_ptr_; } /// Get packed data (only valid if set via set_raw_timings_packed) const uint8_t *get_packed_data() const { return this->packed_data_; } uint16_t get_packed_length() const { return this->packed_length_; } @@ -84,8 +84,8 @@ class InfraredCall { optional carrier_frequency_; // Pointer to vector-based timings (caller-owned, must outlive perform()) const std::vector *raw_timings_{nullptr}; - // Pointer to base85-encoded string (caller-owned, must outlive perform()) - const std::string *base85_ptr_{nullptr}; + // Pointer to base64url-encoded string (caller-owned, must outlive perform()) + const std::string *base64url_ptr_{nullptr}; // Pointer to packed protobuf buffer (caller-owned, must outlive perform()) const uint8_t *packed_data_{nullptr}; uint16_t packed_length_{0}; diff --git a/esphome/components/remote_base/remote_base.cpp b/esphome/components/remote_base/remote_base.cpp index 53c9c38c7d5..b4a549f0bed 100644 --- a/esphome/components/remote_base/remote_base.cpp +++ b/esphome/components/remote_base/remote_base.cpp @@ -2,8 +2,6 @@ #include "esphome/core/helpers.h" #include "esphome/core/log.h" -#include - namespace esphome { namespace remote_base { @@ -160,8 +158,8 @@ void RemoteTransmitData::set_data_from_packed_sint32(const uint8_t *data, size_t } } -bool RemoteTransmitData::set_data_from_base85(const std::string &base85) { - return base85_decode_int32_vector(base85, this->data_); +bool RemoteTransmitData::set_data_from_base64url(const std::string &base64url) { + return base64_decode_int32_vector(base64url, this->data_); } /* RemoteTransmitterBase */ diff --git a/esphome/components/remote_base/remote_base.h b/esphome/components/remote_base/remote_base.h index 2d7642cc31b..0cac28506fd 100644 --- a/esphome/components/remote_base/remote_base.h +++ b/esphome/components/remote_base/remote_base.h @@ -36,11 +36,11 @@ class RemoteTransmitData { /// @param len Length of the buffer in bytes /// @param count Number of values (for reserve optimization) void set_data_from_packed_sint32(const uint8_t *data, size_t len, size_t count); - /// Set data from base85-encoded int32 values - /// Decodes directly into internal buffer (zero heap allocations) - /// @param base85 Base85-encoded string (5 chars per int32 value) + /// Set data from base64url-encoded little-endian int32 values + /// Base64url is URL-safe: uses '-' instead of '+', '_' instead of '/' + /// @param base64url Base64url-encoded string of little-endian int32 values /// @return true if successful, false if decode failed or invalid size - bool set_data_from_base85(const std::string &base85); + bool set_data_from_base64url(const std::string &base64url); void reset() { this->data_.clear(); this->carrier_frequency_ = 0; From bcc8351d655fbd2ef3621521fa4f6cb545b9a8ac Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 16 Jan 2026 14:42:47 -1000 Subject: [PATCH 3/4] proto --- esphome/components/api/proto.cpp | 12 +- .../proto_bounds_check_overflow_noise.yaml | 11 + ...proto_bounds_check_overflow_plaintext.yaml | 9 + .../proto_fixed32_bounds_check_plaintext.yaml | 9 + .../test_proto_bounds_check_overflow.py | 278 ++++++++++++++++++ 5 files changed, 315 insertions(+), 4 deletions(-) create mode 100644 tests/integration/fixtures/proto_bounds_check_overflow_noise.yaml create mode 100644 tests/integration/fixtures/proto_bounds_check_overflow_plaintext.yaml create mode 100644 tests/integration/fixtures/proto_fixed32_bounds_check_plaintext.yaml create mode 100644 tests/integration/test_proto_bounds_check_overflow.py diff --git a/esphome/components/api/proto.cpp b/esphome/components/api/proto.cpp index eac26997cfc..945a192b923 100644 --- a/esphome/components/api/proto.cpp +++ b/esphome/components/api/proto.cpp @@ -48,14 +48,16 @@ uint32_t ProtoDecodableMessage::count_repeated_field(const uint8_t *buffer, size } uint32_t field_length = res->as_uint32(); ptr += consumed; - if (ptr + field_length > end) { + // Use subtraction to avoid integer overflow on 32-bit systems + if (field_length > static_cast(end - ptr)) { return count; // Out of bounds } ptr += field_length; break; } case WIRE_TYPE_FIXED32: { // 32-bit - skip 4 bytes - if (ptr + 4 > end) { + // Use subtraction to avoid integer overflow on 32-bit systems + if (static_cast(end - ptr) < 4) { return count; } ptr += 4; @@ -110,7 +112,8 @@ void ProtoDecodableMessage::decode(const uint8_t *buffer, size_t length) { } uint32_t field_length = res->as_uint32(); ptr += consumed; - if (ptr + field_length > end) { + // Use subtraction to avoid integer overflow on 32-bit systems + if (field_length > static_cast(end - ptr)) { ESP_LOGV(TAG, "Out-of-bounds Length Delimited at offset %ld", (long) (ptr - buffer)); return; } @@ -121,7 +124,8 @@ void ProtoDecodableMessage::decode(const uint8_t *buffer, size_t length) { break; } case WIRE_TYPE_FIXED32: { // 32-bit - if (ptr + 4 > end) { + // Use subtraction to avoid integer overflow on 32-bit systems + if (static_cast(end - ptr) < 4) { ESP_LOGV(TAG, "Out-of-bounds Fixed32-bit at offset %ld", (long) (ptr - buffer)); return; } diff --git a/tests/integration/fixtures/proto_bounds_check_overflow_noise.yaml b/tests/integration/fixtures/proto_bounds_check_overflow_noise.yaml new file mode 100644 index 00000000000..4c04d74d0d8 --- /dev/null +++ b/tests/integration/fixtures/proto_bounds_check_overflow_noise.yaml @@ -0,0 +1,11 @@ +esphome: + name: proto-overflow-noise + +host: + +api: + encryption: + key: "N4Yle5YirwZhPiHHsdZLdOA73ndj/84veVaLhTvxCuU=" + +logger: + level: VERY_VERBOSE diff --git a/tests/integration/fixtures/proto_bounds_check_overflow_plaintext.yaml b/tests/integration/fixtures/proto_bounds_check_overflow_plaintext.yaml new file mode 100644 index 00000000000..feb4bb57251 --- /dev/null +++ b/tests/integration/fixtures/proto_bounds_check_overflow_plaintext.yaml @@ -0,0 +1,9 @@ +esphome: + name: proto-overflow-plaintext + +host: + +api: + +logger: + level: VERY_VERBOSE diff --git a/tests/integration/fixtures/proto_fixed32_bounds_check_plaintext.yaml b/tests/integration/fixtures/proto_fixed32_bounds_check_plaintext.yaml new file mode 100644 index 00000000000..feb4bb57251 --- /dev/null +++ b/tests/integration/fixtures/proto_fixed32_bounds_check_plaintext.yaml @@ -0,0 +1,9 @@ +esphome: + name: proto-overflow-plaintext + +host: + +api: + +logger: + level: VERY_VERBOSE diff --git a/tests/integration/test_proto_bounds_check_overflow.py b/tests/integration/test_proto_bounds_check_overflow.py new file mode 100644 index 00000000000..11606965610 --- /dev/null +++ b/tests/integration/test_proto_bounds_check_overflow.py @@ -0,0 +1,278 @@ +"""Integration tests for protobuf bounds check integer overflow fix (GHSA-4h3h-63v6-88qx). + +This tests the fix for CVE where an integer overflow in the comparison +`ptr + field_length > end` could be bypassed by sending a large field_length value, +causing the device to crash by reading out-of-bounds memory. + +The fix changes the comparison to `field_length > static_cast(end - ptr)` +which avoids the overflow by comparing against the remaining buffer size directly. +""" + +from __future__ import annotations + +import asyncio +import contextlib +import socket + +import pytest + +from .const import LOCALHOST +from .types import APIClientConnectedWithDisconnectFactory, RunCompiledFunction + + +def _encode_varint(value: int) -> bytes: + """Encode an integer as a protobuf varint.""" + result = [] + while value > 127: + result.append((value & 0x7F) | 0x80) + value >>= 7 + result.append(value & 0x7F) + return bytes(result) + + +def _create_malicious_hello_request(field_length: int) -> bytes: + """Create a malicious HelloRequest packet with overflow-inducing field_length. + + The packet structure is: + - 0x00: Plaintext protocol indicator + - VarInt: Total message size + - 0x01: Message type (HelloRequest) + - 0x02: Field tag (field_id=0, wire_type=2 LENGTH_DELIMITED) + - VarInt: field_length (the malicious value) + + When field_length is large (e.g., 0xe0000000), on 32-bit systems the comparison + `ptr + field_length > end` would overflow, bypassing the bounds check. + """ + field_length_varint = _encode_varint(field_length) + # Message content: field tag (0x02) + field_length varint + message_content = bytes([0x02]) + field_length_varint + # Full message: message type (0x01) + content + full_message = bytes([0x01]) + message_content + # Size varint + size_varint = _encode_varint(len(full_message)) + # Complete packet: indicator (0x00) + size + message + return bytes([0x00]) + size_varint + full_message + + +def _send_malicious_packets_raw(host: str, port: int, packets: list[bytes]) -> None: + """Send malicious packets using a raw socket connection. + + This bypasses the aioesphomeapi client to send raw malformed data directly + to the ESPHome API server. + """ + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.settimeout(5.0) + try: + sock.connect((host, port)) + for packet in packets: + sock.sendall(packet) + except (TimeoutError, ConnectionResetError, BrokenPipeError): + # Expected - server may close connection after malformed packet + pass + finally: + sock.close() + + +@pytest.mark.asyncio +async def test_proto_bounds_check_overflow_plaintext( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected_with_disconnect: APIClientConnectedWithDisconnectFactory, + unused_tcp_port: int, +) -> None: + """Test that protobuf bounds check overflow doesn't crash the device (plaintext). + + This tests the fix for GHSA-4h3h-63v6-88qx where sending a HelloRequest + with a large field_length could cause an integer overflow in the bounds check, + leading to out-of-bounds memory access and device crash. + + The attack works by sending a packet where field_length is large enough that + `ptr + field_length` wraps around to a smaller value, bypassing the > end check. + """ + process_crashed = False + invalid_length_logged = False + + def check_logs(line: str) -> None: + nonlocal process_crashed, invalid_length_logged + # Check for signs that the process crashed + if "Segmentation fault" in line or "core dumped" in line: + process_crashed = True + # Check if the bounds check caught the malicious packet + if "Out-of-bounds Length Delimited" in line: + invalid_length_logged = True + + async with run_compiled(yaml_config, line_callback=check_logs): + # First verify the API is working normally + async with api_client_connected_with_disconnect() as (client, _): + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "proto-overflow-plaintext" + + # Now send malicious packets using raw socket + # Test with multiple field_length values that would cause overflow on 32-bit + # These values are chosen to cause ptr + field_length to wrap around + overflow_values = [ + 0xE0000000, # Causes crash on ESP32 and RPi Pico W + 0xD0000000, # Crashes ESP32 + 0xF0000000, # May not crash but reads unrelated memory + 0xFFFFFFFF, # Maximum uint32 value + ] + + malicious_packets = [ + _create_malicious_hello_request(val) for val in overflow_values + ] + + # Send malicious packets in executor to not block event loop + loop = asyncio.get_running_loop() + await loop.run_in_executor( + None, + _send_malicious_packets_raw, + LOCALHOST, + unused_tcp_port, + malicious_packets, + ) + + # Small delay to let ESPHome process the packets + await asyncio.sleep(0.5) + + # After the malicious packets, verify the process didn't crash + assert not process_crashed, ( + "ESPHome process crashed! The bounds check overflow fix is not working." + ) + + # Most importantly: verify we can reconnect, proving the process is still running + async with api_client_connected_with_disconnect() as (client2, _): + device_info = await client2.device_info() + assert device_info is not None + assert device_info.name == "proto-overflow-plaintext" + + +@pytest.mark.asyncio +async def test_proto_bounds_check_overflow_noise( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected_with_disconnect: APIClientConnectedWithDisconnectFactory, +) -> None: + """Test that protobuf bounds check overflow doesn't crash the device (noise encryption). + + With noise encryption, the attack requires knowledge of the encryption key. + This test verifies that even with a valid encryption session, malicious + protobuf content doesn't crash the device. + """ + noise_key = "N4Yle5YirwZhPiHHsdZLdOA73ndj/84veVaLhTvxCuU=" + process_crashed = False + + def check_logs(line: str) -> None: + nonlocal process_crashed + if "Segmentation fault" in line or "core dumped" in line: + process_crashed = True + + async with run_compiled(yaml_config, line_callback=check_logs): + async with api_client_connected_with_disconnect(noise_psk=noise_key) as ( + client, + disconnect_event, + ): + # Verify basic connection works first + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "proto-overflow-noise" + + # With noise encryption, we need to send through the frame helper + # which will encrypt the data. We'll send a message with a malformed + # protobuf body that has a large length-delimited field. + frame_helper = client._connection._frame_helper + + # Create a malformed protobuf body with overflow-inducing field length + # This is the content after encryption/decryption + # Tag 0x02 (field_id=0, wire_type=2) followed by large length + malformed_bodies = [ + bytes([0x02]) + _encode_varint(0xE0000000), # Overflow value + bytes([0x02]) + _encode_varint(0xFFFFFFFF), # Max uint32 + ] + + for body in malformed_bodies: + # Send as HelloRequest (type 1) + try: + frame_helper.write_packets([(1, body)], True) + except (ConnectionResetError, BrokenPipeError, OSError): + # Connection may be closed after malformed packet + break + await asyncio.sleep(0.1) + + # Wait briefly for any disconnect + with contextlib.suppress(TimeoutError): + await asyncio.wait_for(disconnect_event.wait(), timeout=1.0) + + # Verify process didn't crash + assert not process_crashed, ( + "ESPHome process crashed! The bounds check overflow fix is not working." + ) + + # Verify we can reconnect + async with api_client_connected_with_disconnect(noise_psk=noise_key) as ( + client2, + _, + ): + device_info = await client2.device_info() + assert device_info is not None + assert device_info.name == "proto-overflow-noise" + + +@pytest.mark.asyncio +async def test_proto_fixed32_bounds_check_plaintext( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected_with_disconnect: APIClientConnectedWithDisconnectFactory, + unused_tcp_port: int, +) -> None: + """Test that fixed32 bounds check works correctly. + + This tests the simpler case where we check if there are 4 bytes remaining. + While less likely to overflow, the fix ensures consistent bounds checking. + """ + process_crashed = False + + def check_logs(line: str) -> None: + nonlocal process_crashed + if "Segmentation fault" in line or "core dumped" in line: + process_crashed = True + + async with run_compiled(yaml_config, line_callback=check_logs): + # First verify the API is working normally + async with api_client_connected_with_disconnect() as (client, _): + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "proto-overflow-plaintext" + + # Create a packet with a fixed32 field (wire type 5) but truncated data + # Tag: field_id=1, wire_type=5 (fixed32) = (1 << 3) | 5 = 0x0D + # This should be caught by the bounds check + truncated_fixed32 = bytes( + [ + 0x00, # Plaintext indicator + 0x03, # Size (3 bytes of message) + 0x01, # Message type (HelloRequest) + 0x0D, # Field tag (field_id=1, wire_type=5 fixed32) + 0x42, # Only 1 byte of data instead of 4 + ] + ) + + # Send using raw socket + loop = asyncio.get_running_loop() + await loop.run_in_executor( + None, + _send_malicious_packets_raw, + LOCALHOST, + unused_tcp_port, + [truncated_fixed32], + ) + + await asyncio.sleep(0.5) + + assert not process_crashed, "ESPHome process crashed on truncated fixed32!" + + # Verify we can still reconnect + async with api_client_connected_with_disconnect() as (client2, _): + device_info = await client2.device_info() + assert device_info is not None + assert device_info.name == "proto-overflow-plaintext" From 20baa43aa2d93597f02c50c28b2bac6814715e86 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 16 Jan 2026 14:49:16 -1000 Subject: [PATCH 4/4] fix --- esphome/components/api/proto.cpp | 8 +- .../proto_bounds_check_overflow_noise.yaml | 11 - ...proto_bounds_check_overflow_plaintext.yaml | 9 - .../proto_fixed32_bounds_check_plaintext.yaml | 9 - .../test_proto_bounds_check_overflow.py | 278 ------------------ 5 files changed, 4 insertions(+), 311 deletions(-) delete mode 100644 tests/integration/fixtures/proto_bounds_check_overflow_noise.yaml delete mode 100644 tests/integration/fixtures/proto_bounds_check_overflow_plaintext.yaml delete mode 100644 tests/integration/fixtures/proto_fixed32_bounds_check_plaintext.yaml delete mode 100644 tests/integration/test_proto_bounds_check_overflow.py diff --git a/esphome/components/api/proto.cpp b/esphome/components/api/proto.cpp index 945a192b923..777fb358802 100644 --- a/esphome/components/api/proto.cpp +++ b/esphome/components/api/proto.cpp @@ -49,7 +49,7 @@ uint32_t ProtoDecodableMessage::count_repeated_field(const uint8_t *buffer, size uint32_t field_length = res->as_uint32(); ptr += consumed; // Use subtraction to avoid integer overflow on 32-bit systems - if (field_length > static_cast(end - ptr)) { + if (field_length > end - ptr) { return count; // Out of bounds } ptr += field_length; @@ -57,7 +57,7 @@ uint32_t ProtoDecodableMessage::count_repeated_field(const uint8_t *buffer, size } case WIRE_TYPE_FIXED32: { // 32-bit - skip 4 bytes // Use subtraction to avoid integer overflow on 32-bit systems - if (static_cast(end - ptr) < 4) { + if (end - ptr < 4) { return count; } ptr += 4; @@ -113,7 +113,7 @@ void ProtoDecodableMessage::decode(const uint8_t *buffer, size_t length) { uint32_t field_length = res->as_uint32(); ptr += consumed; // Use subtraction to avoid integer overflow on 32-bit systems - if (field_length > static_cast(end - ptr)) { + if (field_length > end - ptr) { ESP_LOGV(TAG, "Out-of-bounds Length Delimited at offset %ld", (long) (ptr - buffer)); return; } @@ -125,7 +125,7 @@ void ProtoDecodableMessage::decode(const uint8_t *buffer, size_t length) { } case WIRE_TYPE_FIXED32: { // 32-bit // Use subtraction to avoid integer overflow on 32-bit systems - if (static_cast(end - ptr) < 4) { + if (end - ptr < 4) { ESP_LOGV(TAG, "Out-of-bounds Fixed32-bit at offset %ld", (long) (ptr - buffer)); return; } diff --git a/tests/integration/fixtures/proto_bounds_check_overflow_noise.yaml b/tests/integration/fixtures/proto_bounds_check_overflow_noise.yaml deleted file mode 100644 index 4c04d74d0d8..00000000000 --- a/tests/integration/fixtures/proto_bounds_check_overflow_noise.yaml +++ /dev/null @@ -1,11 +0,0 @@ -esphome: - name: proto-overflow-noise - -host: - -api: - encryption: - key: "N4Yle5YirwZhPiHHsdZLdOA73ndj/84veVaLhTvxCuU=" - -logger: - level: VERY_VERBOSE diff --git a/tests/integration/fixtures/proto_bounds_check_overflow_plaintext.yaml b/tests/integration/fixtures/proto_bounds_check_overflow_plaintext.yaml deleted file mode 100644 index feb4bb57251..00000000000 --- a/tests/integration/fixtures/proto_bounds_check_overflow_plaintext.yaml +++ /dev/null @@ -1,9 +0,0 @@ -esphome: - name: proto-overflow-plaintext - -host: - -api: - -logger: - level: VERY_VERBOSE diff --git a/tests/integration/fixtures/proto_fixed32_bounds_check_plaintext.yaml b/tests/integration/fixtures/proto_fixed32_bounds_check_plaintext.yaml deleted file mode 100644 index feb4bb57251..00000000000 --- a/tests/integration/fixtures/proto_fixed32_bounds_check_plaintext.yaml +++ /dev/null @@ -1,9 +0,0 @@ -esphome: - name: proto-overflow-plaintext - -host: - -api: - -logger: - level: VERY_VERBOSE diff --git a/tests/integration/test_proto_bounds_check_overflow.py b/tests/integration/test_proto_bounds_check_overflow.py deleted file mode 100644 index 11606965610..00000000000 --- a/tests/integration/test_proto_bounds_check_overflow.py +++ /dev/null @@ -1,278 +0,0 @@ -"""Integration tests for protobuf bounds check integer overflow fix (GHSA-4h3h-63v6-88qx). - -This tests the fix for CVE where an integer overflow in the comparison -`ptr + field_length > end` could be bypassed by sending a large field_length value, -causing the device to crash by reading out-of-bounds memory. - -The fix changes the comparison to `field_length > static_cast(end - ptr)` -which avoids the overflow by comparing against the remaining buffer size directly. -""" - -from __future__ import annotations - -import asyncio -import contextlib -import socket - -import pytest - -from .const import LOCALHOST -from .types import APIClientConnectedWithDisconnectFactory, RunCompiledFunction - - -def _encode_varint(value: int) -> bytes: - """Encode an integer as a protobuf varint.""" - result = [] - while value > 127: - result.append((value & 0x7F) | 0x80) - value >>= 7 - result.append(value & 0x7F) - return bytes(result) - - -def _create_malicious_hello_request(field_length: int) -> bytes: - """Create a malicious HelloRequest packet with overflow-inducing field_length. - - The packet structure is: - - 0x00: Plaintext protocol indicator - - VarInt: Total message size - - 0x01: Message type (HelloRequest) - - 0x02: Field tag (field_id=0, wire_type=2 LENGTH_DELIMITED) - - VarInt: field_length (the malicious value) - - When field_length is large (e.g., 0xe0000000), on 32-bit systems the comparison - `ptr + field_length > end` would overflow, bypassing the bounds check. - """ - field_length_varint = _encode_varint(field_length) - # Message content: field tag (0x02) + field_length varint - message_content = bytes([0x02]) + field_length_varint - # Full message: message type (0x01) + content - full_message = bytes([0x01]) + message_content - # Size varint - size_varint = _encode_varint(len(full_message)) - # Complete packet: indicator (0x00) + size + message - return bytes([0x00]) + size_varint + full_message - - -def _send_malicious_packets_raw(host: str, port: int, packets: list[bytes]) -> None: - """Send malicious packets using a raw socket connection. - - This bypasses the aioesphomeapi client to send raw malformed data directly - to the ESPHome API server. - """ - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock.settimeout(5.0) - try: - sock.connect((host, port)) - for packet in packets: - sock.sendall(packet) - except (TimeoutError, ConnectionResetError, BrokenPipeError): - # Expected - server may close connection after malformed packet - pass - finally: - sock.close() - - -@pytest.mark.asyncio -async def test_proto_bounds_check_overflow_plaintext( - yaml_config: str, - run_compiled: RunCompiledFunction, - api_client_connected_with_disconnect: APIClientConnectedWithDisconnectFactory, - unused_tcp_port: int, -) -> None: - """Test that protobuf bounds check overflow doesn't crash the device (plaintext). - - This tests the fix for GHSA-4h3h-63v6-88qx where sending a HelloRequest - with a large field_length could cause an integer overflow in the bounds check, - leading to out-of-bounds memory access and device crash. - - The attack works by sending a packet where field_length is large enough that - `ptr + field_length` wraps around to a smaller value, bypassing the > end check. - """ - process_crashed = False - invalid_length_logged = False - - def check_logs(line: str) -> None: - nonlocal process_crashed, invalid_length_logged - # Check for signs that the process crashed - if "Segmentation fault" in line or "core dumped" in line: - process_crashed = True - # Check if the bounds check caught the malicious packet - if "Out-of-bounds Length Delimited" in line: - invalid_length_logged = True - - async with run_compiled(yaml_config, line_callback=check_logs): - # First verify the API is working normally - async with api_client_connected_with_disconnect() as (client, _): - device_info = await client.device_info() - assert device_info is not None - assert device_info.name == "proto-overflow-plaintext" - - # Now send malicious packets using raw socket - # Test with multiple field_length values that would cause overflow on 32-bit - # These values are chosen to cause ptr + field_length to wrap around - overflow_values = [ - 0xE0000000, # Causes crash on ESP32 and RPi Pico W - 0xD0000000, # Crashes ESP32 - 0xF0000000, # May not crash but reads unrelated memory - 0xFFFFFFFF, # Maximum uint32 value - ] - - malicious_packets = [ - _create_malicious_hello_request(val) for val in overflow_values - ] - - # Send malicious packets in executor to not block event loop - loop = asyncio.get_running_loop() - await loop.run_in_executor( - None, - _send_malicious_packets_raw, - LOCALHOST, - unused_tcp_port, - malicious_packets, - ) - - # Small delay to let ESPHome process the packets - await asyncio.sleep(0.5) - - # After the malicious packets, verify the process didn't crash - assert not process_crashed, ( - "ESPHome process crashed! The bounds check overflow fix is not working." - ) - - # Most importantly: verify we can reconnect, proving the process is still running - async with api_client_connected_with_disconnect() as (client2, _): - device_info = await client2.device_info() - assert device_info is not None - assert device_info.name == "proto-overflow-plaintext" - - -@pytest.mark.asyncio -async def test_proto_bounds_check_overflow_noise( - yaml_config: str, - run_compiled: RunCompiledFunction, - api_client_connected_with_disconnect: APIClientConnectedWithDisconnectFactory, -) -> None: - """Test that protobuf bounds check overflow doesn't crash the device (noise encryption). - - With noise encryption, the attack requires knowledge of the encryption key. - This test verifies that even with a valid encryption session, malicious - protobuf content doesn't crash the device. - """ - noise_key = "N4Yle5YirwZhPiHHsdZLdOA73ndj/84veVaLhTvxCuU=" - process_crashed = False - - def check_logs(line: str) -> None: - nonlocal process_crashed - if "Segmentation fault" in line or "core dumped" in line: - process_crashed = True - - async with run_compiled(yaml_config, line_callback=check_logs): - async with api_client_connected_with_disconnect(noise_psk=noise_key) as ( - client, - disconnect_event, - ): - # Verify basic connection works first - device_info = await client.device_info() - assert device_info is not None - assert device_info.name == "proto-overflow-noise" - - # With noise encryption, we need to send through the frame helper - # which will encrypt the data. We'll send a message with a malformed - # protobuf body that has a large length-delimited field. - frame_helper = client._connection._frame_helper - - # Create a malformed protobuf body with overflow-inducing field length - # This is the content after encryption/decryption - # Tag 0x02 (field_id=0, wire_type=2) followed by large length - malformed_bodies = [ - bytes([0x02]) + _encode_varint(0xE0000000), # Overflow value - bytes([0x02]) + _encode_varint(0xFFFFFFFF), # Max uint32 - ] - - for body in malformed_bodies: - # Send as HelloRequest (type 1) - try: - frame_helper.write_packets([(1, body)], True) - except (ConnectionResetError, BrokenPipeError, OSError): - # Connection may be closed after malformed packet - break - await asyncio.sleep(0.1) - - # Wait briefly for any disconnect - with contextlib.suppress(TimeoutError): - await asyncio.wait_for(disconnect_event.wait(), timeout=1.0) - - # Verify process didn't crash - assert not process_crashed, ( - "ESPHome process crashed! The bounds check overflow fix is not working." - ) - - # Verify we can reconnect - async with api_client_connected_with_disconnect(noise_psk=noise_key) as ( - client2, - _, - ): - device_info = await client2.device_info() - assert device_info is not None - assert device_info.name == "proto-overflow-noise" - - -@pytest.mark.asyncio -async def test_proto_fixed32_bounds_check_plaintext( - yaml_config: str, - run_compiled: RunCompiledFunction, - api_client_connected_with_disconnect: APIClientConnectedWithDisconnectFactory, - unused_tcp_port: int, -) -> None: - """Test that fixed32 bounds check works correctly. - - This tests the simpler case where we check if there are 4 bytes remaining. - While less likely to overflow, the fix ensures consistent bounds checking. - """ - process_crashed = False - - def check_logs(line: str) -> None: - nonlocal process_crashed - if "Segmentation fault" in line or "core dumped" in line: - process_crashed = True - - async with run_compiled(yaml_config, line_callback=check_logs): - # First verify the API is working normally - async with api_client_connected_with_disconnect() as (client, _): - device_info = await client.device_info() - assert device_info is not None - assert device_info.name == "proto-overflow-plaintext" - - # Create a packet with a fixed32 field (wire type 5) but truncated data - # Tag: field_id=1, wire_type=5 (fixed32) = (1 << 3) | 5 = 0x0D - # This should be caught by the bounds check - truncated_fixed32 = bytes( - [ - 0x00, # Plaintext indicator - 0x03, # Size (3 bytes of message) - 0x01, # Message type (HelloRequest) - 0x0D, # Field tag (field_id=1, wire_type=5 fixed32) - 0x42, # Only 1 byte of data instead of 4 - ] - ) - - # Send using raw socket - loop = asyncio.get_running_loop() - await loop.run_in_executor( - None, - _send_malicious_packets_raw, - LOCALHOST, - unused_tcp_port, - [truncated_fixed32], - ) - - await asyncio.sleep(0.5) - - assert not process_crashed, "ESPHome process crashed on truncated fixed32!" - - # Verify we can still reconnect - async with api_client_connected_with_disconnect() as (client2, _): - device_info = await client2.device_info() - assert device_info is not None - assert device_info.name == "proto-overflow-plaintext"