runtime-spec icon indicating copy to clipboard operation
runtime-spec copied to clipboard

seccomp listener: define the max size of Container Process State messages

Open AkihiroSuda opened this issue 3 years ago • 4 comments

https://github.com/opencontainers/runtime-spec/blob/8958f93039ab90be53d803cd7e231a775f644451/config-linux.md#the-container-process-state

Currently we don't have any definition of the message size. I suggest to limit the size to be 4096 bytes corresponding to https://github.com/opencontainers/runc/blob/v1.1.0/contrib/cmd/seccompagent/seccompagent.go#L78 , but that may result in losing some annotations.

We should have prepended uint32le len field to the messages, but probably we are too late to change the spec.

AkihiroSuda avatar Feb 02 '22 05:02 AkihiroSuda

cc @rata @alban @cyphar PTAL

AkihiroSuda avatar Feb 02 '22 05:02 AkihiroSuda

@AkihiroSuda can you provide some context on why to limit the size?

That struct includes the OCI state, that AFAIK does not have a size limit, right? (annotations are arbitrary long, for example). How would we limit the size and why would we do it (note that the example is just an example as stated in the README)?

Please note that the listenerPath description says:

This socket MUST use AF_UNIX domain and SOCK_STREAM type. The runtime MUST send exactly one container process state per connection. The connection MUST NOT be reused and it MUST be closed after sending a seccomp state

This basically makes it super easy to handle big messages (it is SOCK_STREAM, no fixed limit), there will be only one message sent per connection, so it is trivial to reassemble all the pieces in one buffer and, of course, the agent can deny if the message if bigger than X.

Can you please elaborate?

rata avatar Feb 02 '22 10:02 rata

Defining hard limit is not needed, but implementations have to raise an error when the message exceeds some implementation-specific threshold value anyway (to avoid malloccing too much), so it might be still helpful to say The message size SHOULD (not "MUST") be 4096 (or 65536?) bytes at maximum.

AkihiroSuda avatar Feb 02 '22 12:02 AkihiroSuda

Oh, I'm fine with a SHOULD addition.

I'm not sure if the limit should be 4096 or what, though. In an ideal world, I'd like to see stats of containers running in the wild and see how long the OCI state is. But if someone can provide something that seems meaningful, or some other limit that was already used in the past, seems good to :)

rata avatar Feb 02 '22 13:02 rata