Validate mount options for null bytes (\u0000) early to provide clearer error messages

[74suiko]

Description: I noticed that while runc (v1.4.2) correctly rejects mount options containing null bytes (\u0000), the failure occurs very late in the lifecycle—at the syscall level—resulting in a somewhat generic and cryptic error.

When injecting a null byte into mount options (e.g., nosuid\u0000) for filesystems like cgroup, mqueue, sysfs, and tmpfs, the unvalidated string is passed down to the mount(2) syscall, causing the kernel to return an EINVAL (Invalid Argument) error.

(For context, youki v0.6.0 exhibits similar behavior, whereas crun v1.28 silently strips everything after the null byte and proceeds).

The Proposal: It might be beneficial to add early validation during the configuration parsing phase. If the parser explicitly checks for and rejects \u0000 in mount options, it could return a much more actionable and user-friendly error message (e.g., Error: mount option 'nosuid\u0000' contains illegal null byte), rather than relying on the kernel to catch it.

Steps to reproduce:

1. Use the following config.json snippet where \u0000 is injected into the mount options:

config.json snippet

"mounts": [
    {
        "destination": "/proc",
        "type": "proc",
        "source": "proc"
    },
    {
        "destination": "/dev",
        "type": "tmpfs",
        "source": "tmpfs",
        "options": [
            "nosuid",
            "strictatime",
            "mode=755",
            "size=65536k"
        ]
    },
    {
        "destination": "/dev/pts",
        "type": "devpts",
        "source": "devpts",
        "options": [
            "nosuid",
            "noexec",
            "newinstance",
            "ptmxmode=0666",
            "mode=0620",
            "gid=5"
        ]
    },
    {
        "destination": "/dev/shm",
        "type": "tmpfs",
        "source": "shm",
        "options": [
            "nosuid",
            "noexec",
            "nodev",
            "mode=1777",
            "size=65536k"
        ]
    },
    {
        "destination": "/dev/mqueue",
        "type": "mqueue",
        "source": "mqueue",
        "options": [
            "nosuid",
            "noexec",
            "nodev"
        ]
    },
    {
        "destination": "/sys",
        "type": "sysfs",
        "source": "sysfs",
        "options": [
            "nosuid",
            "noexec",
            "nodev",
            "ro"
        ]
    },
    {
        "destination": "/sys/fs/cgroup",
        "type": "cgroup2",
        "source": "cgroup2",
        "options": [
            "nosuid\u0000",
            "noexec",
            "nodev",
            "relatime",
            "ro",
            "nsdelegate"
        ]
    }
]

1. Run the container creation command (using sudo):

Bash

sudo runc create mycontainer

Current Logs and Outputs:

Currently, the runtime passes the string to the kernel and crashes with a syscall error.

Plaintext

ERRO[0000] runc create failed: unable to start container process: error during container init: error mounting "cgroup2" to rootfs at "/sys/fs/cgroup": mount src=cgroup2, dst=/sys/fs/cgroup, dstFd=/proc/thread-self/fd/11, flags=MS_RDONLY|MS_NODEV|MS_NOEXEC|MS_RELATIME, data=nosuid,nsdelegate: invalid argument

Expected Behavior:

The runtime should catch the null byte during the spec validation phase (before executing syscalls) and print a clear error indicating that null bytes are not allowed in string parameters.

[kolyshkin]

Honestly I think you're trying to solve the problem that largely doesn't exist.

OTOH we already check env vars in this manner (initially added by #3017), so maybe worth to generalize it to some other fields, too.

[cyphar]

NUL bytes in mount options are actually permitted, filesystems could parse arbitrary data from data (the pointer is not actually defined as being a C string buffer -- the kernel internally always copies out a PAGE_SIZE buffer for this reason).

Several upstream filesystems do this, codafs has an entirely binary format and older versions of vboxsf used a binary format.

I'm not sure how well that works with our config format but I'm quite reticent about restricting configurations during parsing like this.

[74suiko]

NUL bytes in mount options are actually permitted, filesystems could parse arbitrary data from data (the pointer is not actually defined as being a C string buffer -- the kernel internally always copies out a PAGE_SIZE buffer for this reason).在挂载选项中，使用空字节其实是被允许的。文件系统可以从 data 位置开始解析任意数据（需要注意的是，该指针实际上并不被视作 C 语言中的字符串缓冲区——出于这个原因，内核内部总会先复制一份数据到 PAGE_SIZE 缓冲区中）。

Several upstream filesystems do this, codafs has an entirely binary format and older versions of vboxsf used a binary format.一些上游文件系统都是这么做的。 codafs 采用完全二进制格式，而 vboxsf 的旧版本也使用二进制格式。

I'm not sure how well that works with our config format but I'm quite reticent about restricting configurations during parsing like this.我不确定这种方式是否适用于我们的配置格式。不过，我实在不愿意在解析过程中对配置进行这样的限制。

Many thanks for the prompt response. That makes sense. I had assumed NUL bytes in mount options were always invalid, but I see your point that mount(2) data is filesystem-specific and may be binary for some filesystems.

So a blanket rejection during config parsing is probably not the right approach.

The smaller issue I was trying to point out is diagnostic: in this case, nosuid\0 does not match the known nosuid flag, falls through into the mount data path, and later fails with a generic EINVAL. That makes it hard to see that the original input contained an embedded NUL.

Maybe the better fix would be to improve error reporting / escaping of the original mount options, rather than rejecting all NUL bytes?

[kolyshkin]

@74suiko will something like this help?

diff --git a/libcontainer/mount_linux.go b/libcontainer/mount_linux.go
index 82ef0fdf1..0ff2a0f33 100644
--- a/libcontainer/mount_linux.go
+++ b/libcontainer/mount_linux.go
@@ -124,7 +124,7 @@ func (e *mountError) Error() string {
                out += ", flags=" + stringifyMountFlags(e.flags)
        }
        if e.data != "" {
-               out += ", data=" + e.data
+               out += ", data=" + strconv.Quote(e.data)
        }
 
        out += ": " + e.err.Error()

(other fields can (should?) be similarly quoted).

[74suiko]

@74suiko will something like this help?

diff --git a/libcontainer/mount_linux.go b/libcontainer/mount_linux.go
index 82ef0fdf1..0ff2a0f33 100644
--- a/libcontainer/mount_linux.go
+++ b/libcontainer/mount_linux.go
@@ -124,7 +124,7 @@ func (e *mountError) Error() string {
out += ", flags=" + stringifyMountFlags(e.flags)
}
if e.data != "" {

•           out += ", data=" + e.data
  

•           out += ", data=" + strconv.Quote(e.data)
    }
  
    out += ": " + e.err.Error()
  

(other fields can (should?) be similarly quoted).

Thanks for the quick suggestion! I've tested it and it works great. I agree that other fields should be quoted too.

[kolyshkin]

Thanks for the quick suggestion! I've tested it and it works great. I agree that other fields should be quoted too.

Feel free to open a PR.