Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag to compile Swoole 6.x without io_uring support #1044

Merged
merged 4 commits into from
Jan 9, 2025

Conversation

MichaelGooden
Copy link
Contributor

@MichaelGooden MichaelGooden commented Jan 9, 2025

This PR adds a flag to optionally compile Swoole without enabling io_uring support.

(I am not sure if this is the best way to implement this in the script)

Security experts generally believe io_uring to be unsafe. In fact Google ChromeOS and Android have turned it off, plus all Google production servers turn it off. Based on the blog published by Google below it seems like a bunch of vulnerabilities related to io_uring can be exploited to breakout of the container. Google Security Blog
Other security reaserchers also hold this opinion: see the Blackhat Presentation on io_uring exploits.

Impact on IPE:
Secure defaults on many container runtimes and operating systems limit io_uring syscalls to privileged users holding CAP_SYS_ADMIN. If Swoole is compiled with io_uring support enabled, and the resulting container is run as a non-privileged/non-root user with these secure defaults, the installation will crash with an io_uring permissions error when using any filesystem function hooked by Swoole in a Coroutine.

There does not appear to be a mechanism to disable this behavior at Swoole runtime.

@MichaelGooden MichaelGooden marked this pull request as ready for review January 9, 2025 13:15
@mlocati
Copy link
Owner

mlocati commented Jan 9, 2025

Thanks!

That's the correct approach (at least, it's the approach used elsewhere in the script).

What about:

  1. Disabling iouring by default, inverting the logic (the env var could be something like IPE_SWOOLE_WITH_IOURING)

  2. Fixing the links in the README

    In the README of your PR we have misplaced links:

    immagine

    I'd replace

    | swoole | `IPE_SWOOLE_WITHOUT_IOURING=1` | The io_uring kernel functionality is considered unsafe by security experts [1][2]; By default Swoole 6 and later is configured with io_uring support, use this environment variable to skip io_uring |
    
    [1](https://security.googleblog.com/2023/06/learnings-from-kctf-vrps-42-linux.html)
    [2](https://i.blackhat.com/BH-US-23/Presentations/US-23-Lin-bad_io_uring.pdf)

    with something like this:

    | swoole | `IPE_SWOOLE_WITHOUT_IOURING=1` | The io_uring kernel functionality is considered unsafe by security experts (see [here](https://security.googleblog.com/2023/06/learnings-from-kctf-vrps-42-linux.html) and [here](https://i.blackhat.com/BH-US-23/Presentations/US-23-Lin-bad_io_uring.pdf)). By default Swoole 6 and later is configured with io_uring support, use this environment variable to skip io_uring |

@MichaelGooden
Copy link
Contributor Author

MichaelGooden commented Jan 9, 2025

  1. Disabling iouring by default, inverting the logic (the env var could be something like IPE_SWOOLE_WITH_IOURING)

I have some thoughts on this:
a) Backwards compatibility: there may be users relying on io_uring being enabled by default, per existing behavior
b) This may be an opinionated security position. io_uring is in active development and receives regular security fixes - my understanding is that it isn't inherently dangerous, it just has a very large attack surface and is relatively new code to the kernel

As an end user, I appreciate and rely on the "batteries-included" approach to your installer. My concern would be setting precedent on having to opt-in to otherwise expected defaults.

  1. Fixing the links in the README

Will be updated shortly.

@mlocati mlocati merged commit 3529772 into mlocati:master Jan 9, 2025
40 checks passed
@MichaelGooden MichaelGooden deleted the swoole-without-iouring branch January 17, 2025 11:02
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.

2 participants