Skip to content

criu: validate non-regular descriptor files - #5402

Open
rst0git wants to merge 1 commit into
opencontainers:mainfrom
rst0git:fix-readjson-regular-files
Open

criu: validate non-regular descriptor files#5402
rst0git wants to merge 1 commit into
opencontainers:mainfrom
rst0git:fix-readjson-regular-files

Conversation

@rst0git

@rst0git rst0git commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

When restore opens descriptors.json from the checkpoint image, a FIFO at this path may block the read indefinitely, so repeated restore attempts can exhaust caller threads in container runtimes. To fix this, we should open this file through the pre-opened image directory with O_PATH, verify the inode is a regular file, and reopen that handle for reading. This patch also rejects symlinks and other special files, and limits the read to 1 MiB to prevent unbounded allocations.

When restore opens `descriptors.json` from the checkpoint image, a FIFO
at this path may block the read indefinitely, so repeated restore
attempts can exhaust caller threads in container runtimes. To fix this,
we should open this file through the pre-opened image directory with
`O_PATH`, verify the inode is a regular file, and reopen that handle for
reading. This patch also rejects symlinks and other special files, and
limits the read to 1 MiB to prevent unbounded allocations.

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
// type check, so the checked inode is the one that is read).
func openRegularFileAt(dir *os.File, name string) (*os.File, error) {
path := filepath.Join(dir.Name(), name)
handle, err := utils.Openat(dir, name, unix.O_PATH|unix.O_NOFOLLOW, 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add O_PATH?

May also add O_CLOEXEC to be explicit, but it's added by utils.Openat anyway

path := filepath.Join(dir.Name(), name)
handle, err := utils.Openat(dir, name, unix.O_PATH|unix.O_NOFOLLOW, 0)
if err != nil {
return nil, fmt.Errorf("open %s: %w", path, err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need to wrap the error as it already comes as os.PathError with both "open" and path specified.


info, err := handle.Stat()
if err != nil {
return nil, fmt.Errorf("stat %s: %w", path, err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here: the error from stat is os.PathError telling both the op (stat) and the path.


info, err := f.Stat()
if err != nil {
return nil, fmt.Errorf("stat %s: %w", path, err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

}
defer f.Close()

info, err := f.Stat()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slightly dislike that you're doing two stat calls on the same file -- once to check if it's a regular file, then to check the size. Can probably combine those.

@kolyshkin

Copy link
Copy Markdown
Contributor

So, we have the same issue when reading say config.json or state.json. Does it make sense to harden against fifos and large files in there? Does this PR make sense? I mean, are we doing this for security? If yes, how could the possible exploit look like?

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