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
overlord/snapstate: do a minimal sanity check on containers #4464
Changes from all commits
91b4232
09d0b43
9652b83
3328b78
4305dcb
2f7a8ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,13 +20,17 @@ | |
package snapstate | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"os" | ||
"path/filepath" | ||
"regexp" | ||
"strconv" | ||
"strings" | ||
|
||
"github.com/snapcore/snapd/arch" | ||
"github.com/snapcore/snapd/cmd" | ||
"github.com/snapcore/snapd/logger" | ||
"github.com/snapcore/snapd/overlord/snapstate/backend" | ||
"github.com/snapcore/snapd/overlord/state" | ||
"github.com/snapcore/snapd/release" | ||
|
@@ -180,13 +184,191 @@ func validateInfoAndFlags(info *snap.Info, snapst *SnapState, flags Flags) error | |
return nil | ||
} | ||
|
||
// normPath is a helper for validateContainer. It takes a relative path (e.g. an | ||
// app's RestartCommand, which might be empty to mean there is no such thing), | ||
// and cleans it. | ||
// | ||
// * empty paths are returned as is | ||
// * if the path is not relative, it's initial / is dropped | ||
// * if the path goes "outside" (ie starts with ../), the empty string is | ||
// returned (i.e. "ignore") | ||
// * if there's a space in the command, ignore the rest of the string | ||
// (see also cmd/snap-exec/main.go's comment about strings.Split) | ||
func normPath(path string) string { | ||
if path == "" { | ||
return "" | ||
} | ||
|
||
path = strings.TrimPrefix(filepath.Clean(path), "/") | ||
if strings.HasPrefix(path, "../") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, some questions: when does this condition happens? And if we don't handle it, is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can create a snap with an app whose command is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice if we had a bit of logic that reconstructs the real path knowing two bits of information:
Then we can construct an absolute path by normalizing whatever relative paths we get here and complete the validation. |
||
// not something inside the snap | ||
return "" | ||
} | ||
if idx := strings.IndexByte(path, ' '); idx > -1 { | ||
return path[:idx] | ||
} | ||
|
||
return path | ||
} | ||
|
||
var ( | ||
ErrBadModes = errors.New("snap is unusable due to bad permissions; contact develper") | ||
ErrMissingPaths = errors.New("snap is unusable due to missing files; contact developer") | ||
) | ||
|
||
func validateContainer(s *snap.Info, c snap.Container) error { | ||
// needsrx keeps track of things that need to have at least 0555 perms | ||
needsrx := map[string]bool{ | ||
".": true, | ||
"meta": true, | ||
} | ||
// needsx keeps track of things that need to have at least 0111 perms | ||
needsx := map[string]bool{} | ||
// needsr keeps track of things that need to have at least 0444 perms | ||
needsr := map[string]bool{ | ||
"meta/snap.yaml": true, | ||
} | ||
// needsf keeps track of things that need to be regular files (or symlinks to regular files) | ||
needsf := map[string]bool{} | ||
// noskipd tracks directories we want to descend into despite not being in needs* | ||
noskipd := map[string]bool{} | ||
|
||
for _, app := range s.Apps { | ||
// for non-services, paths go into the needsrx bag because users | ||
// need rx perms to execute it | ||
bag := needsrx | ||
paths := []string{app.Command} | ||
if app.IsService() { | ||
// services' paths just need to not be skipped by the validator | ||
bag = noskipd | ||
// additional paths to check for services: | ||
// XXX maybe have a method on app to keep this in sync | ||
paths = append(paths, app.StopCommand, app.ReloadCommand, app.PostStopCommand) | ||
} | ||
|
||
for _, path := range paths { | ||
path = normPath(path) | ||
if path == "" { | ||
continue | ||
} | ||
|
||
needsf[path] = true | ||
if app.IsService() { | ||
needsx[path] = true | ||
} | ||
for ; path != "."; path = filepath.Dir(path) { | ||
bag[path] = true | ||
} | ||
} | ||
|
||
// completer is special :-/ | ||
if path := normPath(app.Completer); path != "" { | ||
needsr[path] = true | ||
for path = filepath.Dir(path); path != "."; path = filepath.Dir(path) { | ||
needsrx[path] = true | ||
} | ||
} | ||
} | ||
// note all needsr so far need to be regular files (or symlinks) | ||
for k := range needsr { | ||
needsf[k] = true | ||
} | ||
// thing can get jumbled up | ||
for path := range needsrx { | ||
delete(needsx, path) | ||
delete(needsr, path) | ||
} | ||
for path := range needsx { | ||
if needsr[path] { | ||
delete(needsx, path) | ||
delete(needsr, path) | ||
needsrx[path] = true | ||
} | ||
} | ||
seen := make(map[string]bool, len(needsx)+len(needsrx)+len(needsr)) | ||
|
||
// bad modes are logged instead of being returned because the end user | ||
// can do nothing with the info (and the developer can read the logs) | ||
hasBadModes := false | ||
err := c.Walk(".", func(path string, info os.FileInfo, err error) error { | ||
if err != nil { | ||
return err | ||
} | ||
|
||
mode := info.Mode() | ||
if needsrx[path] || needsx[path] || needsr[path] { | ||
seen[path] = true | ||
} | ||
if !needsrx[path] && !needsx[path] && !needsr[path] && !strings.HasPrefix(path, "meta/") { | ||
if mode.IsDir() { | ||
if noskipd[path] { | ||
return nil | ||
} | ||
return filepath.SkipDir | ||
} | ||
return nil | ||
} | ||
|
||
if needsrx[path] || mode.IsDir() { | ||
if mode.Perm()&0555 != 0555 { | ||
logger.Noticef("in snap %q: %q should be world-readable and executable, and isn't: %s", s.Name(), path, mode) | ||
hasBadModes = true | ||
} | ||
} else { | ||
if needsf[path] { | ||
// this assumes that if it's a symlink it's OK. Arguably we | ||
// should instead follow the symlink. We'd have to expose | ||
// Lstat(), and guard against loops, and ... huge can of | ||
// worms, and as this validator is meant as a developer aid | ||
// more than anything else, not worth it IMHO (as I can't | ||
// imagine this happening by accident). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OTOH it would be nice to cover a scenario when Simplistic resolving can be done like this: https://play.golang.org/p/oDwwA5x6lBo Then provide something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the problem I don't want to address (and tried to point out in the comment) is that a symlink in a squashfs snap can look like
in Yes, it would be nicer. I don't think it's worth it (at least for now). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realised I need to do something about this anyway. Augh. |
||
if mode&(os.ModeDir|os.ModeNamedPipe|os.ModeSocket|os.ModeDevice) != 0 { | ||
logger.Noticef("in snap %q: %q should be a regular file (or a symlink) and isn't", s.Name(), path) | ||
hasBadModes = true | ||
} | ||
} | ||
if needsx[path] || strings.HasPrefix(path, "meta/hooks/") { | ||
if mode.Perm()&0111 == 0 { | ||
logger.Noticef("in snap %q: %q should be executable, and isn't: %s", s.Name(), path, mode) | ||
hasBadModes = true | ||
} | ||
} else { | ||
// in needsr, or under meta but not a hook | ||
if mode.Perm()&0444 != 0444 { | ||
logger.Noticef("in snap %q: %q should be world-readable, and isn't: %s", s.Name(), path, mode) | ||
hasBadModes = true | ||
} | ||
} | ||
} | ||
return nil | ||
}) | ||
if err != nil { | ||
return err | ||
} | ||
if len(seen) != len(needsx)+len(needsrx)+len(needsr) { | ||
for _, needs := range []map[string]bool{needsx, needsrx, needsr} { | ||
for path := range needs { | ||
if !seen[path] { | ||
logger.Noticef("in snap %q: path %q does not exist", s.Name(), path) | ||
} | ||
} | ||
} | ||
return ErrMissingPaths | ||
} | ||
|
||
if hasBadModes { | ||
return ErrBadModes | ||
} | ||
return nil | ||
} | ||
|
||
var openSnapFile = backend.OpenSnapFile | ||
|
||
// checkSnap ensures that the snap can be installed. | ||
func checkSnap(st *state.State, snapFilePath string, si *snap.SideInfo, curInfo *snap.Info, flags Flags) error { | ||
// This assumes that the snap was already verified or --dangerous was used. | ||
|
||
s, _, err := openSnapFile(snapFilePath, si) | ||
s, c, err := openSnapFile(snapFilePath, si) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -195,6 +377,10 @@ func checkSnap(st *state.State, snapFilePath string, si *snap.SideInfo, curInfo | |
return err | ||
} | ||
|
||
if err := validateContainer(s, c); err != nil { | ||
return err | ||
} | ||
|
||
st.Lock() | ||
defer st.Unlock() | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon my ignorance, but why an extra empty string (
""
) here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the last argument of
MakeTestSnapWithFiles
is a[][]string
, with the inner list's first element being the filename, and the second being its contents