Skip to content

protocol/routing_info_entry: bounds-check the address/port memcpys#1040

Open
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/routing-info-entry-bounds
Open

protocol/routing_info_entry: bounds-check the address/port memcpys#1040
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/routing-info-entry-bounds

Conversation

@SoundMatt
Copy link
Copy Markdown

Problem

routing_info_entry::deserialize (implementation/protocol/src/routing_info_entry.cpp) had two missing bounds checks that let a peer routing client read up to ~22 bytes of adjacent heap memory off the receiving process by sending a malformed routing_info_command.

The function ends up being called from routing_manager_client::on_routing_info (and the routing-stub side), so the trust boundary here is the local IPC routing socket — which under the default security policy is bounded but is the same surface that vsomeip's security layer is supposed to protect.

Bug 1 — initial size check missed sizeof(its_client_size)

if (_buffer.size() < _index + sizeof(type_) + sizeof(its_size) + sizeof(client_)) {
    _error = error_e::ERROR_NOT_ENOUGH_BYTES;
    return;
}

That check covers 1 + 4 + 2 = 7 bytes, but the function actually consumes 1 + 4 + 4 + 2 = 11 bytes from _buffer at the top (the its_client_size read at line 102 isn't in the check). A peer that writes a routing_info_command whose payload is exactly 7 bytes drives the memcpy(&its_client_size, ...) to read 2 bytes past the end of the buffer and the memcpy(&client_, ...) to read another 2 bytes past — 4 bytes of OOB read, into adjacent heap memory of the routing manager's std::vector<byte_t>.

Bug 2 — address/port memcpys have no _buffer.size() check

if (its_client_size > sizeof(client_)) {
    uint32_t its_address_size = its_client_size - uint32_t(sizeof(client_t) + sizeof(port_));
    if (its_address_size == sizeof(boost::asio::ip::address_v4::bytes_type)) {
        ...
        std::memcpy(&its_array, &_buffer[_index], its_array.size());   // 4 bytes, no bounds check
        ...
    } else if (its_address_size == sizeof(boost::asio::ip::address_v6::bytes_type)) {
        ...
        std::memcpy(&its_array, &_buffer[_index], its_array.size());   // 16 bytes, no bounds check
        ...
    }
    std::memcpy(&port_, &_buffer[_index], sizeof(port_));               // 2 bytes, no bounds check
}

its_client_size is wire-controlled. A peer that sets it to 8 selects the v4 branch (4-byte address read) and a peer that sets it to 20 selects the v6 branch (16-byte address read), in both cases plus a 2-byte port read — all unguarded. Total OOB read primitive: 6, 12, or 18 bytes depending on which combination is reached.

The subtraction its_client_size - (sizeof(client_t) + sizeof(port_)) also underflows when its_client_size < 4, producing a near-UINT32_MAX its_address_size. The existing else { ERROR_MALFORMED } arm catches the underflow case incidentally, but the underflow itself shouldn't have been reachable in the first place.

Fix

  • Extend the initial size check to include sizeof(its_client_size) so it actually covers the 11 bytes read at the top.
  • Reject its_client_size values that would underflow the its_address_size subtraction.
  • Validate its_address_size against the two recognised IP families (v4=4, v6=16) before the bounds check, so the bounds check below can rely on a sane value.
  • Bounds-check its_address_size + sizeof(port_) against the remaining buffer before any memcpy.
  • Restructure the v4 / else-if-v6 / else-MALFORMED dispatch into v4 / else-v6 (the validation above has already rejected anything that isn't one of the two).

Diff: +41 / −7 in a single file.

Notes

  • The trailing services-array section (lines 145+ in the original) was already properly bounds-checked; not touched.
  • I haven't been able to run vsomeip's full test suite locally on macOS (boost+CMake setup pending). Pointers to the canonical way to add a malformed-input regression test for this entry would be appreciated; I'd be happy to follow up with a test commit on this branch once I have the build working.
  • This was found via a static audit pass over implementation/protocol/, implementation/service_discovery/, and implementation/endpoints/. There are a small number of further findings I'd like to bring up in separate PRs (e.g. message_impl::deserialize underflows header_.length_ - VSOMEIP_SOMEIP_HEADER_SIZE and passes the result to data_.reserve() — a ~4 GB allocation per malformed message). Happy to file those one at a time after this lands.

routing_info_entry::deserialize had two missing bounds checks that
let a peer routing client read up to ~22 bytes of adjacent heap
memory off the receiving process by sending a malformed
routing_info_command:

1. The initial _buffer.size() check covered sizeof(type_) +
   sizeof(its_size) + sizeof(client_) = 7 bytes, but the function
   actually consumes 1 + 4 + 4 + 2 = 11 bytes from _buffer at the
   top (it was missing sizeof(its_client_size) from the check).
   Send a routing_info_command whose payload is exactly 7 bytes
   and the memcpy()s for its_client_size and client_ read 4 + 2
   bytes past the end of the std::vector.

2. Inside 'if (its_client_size > sizeof(client_))', the v4/v6
   address memcpy and the trailing port memcpy ran with no
   _buffer.size() check at all. its_client_size is wire-controlled,
   so a peer setting it to 8 (v4) or 20 (v6) drives memcpy() to
   read 4 or 16 bytes plus 2 more for the port from past the
   buffer end.

Fix:
- Extend the initial size check to include sizeof(its_client_size).
- Reject its_client_size values that would underflow the
  its_address_size subtraction below.
- Validate its_address_size is a recognised IP family (v4=4,
  v6=16) before doing the bounds check, so the bounds check below
  can rely on a sane value.
- Bounds-check 'its_address_size + sizeof(port_)' against the
  remaining buffer before any memcpy.
- Restructure the v4/v6/else-MALFORMED dispatch into v4/else-v6
  since the validation above has already rejected anything that
  isn't one of the two.

Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant