Skip to content

STRING_ARRAY_FIELD pretty-printing lacks the STRING_FIELD bounds check (out-of-bounds read) #41671

Description

Summary

The PrettyPrint hardening from #41664 (commit 6ab1ac6, "Fix potential out of bound access when pretty-printing string field in init messages") bounds-checked STRING_FIELD via PrettyPrintSafeStringView, and #41637 (d49f455) converted the count-driven string walks to ArrayFromSpan. Both shipped in stable 2.9.13. The remaining string macro, STRING_ARRAY_FIELD, was not covered: the StringArray printer still performs unchecked pointer arithmetic with the message-supplied offset, so a message whose array offset lies past the end of the message causes an out-of-bounds read (and potentially a crash) in the receiving Windows process when the message is pretty-printed.

Location

Macro — src/shared/inc/prettyprintshared.h:36:

#define STRING_ARRAY_FIELD(Name) #Name, (StringArray((char*)(this), Name, Header.MessageSize))

(STRING_FIELD two lines above is the hardened counterpart.)

Printer — the StringArray branch in the same header (~lines 120-131):

if (Value.Index <= 0)
{
    Out << "<empty>";
    return;
}

gsl::span<const char> span(Value.MessageHead + Value.Index, Value.MessageHead + Value.MessageSize);
Out << wsl::shared::string::Join(wsl::shared::string::ArrayFromSpan(gsl::as_bytes(span)), ',');

There is no check that Value.Index (the message-supplied offset) is within Value.MessageSize before the span is constructed.

Mechanism

Value.Index comes from the message itself. When Index > MessageSize, last - first for the dynamic gsl::span(first, last) constructor is negative. WSL pins GSL v4.0.0 via FetchContent (CMakeLists.txt) with no GSL_* contract macros; that constructor is noexcept and computes the extent as narrow_cast<std::size_t>(lastElem - firstElem), so the negative difference wraps to ~SIZE_MAX rather than trapping.

ArrayFromSpan (src/shared/inc/stringshared.h, ~lines 370-407) then walks that span: it reads an int32_t length at MessageHead + Index — an out-of-bounds read at a message-chosen offset up to 4 GiB past the message buffer — and on any non-negative value constructs std::string(begin, size), copying further out-of-bounds memory into the pretty-print output until it reaches an unmapped page (crash), a -1 length terminator, or a length_error throw.

Reachability (receive path, Windows)

WSLC_LISTDIR_RESULT (src/shared/inc/lxinitshared.h, ~lines 1665-1678) is built by the guest and received by the host:

MESSAGE_HEADER Header;
int Result{};
unsigned int EntriesIndex{};
char Buffer[];

PRETTY_PRINT(FIELD(Header), FIELD(Result), STRING_ARRAY_FIELD(EntriesIndex));
  • Host call site: WSLCVirtualMachine::ListDirectory (src/windows/wslcsession/WSLCVirtualMachine.cpp, ~lines 674-685) -> Transaction<WSLC_LISTDIR> -> SocketChannel::ReceiveMessage.
  • ReceiveMessage (src/shared/inc/SocketChannel.h, ~lines 496-506) pretty-prints every received message on WIN32: WSL_LOG("ReceivedMessage", ..., TraceLoggingValue(message->PrettyPrint().c_str(), "Content")). WSL_LOG is a TraceLoggingWrite wrapper, so PrettyPrint() is evaluated on every receive regardless of whether an ETW listener is enabled.
  • The receive sizing in socketshared.h guarantees the received buffer extent equals Header.MessageSize; it does not constrain EntriesIndex, so a guest controlling the response can send MessageSize as the real size while setting EntriesIndex beyond it (e.g. MessageSize + 0x1000).
  • The data path is not affected: ListDirectory's own ArrayFromSpan(responseSpan, response.EntriesIndex) is bounds-checked — only the diagnostic path is unsafe. The receiving process is wslcsession.exe, which runs as the user (not SYSTEM).

Repro sketch (unit level)

Compile a small TU including prettyprintshared.h with GSL v4.0.0:

  1. Allocate a 64-byte WSLC_LISTDIR_RESULT-shaped buffer; set Header.MessageSize = 64, EntriesIndex = 64 + 0x1000; call PrettyPrint() -> out-of-bounds access at buffer + 0x1060 (or a wild std::string construction if that address happens to be mapped).
  2. Controls: EntriesIndex = 0 renders <empty>; an in-bounds length-prefixed array renders its entries.
  3. Family control: the same lying offset through a STRING_FIELD renders <out-of-bounds> — the 6ab1ac6 fix holds; only STRING_ARRAY_FIELD misbehaves.

Suggested fix

Mirror the PrettyPrintSafeStringView pattern for the array case, before any pointer arithmetic:

if (Value.Index == 0) { Out << "<empty>"; return; }
if (Value.Index >= Value.MessageSize) { Out << "<out-of-bounds>"; return; }

One-file change in the same style as the STRING_FIELD fix, coverable by a unit test alongside PrettyPrintOutOfBoundsFields in test/windows/UnitTests.cpp.

Observed at commit 535b16d; also present in the 2.9.13 tag.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions