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

Why do we limit the messages that can be sent to the conmon socket. #461

Closed
rhatdan opened this issue Nov 1, 2023 · 11 comments · Fixed by #469
Closed

Why do we limit the messages that can be sent to the conmon socket. #461

rhatdan opened this issue Nov 1, 2023 · 11 comments · Fixed by #469

Comments

@rhatdan
Copy link
Member

rhatdan commented Nov 1, 2023

containers/buildah#5125 (comment)

A customer wants to be able to send messages other then the two that are allowed and would like to be able to define their own.

If we allow general messages through, does this cause a security risk?

If so then we could add a mechanism for the caller of conmon to pass down valid messages to be allowed to pass.

Thoughts?

@rhatdan
Copy link
Member Author

rhatdan commented Nov 1, 2023

@vrothberg
Copy link
Member

Is there an issue/customer case to get some more context?

@rhatdan
Copy link
Member Author

rhatdan commented Nov 1, 2023

No, just came from a meeting with a customer, that would like to track via dbus other messages. In the original commit for this code, it was mentioned that this could be extended.

commit 59c2817b70746e78d5957d3c275e6385697224d1
Author: Joseph Gooch <mrwizard@dok.org>
Date:   Mon Aug 17 23:25:05 2020 +0000

    Refactor I/O and add SD_NOTIFY proxy support
    
    Refactored all the conn_sock functionality to be more generic. It can deal
    with different types of sockets, stream vs dgram, and reuses all the same
    callbacks, shutdown and async functionality.
    
    Conmon creates a notify socket which podman bind-mounts into the container,
    and passes in via the spec's environment variables.  Conmon relays the
    READY=1 signal.  This is similar to what runc and crun do, but doing it in
    conmon and NOT passing NOTIFY_SOCKET to the OCI runtime allows us to start
    up properly without runc and crun blocking on the "start" command.
    
    It would also be trivial to add more proxied sockets, i.e. the /dev/log
    proof of concept I did would now be super easy, if we wanted to revisit that.
    
    Signed-off-by: Joseph Gooch <mrwizard@dok.org>

@rhatdan
Copy link
Member Author

rhatdan commented Nov 1, 2023

They want to send messages like
CO_CONN_WAIT=1
CO_CONN_READY=1

I guess. Where CO is based on the company name.

@alexlarsson
Copy link
Contributor

I think also outside of this particular customer, it would be nice to be able to use other standardized messages from https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#Description. For example STOPPING=1 or STATUS=Completed 66% of file system check..

@giuseppe
Copy link
Member

I don't know the details, but when I added this logic to runc/crun/conmon, I asked Lennart if there are potential risks with doing so, and he told me that READY is safe. In fact systemd-nspawn is doing the same: https://github.com/systemd/systemd/blob/main/src/nspawn/nspawn.c#L4225-L4235

I see there that the STATUS= from the container is rewritten to "STATUS=Container running: %s", ...., we could do the same.

Not sure about the others. We can probably enable more of them if we think of the consequences of a rogue container that can forge that messages.

@alexlarsson
Copy link
Contributor

STOPPING seems quite safe to me at least, and its been brought up by the customer.

@alexlarsson
Copy link
Contributor

RELOADING, ERRNO, BUSERROR, MONOTONIC_USEC, EXTEND_TIMEOUT_USEC seems safe too

@giuseppe
Copy link
Member

can EXTEND_TIMEOUT_USEC be abused to cause the container to run longer than it is supposed to do?

@alexlarsson
Copy link
Contributor

@giuseppe Yeah, possibly... Maybe best to not include that, at least without some opt-in.

@rhatdan
Copy link
Member Author

rhatdan commented Nov 29, 2023

Yes lets eliminate that.

alexlarsson added a commit to alexlarsson/conmon that referenced this issue Dec 11, 2023
Several of the standard sd-notify messages are safe to use from a
container and are very useful. This commit cleans up the general
handling of notify messages and allows forwarding of:

 * READY=1
 * RELOADING=1
 * STOPPING=1
 * WATCHDOG=1
 * WATCHDOG=trigger
 * STATUS=...
 * ERRNO=...
 * BUSERROR=...
 * MONOTONIC_USEC...

See https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#Well-known%20assignments
for documentation of these.

Note: We don't allow unknown messages to be forewarded. For one, all
the file-descriptor based ones are currently unsupported (since the
forwarding doesn't handle fds), but also some options (current and
future) may be security sensitive.

This fixes containers#461
alexlarsson added a commit to alexlarsson/conmon that referenced this issue Dec 11, 2023
Several of the standard sd-notify messages are safe to use from a
container and are very useful. This commit cleans up the general
handling of notify messages and allows forwarding of:

 * READY=1
 * RELOADING=1
 * STOPPING=1
 * WATCHDOG=1
 * WATCHDOG=trigger
 * STATUS=...
 * ERRNO=...
 * BUSERROR=...
 * MONOTONIC_USEC...

See https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#Well-known%20assignments
for documentation of these.

Note: We don't allow unknown messages to be forewarded. For one, all
the file-descriptor based ones are currently unsupported (since the
forwarding doesn't handle fds), but also some options (current and
future) may be security sensitive.

This fixes containers#461

Signed-off-by: Alexander Larsson <alexl@redhat.com>
alexlarsson added a commit to alexlarsson/conmon that referenced this issue Dec 12, 2023
Several of the standard sd-notify messages are safe to use from a
container and are very useful. This commit cleans up the general
handling of notify messages and allows forwarding of:

 * READY=1
 * RELOADING=1
 * STOPPING=1
 * WATCHDOG=1
 * WATCHDOG=trigger
 * STATUS=...
 * ERRNO=...
 * BUSERROR=...
 * MONOTONIC_USEC...

See https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#Well-known%20assignments
for documentation of these.

Note: We don't allow unknown messages to be forewarded. For one, all
the file-descriptor based ones are currently unsupported (since the
forwarding doesn't handle fds), but also some options (current and
future) may be security sensitive.

This fixes containers#461 and
containers#311

Signed-off-by: Alexander Larsson <alexl@redhat.com>
alexlarsson added a commit to alexlarsson/conmon that referenced this issue Dec 12, 2023
Several of the standard sd-notify messages are safe to use from a
container and are very useful. This commit cleans up the general
handling of notify messages and allows forwarding of:

 * READY=1
 * RELOADING=1
 * STOPPING=1
 * WATCHDOG=1
 * WATCHDOG=trigger
 * STATUS=...
 * ERRNO=...
 * BUSERROR=...
 * MONOTONIC_USEC...

See https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#Well-known%20assignments
for documentation of these.

Note: We don't allow unknown messages to be forewarded. For one, all
the file-descriptor based ones are currently unsupported (since the
forwarding doesn't handle fds), but also some options (current and
future) may be security sensitive.

fixes containers#461
fixes containers#311

Signed-off-by: Alexander Larsson <alexl@redhat.com>
alexlarsson added a commit to alexlarsson/conmon that referenced this issue Dec 12, 2023
Several of the standard sd-notify messages are safe to use from a
container and are very useful. This commit cleans up the general
handling of notify messages and allows forwarding of:

 * READY=1
 * RELOADING=1
 * STOPPING=1
 * WATCHDOG=1
 * WATCHDOG=trigger
 * STATUS=...
 * ERRNO=...
 * BUSERROR=...
 * MONOTONIC_USEC...

See https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#Well-known%20assignments
for documentation of these.

Note: We don't allow unknown messages to be forewarded. For one, all
the file-descriptor based ones are currently unsupported (since the
forwarding doesn't handle fds), but also some options (current and
future) may be security sensitive.

fixes containers#461
fixes containers#311

Signed-off-by: Alexander Larsson <alexl@redhat.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 a pull request may close this issue.

4 participants