Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions storage/pkg/unshare/unshare_cgo.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ package unshare
// _containers_unshare();
// }
import "C"

func unshareWithNoCGO(c *Cmd) {}
2 changes: 2 additions & 0 deletions storage/pkg/unshare/unshare_gccgo.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,5 @@ func init() {
C.init()
}
}

func unshareWithNoCGO(c *Cmd) {}
3 changes: 3 additions & 0 deletions storage/pkg/unshare/unshare_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@ func (c *Cmd) Start() (retErr error) {
}
}()

// Additional steps are required when CGO is not available
unshareWithNoCGO(c)

// Start the new process.
err = c.Cmd.Start()
if err != nil {
Expand Down
89 changes: 89 additions & 0 deletions storage/pkg/unshare/unshare_nocgo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
//go:build linux && !cgo

package unshare

import (
"fmt"
"io"
"os"
"strconv"
"syscall"
)

func unshareWithNoCGO(c *Cmd) {
if c.Cmd.SysProcAttr == nil {
c.Cmd.SysProcAttr = &syscall.SysProcAttr{}
}
attr := c.Cmd.SysProcAttr
Comment on lines +14 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this could happen only inside the if below.

if c.UnshareFlags&syscall.CLONE_NEWUSER != 0 {
attr.Cloneflags = uintptr(c.UnshareFlags)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing documents why we are not using “unshare” flags for unshare.

AFAICS the two functions don’t accept exactly the same sets of flags, so even if this were correct for the callers we are currently anticipating, I’d expect the code here to report a failure if any unexpected flag value is set.


(And, well, if Cloneflags works here fine nowadays, wouldn’t we want to use it in the CGo code path as well, and save us the unshare trouble? Is there more subtlety to this? There probably is, given the two separate unshare calls.)

This sort of applies generally — if we can do something natively using SysProcAttr nowadays, I’d expect that to always be done, rather than having two different code paths.)

attr.GidMappingsEnableSetgroups = c.GidMappingsEnableSetgroups
Copy link
Contributor

Choose a reason for hiding this comment

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

This does nothing at all unless …

if c.Ctty != nil {
index := len(c.Cmd.ExtraFiles)
c.Cmd.ExtraFiles = append(c.Cmd.ExtraFiles, c.Ctty)
attr.Ctty = index
Copy link
Contributor

Choose a reason for hiding this comment

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

This does nothing unless …

}
}
}

func parseIntContainersUnshareEnv(name string) int {
env := os.Getenv(name)
if env == "" {
return -1
}
_ = os.Unsetenv(name)
v, err := strconv.Atoi(env)
if err != nil {
return -1
Copy link
Contributor

Choose a reason for hiding this comment

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

The C code differentiates between “missing” and “invalid format”

}
return v
}

func init() {
flags := parseIntContainersUnshareEnv("_Containers-unshare")
if flags == -1 {
return
}

if pidFD := parseIntContainersUnshareEnv("_Containers-pid-pipe"); pidFD > -1 {
pidFile := os.NewFile(uintptr(pidFD), "")
_, err := fmt.Fprintf(pidFile, "%d", os.Getpid())
if err != nil {
bailOnError(err, "Write pid failed")
}
_ = pidFile.Close()
}
if continueFD := parseIntContainersUnshareEnv("_Containers-continue-pipe"); continueFD > -1 {
continueFile := os.NewFile(uintptr(continueFD), "")
buf := make([]byte, 2048)
n, err := continueFile.Read(buf)
if err != nil && err != io.EOF {
bailOnError(err, "Read containers continue pipe")
}
if n > 0 {
bailOnError(fmt.Errorf(string(buf)), "Unexpected containers continue pipe read")
Copy link
Contributor

Choose a reason for hiding this comment

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

Format string vulnerability.


Nothing is all that unexpected about this error reporting path, the data exists only to be reported this way.

}
}

if setSid := parseIntContainersUnshareEnv("_Containers-setsid"); setSid == 1 {
_, err := syscall.Setsid()
if err != nil {
bailOnError(err, "Error during setsid")
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Setpgrp handling?

if flags&syscall.CLONE_NEWUSER != 0 {
if err := syscall.Setresuid(0, 0, 0); err != nil {
bailOnError(err, "Setresuid failed")
}
if err := syscall.Setresgid(0, 0, 0); err != nil {
bailOnError(err, "Setresgid failed")
}
Comment on lines +76 to +81
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the UID/GID order differ from the C version?

}

Copy link
Contributor

Choose a reason for hiding this comment

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

The C version does a two-stage unshare. Why is it necessary there and not necessary here?

// Re-invoke the execve system call to obtain capabilities.
err := syscall.Exec("/proc/self/exe", os.Args, os.Environ())
Copy link
Contributor

Choose a reason for hiding this comment

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

containers_reexec does a lot of things. I have no idea why it does all of them (and the code doesn’t document it :| ); either way, I must ask: why is all of that not necessary here?

if err != nil {
bailOnError(err, "syscall.Exec %s", "/proc/self/exe")
}
}
Loading