From 6ee8705ae63ec9918f2cc19b1d903c5cdb0d5487 Mon Sep 17 00:00:00 2001 From: DCCooper <1866858@gmail.com> Date: Mon, 30 Nov 2020 19:40:14 +0800 Subject: [PATCH 3/4] cleancode:fix problems found by code review Signed-off-by: DCCooper <1866858@gmail.com> --- Makefile | 1 + builder/dockerfile/add_copy.go | 5 ++++- builder/dockerfile/builder.go | 7 +++++-- cmd/cli/build.go | 18 ++++++++++-------- cmd/cli/build_test.go | 6 ++---- cmd/daemon/main.go | 34 +++++++++++++++++++++++++++------- daemon/daemon.go | 15 +++++++++++++-- daemon/import.go | 8 ++++++-- daemon/load.go | 8 +++++--- daemon/login.go | 10 +++++++--- daemon/logout.go | 13 ++++++++----- exporter/common.go | 25 +++++++++++++++---------- store/store.go | 8 ++++---- util/common.go | 12 ++++++++++++ util/common_test.go | 18 +++++++++++++++--- 15 files changed, 134 insertions(+), 54 deletions(-) diff --git a/Makefile b/Makefile index f40941b4..cbace592 100644 --- a/Makefile +++ b/Makefile @@ -97,6 +97,7 @@ proto: install: install -D -m0551 bin/isula-build $(BINDIR) install -D -m0550 bin/isula-builder $(BINDIR) + @( getent group isula > /dev/null ) || ( groupadd --system isula ) @[ ! -d ${CONFIG_DIR}/${CONFIG_FILE} ] && install -dm0650 ${CONFIG_DIR} @( [ -f ${CONFIG_DIR}/${CONFIG_FILE} ] && printf "%-20s %s\n" "${CONFIG_FILE}" "already exist in ${CONFIG_DIR}, please replace it manually." ) || install -D -m0600 ${LOCAL_CONF_PREFIX}/${CONFIG_FILE} ${CONFIG_DIR}/${CONFIG_FILE} @( [ -f ${CONFIG_DIR}/${POLICY_FILE} ] && printf "%-20s %s\n" "${POLICY_FILE}" "already exist in ${CONFIG_DIR}, please replace it manually." ) || install -D -m0600 ${LOCAL_CONF_PREFIX}/${POLICY_FILE} ${CONFIG_DIR}/${POLICY_FILE} diff --git a/builder/dockerfile/add_copy.go b/builder/dockerfile/add_copy.go index c5d966e2..42cda7f9 100644 --- a/builder/dockerfile/add_copy.go +++ b/builder/dockerfile/add_copy.go @@ -143,7 +143,10 @@ func (c *cmdBuilder) getCopyContextDir(from string) (string, func(), error) { if err != nil { return "", nil, err } - c.stage.buildOpt.systemContext.DockerCertPath = filepath.Join(constant.DefaultCertRoot, server) + c.stage.buildOpt.systemContext.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, server) + if err != nil { + return "", nil, err + } // "from" is neither name nor index of stage, consider that "from" is image description imgDesc, err := prepareImage(&image.PrepareImageOptions{ diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index 757f4cd5..7e954f0d 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -22,13 +22,13 @@ import ( "io" "io/ioutil" "os" - "path/filepath" "regexp" "sort" "strings" "time" "github.com/containers/image/v5/docker/reference" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/opencontainers/go-digest" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -257,7 +257,10 @@ func (b *Builder) newStageBuilders() error { if err != nil { return err } - sb.buildOpt.systemContext.DockerCertPath = filepath.Join(constant.DefaultCertRoot, server) + sb.buildOpt.systemContext.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, server) + if err != nil { + return err + } b.stageBuilders = append(b.stageBuilders, sb) } diff --git a/cmd/cli/build.go b/cmd/cli/build.go index f2a49e02..3ebbb90f 100644 --- a/cmd/cli/build.go +++ b/cmd/cli/build.go @@ -171,6 +171,13 @@ func newBuildOptions(args []string) error { return nil } + // check cap list + for _, c := range buildOpts.capAddList { + if !util.CheckCap(c) { + return errors.Errorf("cap %v is invalid", c) + } + } + // the path may be a symbol link contextDir, err := filepath.Abs(args[0]) if err != nil { @@ -236,8 +243,9 @@ func modifyLocalTransporter(transport string, absPath string, segments []string) const validIsuladFieldsLen = 3 switch transport { case "docker-archive": - segments[1] = absPath - buildOpts.output = strings.Join(segments, ":") + newSeg := util.CopyStrings(segments) + newSeg[1] = absPath + buildOpts.output = strings.Join(newSeg, ":") return nil case "isulad": if len(segments) != validIsuladFieldsLen { @@ -311,12 +319,6 @@ func runBuild(ctx context.Context, cli Cli) (string, error) { digest string ) - for _, c := range buildOpts.capAddList { - if !util.CheckCap(c) { - return "", errors.Errorf("cap %v is invalid", c) - } - } - if err = checkAndProcessOutput(); err != nil { return "", err } diff --git a/cmd/cli/build_test.go b/cmd/cli/build_test.go index 1fa8ecc4..b4c3a61f 100644 --- a/cmd/cli/build_test.go +++ b/cmd/cli/build_test.go @@ -602,17 +602,15 @@ func TestRunBuildWithCap(t *testing.T) { defer tmpDir.Remove() buildOpts.file = tmpDir.Join("Dockerfile") buildOpts.output = "docker-daemon:cap:latest" - mockBuild := newMockDaemon() - ctx := context.Background() - cli := newMockClient(&mockGrpcClient{imageBuildFunc: mockBuild.build}) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { buildOpts.capAddList = tt.caps - _, err := runBuild(ctx, &cli) + err := newBuildOptions([]string{tmpDir.Path()}) if tt.isErr { assert.ErrorContains(t, err, "is invalid") } + buildOpts.capAddList = nil }) } } diff --git a/cmd/daemon/main.go b/cmd/daemon/main.go index 9fcde5cd..d6f1d080 100644 --- a/cmd/daemon/main.go +++ b/cmd/daemon/main.go @@ -22,6 +22,7 @@ import ( "github.com/BurntSushi/toml" "github.com/containers/storage/pkg/reexec" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/spf13/cobra" @@ -157,9 +158,17 @@ func before(cmd *cobra.Command) error { logrus.SetOutput(os.Stdout) logrus.SetFormatter(&logrus.TextFormatter{FullTimestamp: true}) + runRoot, err := securejoin.SecureJoin(daemonOpts.RunRoot, "storage") + if err != nil { + return err + } + dataRoot, err := securejoin.SecureJoin(daemonOpts.DataRoot, "storage") + if err != nil { + return err + } store.SetDefaultStoreOptions(store.DaemonStoreOptions{ - RunRoot: filepath.Join(daemonOpts.RunRoot, "storage"), - DataRoot: filepath.Join(daemonOpts.DataRoot, "storage"), + RunRoot: runRoot, + DataRoot: dataRoot, Driver: daemonOpts.StorageDriver, DriverOption: util.CopyStrings(daemonOpts.StorageOpts), }) @@ -204,7 +213,7 @@ func loadConfig(path string) (config.TomlConfig, error) { return conf, err } -func mergeStorageConfig(cmd *cobra.Command) { +func mergeStorageConfig(cmd *cobra.Command) error { store.SetDefaultConfigFilePath(constant.StorageConfigPath) option, err := store.GetDefaultStoreOptions(true) if err == nil { @@ -218,10 +227,16 @@ func mergeStorageConfig(cmd *cobra.Command) { var storeOpt store.DaemonStoreOptions if option.RunRoot == "" { - storeOpt.RunRoot = filepath.Join(daemonOpts.RunRoot, "storage") + storeOpt.RunRoot, err = securejoin.SecureJoin(daemonOpts.RunRoot, "storage") + if err != nil { + return err + } } if option.GraphRoot == "" { - storeOpt.DataRoot = filepath.Join(daemonOpts.DataRoot, "storage") + storeOpt.DataRoot, err = securejoin.SecureJoin(daemonOpts.DataRoot, "storage") + if err != nil { + return err + } } if daemonOpts.StorageDriver != "" { storeOpt.Driver = daemonOpts.StorageDriver @@ -230,6 +245,8 @@ func mergeStorageConfig(cmd *cobra.Command) { storeOpt.DriverOption = util.CopyStrings(daemonOpts.StorageOpts) } store.SetDefaultStoreOptions(storeOpt) + + return nil } func mergeConfig(conf config.TomlConfig, cmd *cobra.Command) { @@ -258,7 +275,10 @@ func setupWorkingDirectories() error { return errors.Errorf("runroot(%q) and dataroot(%q) must be different paths", daemonOpts.RunRoot, daemonOpts.DataRoot) } - buildTmpDir := filepath.Join(daemonOpts.DataRoot, dataRootTmpDirPrefix) + buildTmpDir, err := securejoin.SecureJoin(daemonOpts.DataRoot, dataRootTmpDirPrefix) + if err != nil { + return err + } dirs := []string{daemonOpts.DataRoot, daemonOpts.RunRoot, buildTmpDir} for _, dir := range dirs { if !filepath.IsAbs(dir) { @@ -325,7 +345,7 @@ func checkAndValidateConfig(cmd *cobra.Command) error { // if storage config file exists, merge storage config if util.IsExist(constant.StorageConfigPath) { - mergeStorageConfig(cmd) + return mergeStorageConfig(cmd) } return nil diff --git a/daemon/daemon.go b/daemon/daemon.go index 1237f35d..90ccf648 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -23,6 +23,7 @@ import ( "time" "github.com/containerd/containerd/sys/reaper" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" @@ -130,10 +131,20 @@ func (d *Daemon) Run() (err error) { // NewBuilder returns the builder with request sent from GRPC service func (d *Daemon) NewBuilder(ctx context.Context, req *pb.BuildRequest) (b builder.Builder, err error) { + var ( + buildDir string + runDir string + ) // buildDir is used to set directory which is used to store tmp data - buildDir := filepath.Join(d.opts.DataRoot, dataRootTmpDirPrefix, req.BuildID) + buildDir, err = securejoin.SecureJoin(d.opts.DataRoot, filepath.Join(dataRootTmpDirPrefix, req.BuildID)) + if err != nil { + return nil, err + } // runDir is used to store such as container bundle directories - runDir := filepath.Join(d.opts.RunRoot, req.BuildID) + runDir, err = securejoin.SecureJoin(d.opts.RunRoot, req.BuildID) + if err != nil { + return nil, err + } // this key with BuildDir will be used by exporter to save blob temporary // NOTE: keep it be updated before NewBuilder. ctx will be taken by Builder diff --git a/daemon/import.go b/daemon/import.go index a72d732e..21ffeaa3 100644 --- a/daemon/import.go +++ b/daemon/import.go @@ -22,6 +22,7 @@ import ( "github.com/containers/image/v5/tarball" "github.com/containers/image/v5/transports" "github.com/containers/image/v5/types" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sync/errgroup" @@ -43,6 +44,7 @@ func (b *Backend) Import(req *pb.ImportRequest, stream pb.Control_ImportServer) source = req.Source reference = req.Reference importID = req.ImportID + tmpDir string ) logEntry := logrus.WithFields(logrus.Fields{"ImportID": importID}) logEntry.Info("ImportRequest received") @@ -78,8 +80,10 @@ func (b *Backend) Import(req *pb.ImportRequest, stream pb.Control_ImportServer) log := logger.NewCliLogger(constant.CliLogBufferLen) imageCopyOptions := image.NewImageCopyOptions(log) - - tmpDir := filepath.Join(b.daemon.opts.DataRoot, dataRootTmpDirPrefix, importID) + tmpDir, err = securejoin.SecureJoin(b.daemon.opts.DataRoot, filepath.Join(dataRootTmpDirPrefix, importID)) + if err != nil { + return err + } if err = os.MkdirAll(tmpDir, constant.DefaultRootDirMode); err != nil { logEntry.Error(err) return err diff --git a/daemon/load.go b/daemon/load.go index d3f9bf6e..1f4c2f87 100644 --- a/daemon/load.go +++ b/daemon/load.go @@ -14,10 +14,9 @@ package daemon import ( - "path/filepath" - "github.com/containers/image/v5/docker/tarfile" "github.com/containers/storage" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sync/errgroup" @@ -85,7 +84,10 @@ func (b *Backend) Load(req *pb.LoadRequest, stream pb.Control_LoadServer) error func getRepoTagFromImageTar(dataRoot, path string) ([]string, error) { // tmp dir will be removed after NewSourceFromFileWithContext - tmpDir := filepath.Join(dataRoot, dataRootTmpDirPrefix) + tmpDir, err := securejoin.SecureJoin(dataRoot, dataRootTmpDirPrefix) + if err != nil { + return nil, err + } systemContext := image.GetSystemContext() systemContext.BigFilesTemporaryDir = tmpDir diff --git a/daemon/login.go b/daemon/login.go index 012816a4..e3399983 100644 --- a/daemon/login.go +++ b/daemon/login.go @@ -16,11 +16,11 @@ package daemon import ( "context" "crypto" - "path/filepath" "github.com/containers/image/v5/docker" "github.com/containers/image/v5/pkg/docker/config" "github.com/containers/image/v5/types" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -48,12 +48,16 @@ func (b *Backend) Login(ctx context.Context, req *pb.LoginRequest) (*pb.LoginRes "Username": req.GetUsername(), }).Info("LoginRequest received") - if err := validLoginOpts(req); err != nil { + err := validLoginOpts(req) + if err != nil { return &pb.LoginResponse{Content: loginFailed}, err } sysCtx := image.GetSystemContext() - sysCtx.DockerCertPath = filepath.Join(constant.DefaultCertRoot, req.Server) + sysCtx.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, req.Server) + if err != nil { + return &pb.LoginResponse{Content: loginFailed}, err + } if loginWithAuthFile(req) { auth, err := config.GetCredentials(sysCtx, req.Server) diff --git a/daemon/logout.go b/daemon/logout.go index 82c5beac..355b1f7a 100644 --- a/daemon/logout.go +++ b/daemon/logout.go @@ -16,10 +16,10 @@ package daemon import ( "context" "fmt" - "path/filepath" "strings" "github.com/containers/image/v5/pkg/docker/config" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -35,12 +35,16 @@ func (b *Backend) Logout(ctx context.Context, req *pb.LogoutRequest) (*pb.Logout "All": req.GetAll(), }).Info("LogoutRequest received") - if err := validLogoutOpts(req); err != nil { + err := validLogoutOpts(req) + if err != nil { return &pb.LogoutResponse{Result: "Logout Failed"}, err } sysCtx := image.GetSystemContext() - sysCtx.DockerCertPath = filepath.Join(constant.DefaultCertRoot, req.Server) + sysCtx.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, req.Server) + if err != nil { + return &pb.LogoutResponse{Result: "Logout Failed"}, err + } if req.All { if err := config.RemoveAllAuthentication(sysCtx); err != nil { @@ -51,8 +55,7 @@ func (b *Backend) Logout(ctx context.Context, req *pb.LogoutRequest) (*pb.Logout return &pb.LogoutResponse{Result: "Removed authentications"}, nil } - err := config.RemoveAuthentication(sysCtx, req.Server) - if err == nil { + if err = config.RemoveAuthentication(sysCtx, req.Server); err == nil { msg := fmt.Sprintf("Removed authentication for %s", req.Server) logrus.Infof("Success logout from server: %q", req.Server) return &pb.LogoutResponse{Result: msg}, nil diff --git a/exporter/common.go b/exporter/common.go index 1953b4e1..6e70a38e 100644 --- a/exporter/common.go +++ b/exporter/common.go @@ -20,7 +20,6 @@ import ( "io" "os" "os/exec" - "path/filepath" "strings" cp "github.com/containers/image/v5/copy" @@ -30,6 +29,7 @@ import ( "github.com/containers/image/v5/types" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/stringid" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/docker/distribution/reference" "github.com/opencontainers/go-digest" "github.com/pkg/errors" @@ -72,15 +72,12 @@ func Export(src, destSpec string, opts ExportOptions, localStore *store.Store) e if err != nil { return err } - opts.SystemContext.DockerCertPath = filepath.Join(constant.DefaultCertRoot, registry) - - options := NewCopyOptions(opts) - - policyContext, err := NewPolicyContext(opts.SystemContext) + opts.SystemContext.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, registry) if err != nil { return err } - ref, digest, err := export(opts, epter, policyContext, options) + + ref, digest, err := export(epter, opts) if err != nil { return errors.Errorf("export image from %s to %s failed, got error: %s", src, destSpec, err) } @@ -117,13 +114,18 @@ func exportToIsulad(ctx context.Context, tarPath string) error { return nil } -func export(exOpts ExportOptions, e Exporter, policyContext *signature.PolicyContext, cpOpts *cp.Options) (reference.Canonical, digest.Digest, error) { +func export(e Exporter, exOpts ExportOptions) (reference.Canonical, digest.Digest, error) { var ( - err error ref reference.Canonical manifestBytes []byte manifestDigest digest.Digest ) + + cpOpts := NewCopyOptions(exOpts) + policyContext, err := NewPolicyContext(exOpts.SystemContext) + if err != nil { + return nil, "", err + } defer func() { destroyErr := policyContext.Destroy() if err == nil { @@ -176,7 +178,10 @@ func parseExporter(opts ExportOptions, src, destSpec string, localStore *store.S // 3. get dest reference if parts[0] == "isulad" { randomID := stringid.GenerateNonCryptoID()[:constant.DefaultIDLen] - isuladTarPath = filepath.Join(opts.DataDir, fmt.Sprintf("isula-build-tmp-%s.tar", randomID)) + isuladTarPath, err = securejoin.SecureJoin(opts.DataDir, fmt.Sprintf("isula-build-tmp-%s.tar", randomID)) + if err != nil { + return nil, "", err + } // construct format: transport:path:image:tag // parts[1] here could not be empty cause client-end already processed it destSpec = fmt.Sprintf("docker-archive:%s:%s", isuladTarPath, parts[1]) diff --git a/store/store.go b/store/store.go index 263d69e8..410eef11 100644 --- a/store/store.go +++ b/store/store.go @@ -131,15 +131,15 @@ func (s *Store) CleanContainer(id string) error { // Do not care about all the errors whiling cleaning the container, // just return one if the error occurs. - var err error + var finalErr error if _, uerr := s.Unmount(id, false); uerr != nil { - err = uerr + finalErr = uerr logrus.Warnf("Unmount container store failed while cleaning %q", id) } if derr := s.DeleteContainer(id); derr != nil { - err = derr + finalErr = derr logrus.Warnf("Delete container store failed while cleaning %q", id) } - return err + return finalErr } diff --git a/util/common.go b/util/common.go index 9e2e2537..5cd4bb28 100644 --- a/util/common.go +++ b/util/common.go @@ -20,6 +20,7 @@ import ( "path/filepath" "strings" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/pkg/errors" "github.com/spf13/cobra" "golang.org/x/sys/unix" @@ -152,6 +153,17 @@ func ParseServer(server string) (string, error) { return "", errors.Errorf("invalid registry address %s", server) } + // to prevent directory traversal + fakePrefix := "/fakePrefix" + origAddr := fmt.Sprintf("%s/%s", fakePrefix, fields[0]) + cleanAddr, err := securejoin.SecureJoin(fakePrefix, fields[0]) + if err != nil { + return "", err + } + if cleanAddr != origAddr { + return "", errors.Errorf("invalid relative path detected") + } + return fields[0], nil } diff --git a/util/common_test.go b/util/common_test.go index b48c508f..e9b6ad85 100644 --- a/util/common_test.go +++ b/util/common_test.go @@ -153,17 +153,29 @@ func TestParseServer(t *testing.T) { want: "", wantErr: true, }, + { + name: "TC10 - abnormal server address with relative filepath", + args: args{server: "https://mydockerhub/../../../"}, + want: "mydockerhub", + wantErr: false, + }, + { + name: "TC11 - abnormal server address with relative filepath 2", + args: args{server: "https://../../../../mydockerhub"}, + want: "", + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := ParseServer(tt.args.server) + if got != tt.want { + t.Errorf("ParseServer() got = %v, want %v", got, tt.want) + } if (err != nil) != tt.wantErr { t.Errorf("ParseServer() error = %v, wantErr %v", err, tt.wantErr) return } - if got != tt.want { - t.Errorf("ParseServer() got = %v, want %v", got, tt.want) - } }) } } -- 2.27.0