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

Chroot rootfs path usage fix #2237

Closed

Conversation

Zyqsempai
Copy link
Contributor

Fixes: #2214

In order to avoid misunderstanding, it's better to use path parameter in chroot() func.

Signed-off-by: Boris Popovschi zyqsempai@mail.ru

@Zyqsempai
Copy link
Contributor Author

@cyphar PTAL

Signed-off-by: Boris Popovschi <zyqsempai@mail.ru>
@@ -867,7 +867,7 @@ func msMoveRoot(rootfs string) error {
}

func chroot(rootfs string) error {
if err := unix.Chroot("."); err != nil {
if err := unix.Chroot(rootfs); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

needs validation?

Copy link
Member

Choose a reason for hiding this comment

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

should error out if rootfs != "." ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AkihiroSuda Do you mean something like this?(check last commit)

Copy link
Member

Choose a reason for hiding this comment

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

if filepath.Clean(rootfs) != os.Getwd(){ return errors.New(..) }? (Make sure to test it, ideally in CI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

os.Getwd() returns absolute wd path, on other hand rootfs path here can be relative and we will get error even if they are point at same dir.

Copy link
Member

Choose a reason for hiding this comment

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

if filepath.Abs(rootfs) != os.Getwd(){ return errors.New(..) }. The argument of Chroot() should remain ".".

Signed-off-by: Boris Popovschi <zyqsempai@mail.ru>
Signed-off-by: Boris Popovschi <zyqsempai@mail.ru>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

As commented above, Chroot(".") should be kept as-is.

@kolyshkin
Copy link
Contributor

so, should we close this one?

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.

chroot() function appears to ignore its parameter
3 participants