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

osutil/sys, client: add sys.RunAsUidGid, use it for auth.json #4983

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 42 additions & 12 deletions client/login.go
Expand Up @@ -27,6 +27,7 @@ import (
"path/filepath"

"github.com/snapcore/snapd/osutil"
"github.com/snapcore/snapd/osutil/sys"
)

// User holds logged in user information.
Expand Down Expand Up @@ -118,30 +119,46 @@ func writeAuthData(user User) error {

targetFile := storeAuthDataFilename(real.HomeDir)

if err := osutil.MkdirAllChown(filepath.Dir(targetFile), 0700, uid, gid); err != nil {
out, err := json.Marshal(user)
if err != nil {
return err
}

outStr, err := json.Marshal(user)
if err != nil {
return nil
}
return sys.RunAsUidGid(uid, gid, func() error {
if err := os.MkdirAll(filepath.Dir(targetFile), 0700); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, just return the error directly? :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, the diff formatting confused me, never mind :)

return err
}

return osutil.AtomicWriteFileChown(targetFile, []byte(outStr), 0600, 0, uid, gid)
return osutil.AtomicWriteFile(targetFile, out, 0600, 0)
})
}

// readAuthData reads previously written authentication details
func readAuthData() (*User, error) {
sourceFile := storeAuthDataFilename("")
f, err := os.Open(sourceFile)
real, err := osutil.RealUser()
if err != nil {
return nil, err
}

uid, gid, err := osutil.UidGid(real)
if err != nil {
return nil, err
}
defer f.Close()

var user User
dec := json.NewDecoder(f)
if err := dec.Decode(&user); err != nil {
sourceFile := storeAuthDataFilename("")

if err := sys.RunAsUidGid(uid, gid, func() error {
f, err := os.Open(sourceFile)
if err != nil {
return err
}
defer f.Close()

dec := json.NewDecoder(f)

return dec.Decode(&user)
}); err != nil {
return nil, err
}

Expand All @@ -150,6 +167,19 @@ func readAuthData() (*User, error) {

// removeAuthData removes any previously written authentication details.
func removeAuthData() error {
real, err := osutil.RealUser()
if err != nil {
return err
}

uid, gid, err := osutil.UidGid(real)
if err != nil {
return err
}

filename := storeAuthDataFilename("")
return os.Remove(filename)

return sys.RunAsUidGid(uid, gid, func() error {
return os.Remove(filename)
})
}
78 changes: 78 additions & 0 deletions osutil/sys/syscall.go
Expand Up @@ -20,7 +20,9 @@
package sys

import (
"fmt"
"os"
"runtime"
"syscall"
"unsafe"
)
Expand Down Expand Up @@ -107,3 +109,79 @@ func FcntlGetFl(fd int) (int, error) {
}
return int(flags), nil
}

func retryRawSyscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err syscall.Errno) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this function warrants a comment

ms := &syscall.Timespec{Nsec: 1000}
for i := 0; i < 30; i++ {
r1, r2, err = syscall.RawSyscall(trap, a1, a2, a3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a need to use RawSyscall vs Syscall? The difference, AFAIK, is that raw syscall blocks directly and Syscall will ping back to the runtime to say its about to enter and has just left a system call. If there's a need then please document why we use RawSyscall here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trusting the go standard library on the distinction here (syscall.Setreuid et al use RawSyscall). The difference is not documented and I don't know enough of the internals to have a good opinion.

if err != syscall.EAGAIN {
break
}
// this could fail, making the loop a little too aggressive
syscall.Nanosleep(ms, nil)
}
return r1, r2, err
}

type UnrecoverableError struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please document this type.

Call string
Err error
}

func (e UnrecoverableError) Error() string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please document this error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing docs :)

return fmt.Sprintf("%s: %v", e.Call, e.Err)
}

// RunAsUidGid starts a goroutine, pins it to the OS thread, sets euid and egid,
// and runs the function; after the function returns, it restores euid and egid.
//
// If restoring the original euid and egid fails this function will panic with
// an UnrecoverableError, and you should _not_ try to recover from it: the
// runtime itself is going to be in trouble.
func RunAsUidGid(uid UserID, gid GroupID, f func() error) error {
ch := make(chan error, 1)
go func() {
// from the docs:
// until the goroutine exits or calls UnlockOSThread, it will
// always execute in this thread, and no other goroutine can.
// that last bit means it's safe to setuid/setgid in here, as no
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick, That

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm torn between fixing it now, or landing it now and fixing it later :-)

// other code will run.
runtime.LockOSThread()

// from Go 1.10 on we could just not unlock, which would make
// the thread get reaped at goroutine exit. Instead we need to
// carefully restore thread state.
defer runtime.UnlockOSThread()

ruid := Getuid()
Copy link
Collaborator

Choose a reason for hiding this comment

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

While reading I read the code for those. Is there any reason we are not using getres{u,g}id()? It uses one less system call.

rgid := Getgid()

// do the setregid first =)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you document if the order is significant and if so, why, please?

if _, _, err := retryRawSyscall(_SYS_SETREGID, FlagID, uintptr(gid), 0); err != 0 {
ch <- fmt.Errorf("setregid: %v", err)
return
}
defer func() {
// try to restore egid
if _, _, err := retryRawSyscall(_SYS_SETREGID, FlagID, uintptr(rgid), 0); err != 0 {
// ¯\_(ツ)_/¯
panic(UnrecoverableError{Call: "setregid", Err: err})
}
}()

if _, _, err := retryRawSyscall(_SYS_SETREUID, FlagID, uintptr(uid), 0); err != 0 {
ch <- fmt.Errorf("setreuid: %v", err)
return
}
defer func() {
// try to restore euid
if _, _, err := retryRawSyscall(_SYS_SETREUID, FlagID, uintptr(ruid), 0); err != 0 {
// ¯\_(ツ)_/¯
panic(UnrecoverableError{Call: "setreuid", Err: err})
}
}()

ch <- f()
}()
return <-ch
}
3 changes: 3 additions & 0 deletions osutil/sys/sysnum_getpid_16.go
Expand Up @@ -30,4 +30,7 @@ const (
_SYS_GETGID = syscall.SYS_GETGID32
_SYS_GETEUID = syscall.SYS_GETEUID32
_SYS_GETEGID = syscall.SYS_GETEGID32

_SYS_SETREUID = syscall.SYS_SETREUID32
_SYS_SETREGID = syscall.SYS_SETREGID32
)
3 changes: 3 additions & 0 deletions osutil/sys/sysnum_getpid_32.go
Expand Up @@ -29,4 +29,7 @@ const (
_SYS_GETGID = syscall.SYS_GETGID
_SYS_GETEUID = syscall.SYS_GETEUID
_SYS_GETEGID = syscall.SYS_GETEGID

_SYS_SETREUID = syscall.SYS_SETREUID
_SYS_SETREGID = syscall.SYS_SETREGID
)
7 changes: 7 additions & 0 deletions tests/main/drop-privs/task.yaml
@@ -0,0 +1,7 @@
summary: test chattr
# ubuntu-core doesn't have go :-)
systems: [-ubuntu-core-16-*]
execute: |
go build ./testit.go
test "$(./testit)" = "before: 0/0, during: 12345/12345 (<nil>), after: 0/0; status: OK"
test "$(sudo -u '#12345' -g '#12345' ./testit)" = "before: 12345/12345, during: 12345/12345 (<nil>), after: 12345/12345; status: OK"
57 changes: 57 additions & 0 deletions tests/main/drop-privs/testit.go
@@ -0,0 +1,57 @@
package main

import (
"fmt"
"runtime"
"sync"

"github.com/snapcore/snapd/osutil/sys"
)

var wg sync.WaitGroup
var mu sync.Mutex

func check(uids []sys.UserID, n int) {
// spin
for i := 0; i < 1<<30; i++ {
}

mu.Lock()
uids[n] = sys.Geteuid()
mu.Unlock()

wg.Done()
}

func main() {
orig := sys.Geteuid()
before := fmt.Sprintf("%d/%d", sys.Geteuid(), sys.Getegid())
var during string
err := sys.RunAsUidGid(12345, 12345, func() error {
during = fmt.Sprintf("%d/%d", sys.Geteuid(), sys.Getegid())
return nil
})
after := fmt.Sprintf("%d/%d", sys.Geteuid(), sys.Getegid())

N := 2 * runtime.NumCPU()
uids := make([]sys.UserID, N)
// launch a lot of goroutines so we cover all threads with space to spare
for i := 0; i < N; i++ {
wg.Add(1)
go check(uids, i)
}
wg.Wait()

bad := 0
for _, uid := range uids {
if uid != orig {
bad++
}
}
status := "OK"
if bad != 0 {
status = fmt.Sprintf("%d BAD!", bad)
}

fmt.Printf("before: %s, during: %s (%v), after: %s; status: %s\n", before, during, err, after, status)
}