Skip to content

FIX: add validation for containerID#726

Open
kaizakin wants to merge 1 commit into
urunc-dev:mainfrom
kaizakin:fix/validate-containerid
Open

FIX: add validation for containerID#726
kaizakin wants to merge 1 commit into
urunc-dev:mainfrom
kaizakin:fix/validate-containerid

Conversation

@kaizakin
Copy link
Copy Markdown
Contributor

@kaizakin kaizakin commented May 25, 2026

Description

  • added containerID validation following the similar approach as runc

Almost every command in urunc delete, kill, ps, start, run, and the internal re-exec phase calls the shared getUnikontainer() helper function to read the container ID from the command line and load its state.
by adding validateID inside this helper we've automatically secured all commands.

only the create command doesn't use the getUnikontainer() because the container state hasn't been initialized yet
so i explicitly added validateId there.

How was this tested?

Manually tested by locally compiling urunc and simulating the path traversal attack vector

image

LLM usage

N/A

Checklist

  • I have read the contribution guide.
  • The linter passes locally (make lint).
  • The e2e tests of at least one tool pass locally (make test_ctr, make test_nerdctl, make test_docker, make test_crictl).
  • If LLMs were used: I have read the llm policy.

@netlify
Copy link
Copy Markdown

netlify Bot commented May 25, 2026

Deploy Preview for urunc canceled.

Name Link
🔨 Latest commit 99fce71
🔍 Latest deploy log https://app.netlify.com/projects/urunc/deploys/6a1ab556e989f70008334684

@kaizakin
Copy link
Copy Markdown
Contributor Author

Hey @cmainas

what's your opinion on this comment #714 (comment)
saying about securejoin.SecureJoin(root, id) instead of filepath.Join

Copy link
Copy Markdown
Contributor

@mdryaan mdryaan left a comment

Choose a reason for hiding this comment

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

Hi @kaizakin, I think two things to fix:

  1. cmd/urunc/utils.go "path/filepath" is in the wrong import group (will fail CI lint)

path/filepath is stdlib it needs to go in the same group as the other stdlib imports, not in its own group between stdlib and external. goimports is enabled in golangci-lint v2 and is checked during golangci-lint run.

  1. go.mod revert the filepath-securejoin entry

github.com/cyphar/filepath-securejoin is not imported anywhere in the code , validateID only uses stdlib path/filepath. Looks like leftover from an earlier attempt. Revert the go.mod change.

The validation logic and placement look correct.

cc @cmainas

Signed-off-by: karthik balasubramanian <karthikbalasubramanian08@gmail.com>
@kaizakin kaizakin force-pushed the fix/validate-containerid branch from 23636fd to 99fce71 Compare May 30, 2026 10:00
@kaizakin
Copy link
Copy Markdown
Contributor Author

Thanks for the review @mdryaan

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.

we should probably validate containerID before constructing the containerDir path

2 participants