SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.6k stars 3.19k forks source link

LibIPC: Servers can be easily forced to OOM #18864

Open BenWiederhake opened 1 year ago

BenWiederhake commented 1 year ago

Sending gigabytes of 0xFF bytes to any LibIPC server will cause them to OOM.

Specifically, Connection::try_parse_messages in Libraries/LibIPC/Connection.h:135 will happy accept any u32 as valid value of message_size, and read until the the internal buffer has that size. If the first four bytes are 0xFFFFFFFF, then it will try to read 4 GiB, which in default configuration causes an OOM:

184467441975.686 [#0 LookupServer(13:13)]: MM: Clean inode release saved the day! Released 1 pages from InodeVMObject
184467441975.686 [#0 LookupServer(13:13)]: MM: couldn't find free physical page. Continuing...
184467441975.686 [#0 LookupServer(13:13)]: MM: Clean inode release saved the day! Released 1 pages from InodeVMObject
184467441975.686 [#0 LookupServer(13:13)]: MM: couldn't find free physical page. Continuing...
184467441975.686 [LookupServer(13:13)]: MM: no physical pages available
184467441975.686 [#0 LookupServer(13:13)]: MM: Unable to allocate page table to map V0x000000077a0a0000
184467441981.114 LookupServer(13:13): ASSERTION FAILED: !_temporary_result.is_error()
././AK/Vector.h:274
184467441981.114 [#0 LookupServer(13:13)]: Terminating LookupServer(13) due to signal 6

Since we already do the "heavy lifting" through shared bitmaps or anonymous files, I don't see a reason to allow arbitrarily large messages. I tried out most programs (including browsing through every bookmarked webpage), and the largest message I saw was 15 KiB. Except for HackStudio, apparently we regularly send large messages to the LanguageServer, where messages with 68067 bytes are "normal". Hmm, maybe we should change that. For LibWeb/CSS/Parser/Parser.cpp (329379 bytes), it sends 224468 bytes. For the ultra-heavy file Build/x86_64/Userland/Libraries/LibLocale/LocaleData.cpp (15076548 bytes) it OOMs immediately, but I'm going to assume it sends linearly much more than that.

I think we should introduce an upper limit on message sizes; something like 5 MiB sounds good. What do you think?

Note that it is fundamentally impossible to fully prevent this from happening. After all, an actively malicious user could simply for a bunch of times, and send "limit-as-defined-above minus one" bytes.

Code and screenshots ``` /* * Copyright (c) 2023, Ben Wiederhake * * SPDX-License-Identifier: BSD-2-Clause */ // Save as: // /Userland/Utilities/evil.cpp // Add to CMakeLists.txt: // target_link_libraries(evil PRIVATE LibCore) // Run inside serenity: // $ evil /tmp/portal/lookup #include #include #include ErrorOr serenity_main(Main::Arguments args) { if (args.strings.size() != 2 || args.strings[1].starts_with('-')) { warnln("USAGE: {} /tmp/portal/lookup", args.strings[0]); exit(1); } static constexpr size_t buffff_size = 64 * KiB; u8 buffff[buffff_size]; memset(buffff, -1, buffff_size); Core::EventLoop event_loop; auto client_socket = MUST(Core::LocalSocket::connect(args.argv[1])); VERIFY(client_socket->is_open()); TRY(client_socket->set_blocking(true)); warnln("Writing {} bytes in a loop ...", buffff_size); while (true) TRY(client_socket->write_until_depleted({ buffff, buffff_size })); return 0; } ``` ![Bildschirmfoto_2023-05-15_19-46-32](https://github.com/SerenityOS/serenity/assets/2690845/c1fcf14d-3081-4641-8cf5-6cf8503f8461) ![Bildschirmfoto_2023-05-15_19-51-05](https://github.com/SerenityOS/serenity/assets/2690845/706ed031-52c7-448f-8a50-016482086c22) ![Bildschirmfoto_2023-05-15_19-56-22](https://github.com/SerenityOS/serenity/assets/2690845/da09b492-a870-4b30-9b0b-0a28f56e3bcb) ![Bildschirmfoto_2023-05-15_20-00-08](https://github.com/SerenityOS/serenity/assets/2690845/0e0c6f0b-b7fc-4d21-9609-ea2d14f8ccaf) … Kaboom!
ADKaster commented 1 year ago

LibWeb currently uses IPC messages to send JS console logs across to the browser, which can be arbitrarily large depending on the objects and their properties logged. Arguably that should use a different channel.

kleinesfilmroellchen commented 1 year ago

A better solution would be to handle errors and call did_misbehave if the message is not allocatable.

BenWiederhake commented 1 year ago

One of the problems is that it does handle the situation without problems for way too long, until a random part of the overall OS is killed through OOM. In my experiments, it was usually a different process that died first.

How about calling did_misbehave() when a packet size larger than 5 MiB is seen? That would catch the problem long before we run into any OOM trouble, and give us sufficient room to breathe with huge IPC messages. (We really shouldn't be sending so large messages anyway.)