[siracozmen01]

Problem

On X11, the library read the lists and strings in the X server's replies and events by the counts the server put beside them, and never checked those counts against the bytes the reply or event held. libxcb sizes a reply's buffer by its length field alone (32 bytes and 4 per unit, in read_packet), and its accessors find each part from fields such as value_len, num_items, nOutput or name_len. A server that sent a count larger than what followed made the library read past the end of that buffer. A property's value_len * format / 8 could also pass INT_MAX, and src/x11_drop.c cast the negative int that xcb_get_property_value_length then returns to size_t (#496).

While there: ReadDevices in src/x11_input.c called xcb_input_xi_query_device_infos_iterator on the reply before checking it for NULL, so a failed XIQueryDevice crashed. The message box took the first screen of the setup without checking that the setup has one.

Change

• src/x11_reply.c holds one check, mwinX11Holds: whether count items of size bytes at a part lie within the bytes held from the reply's start, with arithmetic that cannot wrap. The bytes held are 32 and 4 per unit of length for a reply, the same after the full sequence libxcb adds for a generic event, and 8 and 4 per unit for the setup.
• Every reply is checked as it arrives, by the family of the parts the backend reads in it. A reply with a part past its bytes, or a property of a format no property has, is released and taken as a request the server refused: each caller's path for a NULL reply runs. A malformed XInput event is passed over. A setup that does not hold the screen whole gives no screen: the backend fails to start, and the message box answers mwin_errorPlatform.
• libxcb's iterators step an XInput device class by sizes they reckon from the class's own fields, such as a key class's num_keys, not from the class's len, so a class whose fields disagree with its length, or one of a type libxcb does not know, sends them past the reply even where every length is right. The device infos and their classes are now walked by the length each gives, as the protocol defines it. Each class must be as long as the fields of its type the backend reads (valuator, scroll, touch). The six accessors and steps of libxcb that walk them, and xcb_screen_next, are no longer loaded.
• A property's and an output property's size is counted in size_t from the count and the format the reply gives (mwinX11PropertyBytes, mwinX11OutputPropertyBytes), never through libxcb's xcb_get_property_value_length and xcb_randr_get_output_property_data_length, which return an int, and a value of more than INT_MAX bytes is refused with the rest of a malformed reply. Before, a reply that claimed 2 GiB or more passed the bounds check and its length became a negative int, which src/x11_drop.c turned into a huge size_t for mwinGatherUriList, and the EDID path the same. Both accessors are gone from the function table.
• The message box's QueryFont reply is checked too (mwinX11CheckFont): it must hold its fields whole, up to the ascent and descent at bytes 52 to 55, so a reply of no length is no longer read past its 32 bytes.
• test_x11_reply is a white-box test, which a shared build does not make, so the lines that set its include directories and skip code are kept to builds that have it.
• docs/internals/x11.md says how replies are read.

The 17 accessors that give a part's count, and the font reply's fields read past its fixed part, with their uses at 8d02e25, and how each is read now:

Accessor	Uses	Read now
xcb_get_property_value_length	src/backend_x11.c:165, src/backend_x11.c:177, src/x11_clipboard_read.c:221, src/x11_drop.c:68, src/x11_drop.c:289, src/x11_window.c:665	mwinX11CheckProperty on every GetProperty reply, the size from mwinX11PropertyBytes; the accessor is no longer used
xcb_get_atom_name_name_length	src/x11_output.c:24	mwinX11CheckAtomName
xcb_randr_get_output_property_data_length	src/x11_output.c:132 (the EDID), src/x11_output.c:145	mwinX11CheckOutputProperty, the size from mwinX11OutputPropertyBytes; the accessor is no longer used
xcb_randr_get_screen_resources_current_modes_length	src/x11_output.c:109	mwinX11CheckResources
xcb_randr_get_monitors_monitors_iterator	src/x11_output.c:209, src/x11_output.c:215	mwinX11CheckMonitors, each monitor before its step
xcb_randr_monitor_info_outputs_length	src/x11_output.c:234	the same
xcb_render_query_pict_formats_formats_length	src/x11_cursor.c:129	mwinX11CheckFormats
xcb_input_list_input_devices_devices_length	src/x11_input.c:310	mwinX11CheckInputDevices
xcb_input_xi_query_device_infos_iterator	src/x11_input.c:405	mwinX11CheckDevices, then mwinX11DevicesOf
xcb_input_xi_device_info_classes_iterator	src/x11_input.c:341, src/x11_input.c:366, src/x11_input.c:380	mwinX11CheckDevices, then mwinX11ClassesOf
xcb_input_xi_device_info_name_length	src/x11_input.c:362	mwinX11CheckDevices, then name_len
xcb_input_button_press_valuator_mask_length	src/x11_input.c:560, src/x11_input.c:584, src/x11_input.c:658	mwinX11XiEventWhole on every XInput event
xcb_input_button_press_axisvalues_length	src/x11_input.c:561, src/x11_input.c:585, src/x11_input.c:659	the same
xcb_input_raw_button_press_valuator_mask_length	src/x11_input.c:512	the same
xcb_input_raw_button_press_axisvalues_raw_length	src/x11_input.c:514	the same
xcb_setup_roots_iterator	src/backend_x11.c:244, src/x11_message_box.c:284	mwinX11ScreenOf
xcb_query_font_reply_t's font_ascent and font_descent, past its fixed part	src/x11_message_box.c:247, src/x11_message_box.c:248	mwinX11CheckFont
xcb_get_maximum_request_length	src/x11_icon.c:30	unchanged: a number the backend compares its own requests with, not a part of any buffer

Verification

• test/test_x11_reply.c loads libxcb's own accessors through mwinLoadX11, which needs no display, and hands the checks replies, events and setups built by hand. For each family (a property, an atom's name, an output property, the screen resources' modes, the monitors and their outputs, the picture formats, the input devices, XIQueryDevice's infos, names and classes, device and raw XInput events, and the setup's screens, depths and visuals), it builds one whose parts fit and one whose count passes its bytes, and checks that only the first is kept. It also covers a property count past INT_MAX, a format no property has, a class shorter than its type's fields or of no length, and the walk past a class of a type libxcb does not know. Under ASan and UBSan, a malformed reply's release is seen by the leak check, and a read past a buffer would be seen too: an early version of the device check that read a missing info's name length was caught that way.
• linux-clang-20-asan builds the library and test_x11_reply with no error and no warning, and x11_reply passes.
• test_x11_reply also builds, for each property kind, a short reply whose length field claims 2 GiB, so that it passes the bounds check, with a value of 2 GiB. It also builds QueryFont replies of 32 and 52 bytes. With the INT_MAX refusal taken out of src/x11_reply.c, the two property checks fail and nothing else; with mwinX11CheckFont passing every reply, the two short font checks fail and nothing else. With both in place, x11_reply passes.
• At 2d8cfe8, every tree builds with no error: linux-gcc-14 and web-wasi in the CI image, linux-clang-20-asan, linux-shared (whose configure failed CI's checks job at 2193dd3), linux-minimal, the fuzz tree, MinGW (through clang-20) and android-emulator.
• tools/check.sh pr passed in the CI image at 2d8cfe8: every test, among them the 21 X11 tests that drive the build's own X server, clang-tidy clean on src/x11_reply.c, and 10 seconds of every fuzz target.
• fuzz_x11_selection and fuzz_x11_drop ran 2 minutes each from their committed seeds at 2d8cfe8 with no finding (about 101,000 and 105,000 runs), after 5 minutes each with no finding at 2193dd3.

Closes #496

[siracozmen01]

Review at 2193dd3: changes needed (2). I read the code and did not run it. CI was still pending.

1. The message box still reads a QueryFont reply past the bytes libxcb holds. Measure in src/x11_message_box.c (line 243 at this head) takes api->queryFontReply(...) unchecked and reads metrics->font_ascent and metrics->font_descent. They sit at bytes 52 to 55 of xcb_query_font_reply_t. libxcb holds 32 bytes and 4 per unit of the reply's length, whatever the request was.

• Concrete case: a server answers QueryFont with length 0. libxcb hands over a 32-byte buffer, and Show reads 20 to 24 bytes past it.
• max_bounds.character_width at bytes 28 and 29 is within the 32.
• This is the same fault as #496, and docs/internals/x11.md now says "every reply and event is read through x11_reply.c first".
• A check that the reply holds sizeof(xcb_query_font_reply_t) closes it, released and taken as refused like the others; the box already falls back to 11 and 2. Please add a case for it to test_x11_reply.
• The other fixed-size replies the backend reads (InternAtom, TranslateCoordinates, GrabPointer, GetSelectionOwner, the version queries, PerClientFlags, RandR's CRTC info and output info) only use fields within the first 32 bytes, so they are safe as they are.

2. A property or output property of 2 GiB or more is kept, and its length still turns negative. mwinX11CheckProperty and mwinX11CheckOutputProperty check the value against the bytes held, which is right. They do not check that value_len * format / 8 fits the int that xcb_get_property_value_length and xcb_randr_get_output_property_data_length return.

• Concrete case: a server answers the drop's GetProperty with format 8, value_len 0x80000000 and a length of 0x20000000 units, so it really sends 2 GiB.
• The check keeps it. getPropertyValueLength returns INT_MIN. Take in src/x11_drop.c:292 casts that to size_t, about 1.8e19. mwinGatherUriList then scans the 2 GiB buffer by that length (memchr(list + start, '\n', length - start)) and reads past it when there is no newline.
• The EDID path does the same through (size_t)api->randrOutputPropertyDataLength(edid).
• The test "a property's count past INT_MAX bytes" passes for another reason: its reply holds 4 bytes, so it is refused as short, never for the INT_MAX count. The body names this case as fixed.
• Refuse a value of more than INT_MAX bytes in both checks. Then make the test's reply claim enough length (set length, no need to allocate it), so that only the new check refuses it.

What you asked about.

• Every count read through a check. Every reply with a list or a string the backend reads now passes a check before use: GetProperty in all six places, AtomName, OutputProperty, the screen resources, the monitors, the picture formats, ListInputDevices and XIQueryDevice. So do the XInput events OnXi handles. RAW_MOTION is checked through the raw values, which lie past the mask and the accelerated values. The button, motion and touch events are checked through their axis values. DEVICE_CHANGED's reason is within its fixed part. HIERARCHY reads no fields. The setup goes through mwinX11ScreenOf. The libxcb device and class walkers and xcb_screen_next are no longer loaded, and the walks go by checked lengths. A class of length 0 is refused (len * 4 >= FieldsOf(type)), so the walk always moves on. Finding 1 is the one read left outside.
• Arithmetic. mwinX11Holds compares count <= left / size after checking at - from <= holds, so it cannot wrap. That is the same bound ckd_mul gives. The byte counts are uint32_t times 4 in uint64_t, and the other sums are uint16_t and uint8_t counts times struct sizes, far from any limit.
• Refused, not asserted. Every check releases the reply and returns NULL, or passes the event over, and each caller's NULL path runs. There is no assert on server data. ReadDevices no longer touches a NULL reply. The message box answers mwin_errorPlatform for a setup without a whole first screen.
• Merge and body. Against main, CMakeLists.txt only adds the x11_reply test. The body has Problem, Change and Verification, and no line starts at column 0 with a name and (.

[siracozmen01]

Re-review at c9ea642: ready on the code. One CI failure is unexplained, below. I read the code and did not run it.

Both findings are fixed.

• QueryFont. mwinX11CheckFont keeps a reply only when it holds the whole xcb_query_font_reply_t (InReply(reply, reply, 1, sizeof(*reply)), 60 bytes), and Measure takes the reply through it, so font_ascent and font_descent are always within the bytes held. A refused reply falls back to 6, 11 and 2 as before. The test covers a 60-byte reply (kept, ascent read back), a 32-byte one, one of 52 that ends before the ascent, and NULL.
• Property sizes. FitsInt refuses count * format / 8 past INT_MAX, with ckd_mul in size_t, which a 32-bit target also refuses on overflow. Both checks run it before InReply. Every caller now takes its size from mwinX11PropertyBytes or mwinX11OutputPropertyBytes: drop's type list and data, _NET_WM_STATE, the clipboard read, Xft.dpi, the WM check, the EDID and VRR. The int accessors are no longer loaded, so a call left behind would not compile. The loops in drop and ReadMode now count in size_t. The clipboard's (uint32_t) cast of a size of at most INT_MAX is exact.
• The tests refuse for the right reason now. Huge() claims 2 GiB in the length field, so InReply holds the value, and only FitsInt refuses: format 32 with 0x20000000 items, and format 8 with 0x80000000. The old short-reply case stays, renamed "past its bytes".
• Every other getPropertyReply in the X11 sources goes through mwinX11CheckProperty. x11_clipboard.c, x11_chrome.c and x11_resources.c read only fixed-size replies or none.
• if(TARGET test_x11_reply) covers the shared build, where maul_add_test(... WHITEBOX) makes no target.

CI. ios-simulator failed after 43 s in run 42483229020 (pull_request, this head). Its log is not available until the run ends, so I could not read why. Nothing in this pull request builds for iOS, but please check that it is the runner and not the CMake change before this merges.