updated per code review

Signed-off-by: Patrick Zheng <[email protected]>

cmd/notation/plugin/install.go

@@ -107,7 +107,7 @@ Example - Install plugin from URL, SHA256 checksum is required:
 	}
 	opts.LoggingFlagOpts.ApplyFlags(command.Flags())
 	command.Flags().BoolVar(&opts.isFile, "file", false, "install plugin from a file on file system")
-	command.Flags().BoolVar(&opts.isURL, "url", false, fmt.Sprintf("install plugin from an HTTPS URL. The default plugin download timeout is %s", notationplugin.DownloadPluginFromURLTimeout))
+	command.Flags().BoolVar(&opts.isURL, "url", false, fmt.Sprintf("install plugin from an HTTPS URL. The plugin download timeout is %s", notationplugin.DownloadPluginFromURLTimeout))
 	command.Flags().StringVar(&opts.inputChecksum, "sha256sum", "", "must match SHA256 of the plugin source, required when \"--url\" flag is set")
 	command.Flags().BoolVar(&opts.force, "force", false, "force the installation of the plugin")
 	command.MarkFlagsMutuallyExclusive("file", "url")
@@ -160,11 +160,11 @@ func install(command *cobra.Command, opts *pluginInstallOpts) error {
 // installPlugin installs the plugin given plugin source path
 func installPlugin(ctx context.Context, inputPath string, inputChecksum string, force bool) error {
 	// sanity check
-	inputFileStat, err := os.Stat(inputPath)
+	inputFileInfo, err := os.Stat(inputPath)
 	if err != nil {
 		return err
 	}
-	if !inputFileStat.Mode().IsRegular() {
+	if !inputFileInfo.Mode().IsRegular() {
 		return fmt.Errorf("%s is not a valid file", inputPath)
 	}
 	// checksum check
@@ -185,12 +185,21 @@ func installPlugin(ctx context.Context, inputPath string, inputChecksum string,
 			return err
 		}
 		defer rc.Close()
+		// check for '..' in file name to avoid zip slip vulnerability
+		for _, f := range rc.File {
+			if strings.Contains(f.Name, "..") {
+				return fmt.Errorf("file name in zip cannot contain '..', but found %q", f.Name)
+			}
+		}
 		return installPluginFromFS(ctx, rc, force)
 	case notationplugin.MediaTypeGzip:
 		// when file is gzip, required to be tar
 		return installPluginFromTarGz(ctx, inputPath, force)
 	default:
 		// input file is not in zip or gzip, try install directly
+		if inputFileInfo.Size() >= osutil.MaxFileBytes {
+			return fmt.Errorf("file size reached the %d MiB size limit", osutil.MaxFileBytes/1024/1024)
+		}
 		installOpts := plugin.CLIInstallOptions{
 			PluginPath: inputPath,
 			Overwrite:  force,
@@ -213,6 +222,7 @@ func installPluginFromFS(ctx context.Context, pluginFS fs.FS, force bool) error
 		return fmt.Errorf("failed to create temporary directory: %w", err)
 	}
 	defer os.RemoveAll(tmpDir)
+	var pluginFileSize int64
 	if err := fs.WalkDir(pluginFS, root, func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return err
@@ -225,11 +235,15 @@ func installPluginFromFS(ctx context.Context, pluginFS fs.FS, force bool) error
 		if err != nil {
 			return err
 		}
-		// only accept regular files.
-		// check for `..` in fName to avoid zip slip vulnerability
-		if !info.Mode().IsRegular() || strings.Contains(fName, "..") {
+		// only accept regular files
+		if !info.Mode().IsRegular() {
 			return nil
 		}
+		// check for plugin file size to avoid zip bomb vulnerability
+		pluginFileSize += info.Size()
+		if pluginFileSize >= osutil.MaxFileBytes {
+			return fmt.Errorf("total file size reached the %d MiB size limit", osutil.MaxFileBytes/1024/1024)
+		}
 		logger.Debugf("Extracting file %s...", fName)
 		rc, err := pluginFS.Open(path)
 		if err != nil {
@@ -270,6 +284,7 @@ func installPluginFromTarGz(ctx context.Context, tarGzPath string, force bool) e
 		return fmt.Errorf("failed to create temporary directory: %w", err)
 	}
 	defer os.RemoveAll(tmpDir)
+	var pluginFileSize int64
 	for {
 		header, err := tarReader.Next()
 		if err != nil {
@@ -278,11 +293,19 @@ func installPluginFromTarGz(ctx context.Context, tarGzPath string, force bool) e
 			}
 			return err
 		}
-		// only accept regular files.
-		// check for `..` in fName to avoid zip slip vulnerability
-		if !header.FileInfo().Mode().IsRegular() || strings.Contains(header.Name, "..") {
+		// check for '..' in file name to avoid zip slip vulnerability
+		if strings.Contains(header.Name, "..") {
+			return fmt.Errorf("file name in tar.gz cannot contain '..', but found %q", header.Name)
+		}
+		// only accept regular files
+		if !header.FileInfo().Mode().IsRegular() {
 			continue
 		}
+		// check for plugin file size to avoid zip bomb vulnerability
+		pluginFileSize += header.FileInfo().Size()
+		if pluginFileSize >= osutil.MaxFileBytes {
+			return fmt.Errorf("total file size reached the %d MiB size limit", osutil.MaxFileBytes/1024/1024)
+		}
 		fName := filepath.Base(header.Name)
 		logger.Debugf("Extracting file %s...", fName)
 		tmpFilePath := filepath.Join(tmpDir, fName)

internal/osutil/file.go

@@ -26,7 +26,7 @@ import (
 )
 
 // MaxFileBytes is the maximum file bytes.
-// When used, the value should strictly less than this number.
+// When used, the value should be strictly less than this number.
 var MaxFileBytes int64 = 256 * 1024 * 1024 // 256 MiB
 
 // WriteFile writes to a path with all parent directories created.
@@ -104,25 +104,18 @@ func IsRegularFile(path string) (bool, error) {
 }
 
 // CopyFromReaderToDir copies file from src to dst where dst is the destination
-// file path. The file size must be less than 256 MiB.
+// file path.
 func CopyFromReaderToDir(src io.Reader, dst string, perm fs.FileMode) error {
 	dstFile, err := os.Create(dst)
 	if err != nil {
 		return err
 	}
-	lr := &io.LimitedReader{
-		R: src,
-		N: MaxFileBytes,
-	}
-	if _, err := io.Copy(dstFile, lr); err != nil || lr.N == 0 {
-		_ = dstFile.Close()
-		if err != nil {
-			return err
-		}
-		return fmt.Errorf("file reached the %d MiB size limit", MaxFileBytes/1024/1024)
+	if _, err := io.Copy(dstFile, src); err != nil {
+		dstFile.Close()
+		return err
 	}
 	if err := dstFile.Chmod(perm); err != nil {
-		_ = dstFile.Close()
+		dstFile.Close()
 		return err
 	}
 	return dstFile.Close()

specs/commandline/plugin.md

@@ -52,7 +52,7 @@ Flags:
       --force              force the installation of the plugin
   -h, --help               help for install
       --sha256sum string   must match SHA256 of the plugin source, required when "--url" flag is set
-      --url                install plugin from an HTTPS URL. The default plugin download timeout is 10m0s
+      --url                install plugin from an HTTPS URL. The plugin download timeout is 10m0s
   -v, --verbose            verbose mode
 
 Aliases:
@@ -82,7 +82,7 @@ Aliases:
 
 ### Install a plugin from file system
 
-Install a Notation plugin from file system. Plugin file supports `.zip`, `.tar.gz`, and single plugin executable file format. The checksum validation is optional for this case. 
+Install a Notation plugin from the host file system. `.zip`, `.tar.gz`, and `single plugin executable file` formats are supported. In this scenario, SHA-256 checksum validation is optional.
 
 ```shell
 $ notation plugin install --file <file_path>
@@ -120,7 +120,7 @@ It is not recommended to install an older version. To force the installation, us
 ```
 ### Install a plugin from URL
 
-Install a Notation plugin from a remote location and verify the plugin checksum. Notation only supports installing plugins from an HTTPS URL, which means that the URL must start with "https://".
+Install a Notation plugin from a URL. Notation only supports HTTPS URL, which means that the URL must start with "https://". The URL MUST point to a resource in `.zip`, `.tar.gz`, or `single plugin executable file` format.  In this scenario, the SHA-256 checksum of the resource MUST be provided.
 
 ```shell
 $ notation plugin install --sha256sum <digest> --url <HTTPS_URL>

test/e2e/suite/plugin/install.go

@@ -60,17 +60,31 @@ var _ = Describe("notation plugin install", func() {
 		})
 	})
 
-	It("with content inside zip archive exceeds 256 MiB size limit", func() {
+	It("with zip bomb single file exceeds 256 MiB size limit in zip format", func() {
 		Host(nil, func(notation *utils.ExecOpts, _ *Artifact, vhost *utils.VirtualHost) {
 			notation.ExpectFailure().Exec("plugin", "install", "--file", NotationE2EMaliciousPluginArchivePath+"/large_file_zip.zip", "-v").
-				MatchErrContent("Error: plugin installation failed: file reached the 256 MiB size limit\n")
+				MatchErrContent("Error: plugin installation failed: total file size reached the 256 MiB size limit\n")
 		})
 	})
 
-	It("with content inside tar.gz archive exceeds 256 MiB size limit", func() {
+	It("with zip bomb single file exceeds 256 MiB size limit in tar.gz format", func() {
 		Host(nil, func(notation *utils.ExecOpts, _ *Artifact, vhost *utils.VirtualHost) {
 			notation.ExpectFailure().Exec("plugin", "install", "--file", NotationE2EMaliciousPluginArchivePath+"/large_file_tarGz.tar.gz", "-v").
-				MatchErrContent("Error: plugin installation failed: file reached the 256 MiB size limit\n")
+				MatchErrContent("Error: plugin installation failed: total file size reached the 256 MiB size limit\n")
+		})
+	})
+
+	It("with zip slip", func() {
+		Host(nil, func(notation *utils.ExecOpts, _ *Artifact, vhost *utils.VirtualHost) {
+			notation.ExpectFailure().Exec("plugin", "install", "--file", NotationE2EMaliciousPluginArchivePath+"/large_file_tarGz.tar.gz", "-v").
+				MatchErrContent("Error: plugin installation failed: total file size reached the 256 MiB size limit\n")
+		})
+	})
+
+	It("with zip bomb total file size exceeds 256 MiB size limit", func() {
+		Host(nil, func(notation *utils.ExecOpts, _ *Artifact, vhost *utils.VirtualHost) {
+			notation.ExpectFailure().Exec("plugin", "install", "--file", NotationE2EMaliciousPluginArchivePath+"/zip_slip.zip", "-v").
+				MatchErrContent("Error: plugin installation failed: file name in zip cannot contain '..', but found \"../../../../../../../../tmp/evil.sh\"\n")
 		})
 	})