fix: add path sanitization and EOF validation in CopyFromInstance

- Use securejoin to prevent path traversal attacks
- Return error when stream ends without completion marker

Author: rgarcia

cmd/api/api/cp.go

@@ -290,6 +290,8 @@ func (s *ApiService) handleCopyFrom(ctx context.Context, ws *websocket.Conn, ins
 		return fmt.Errorf("start copy stream: %w", err)
 	}
 
+	var receivedFinal bool
+
 	// Stream responses to WebSocket client
 	for {
 		resp, err := stream.Recv()
@@ -334,6 +336,7 @@ func (s *ApiService) handleCopyFrom(ctx context.Context, ws *websocket.Conn, ins
 				return fmt.Errorf("write end: %w", err)
 			}
 			if r.End.Final {
+				receivedFinal = true
 				return nil
 			}
 
@@ -349,6 +352,9 @@ func (s *ApiService) handleCopyFrom(ctx context.Context, ws *websocket.Conn, ins
 		}
 	}
 
+	if !receivedFinal {
+		return fmt.Errorf("copy stream ended without completion marker")
+	}
 	return nil
 }
 

docker-cp.txt

@@ -0,0 +1,64 @@
+docker container cp
+Description	Copy files/folders between a container and the local filesystem
+Usage	docker container cp [OPTIONS] CONTAINER:SRC_PATH DEST_PATH|- docker cp [OPTIONS] SRC_PATH|- CONTAINER:DEST_PATH
+Aliases
+docker cp
+Description
+The docker cp utility copies the contents of SRC_PATH to the DEST_PATH. You can copy from the container's file system to the local machine or the reverse, from the local filesystem to the container. If - is specified for either the SRC_PATH or DEST_PATH, you can also stream a tar archive from STDIN or to STDOUT. The CONTAINER can be a running or stopped container. The SRC_PATH or DEST_PATH can be a file or directory.
+
+The docker cp command assumes container paths are relative to the container's / (root) directory. This means supplying the initial forward slash is optional; The command sees compassionate_darwin:/tmp/foo/myfile.txt and compassionate_darwin:tmp/foo/myfile.txt as identical. Local machine paths can be an absolute or relative value. The command interprets a local machine's relative paths as relative to the current working directory where docker cp is run.
+
+The cp command behaves like the Unix cp -a command in that directories are copied recursively with permissions preserved if possible. Ownership is set to the user and primary group at the destination. For example, files copied to a container are created with UID:GID of the root user. Files copied to the local machine are created with the UID:GID of the user which invoked the docker cp command. However, if you specify the -a option, docker cp sets the ownership to the user and primary group at the source. If you specify the -L option, docker cp follows any symbolic link in the SRC_PATH. docker cp doesn't create parent directories for DEST_PATH if they don't exist.
+
+Assuming a path separator of /, a first argument of SRC_PATH and second argument of DEST_PATH, the behavior is as follows:
+
+SRC_PATH specifies a file
+DEST_PATH does not exist
+the file is saved to a file created at DEST_PATH
+DEST_PATH does not exist and ends with /
+Error condition: the destination directory must exist.
+DEST_PATH exists and is a file
+the destination is overwritten with the source file's contents
+DEST_PATH exists and is a directory
+the file is copied into this directory using the basename from SRC_PATH
+SRC_PATH specifies a directory
+DEST_PATH does not exist
+DEST_PATH is created as a directory and the contents of the source directory are copied into this directory
+DEST_PATH exists and is a file
+Error condition: cannot copy a directory to a file
+DEST_PATH exists and is a directory
+SRC_PATH does not end with /. (that is: slash followed by dot)
+the source directory is copied into this directory
+SRC_PATH does end with /. (that is: slash followed by dot)
+the content of the source directory is copied into this directory
+The command requires SRC_PATH and DEST_PATH to exist according to the above rules. If SRC_PATH is local and is a symbolic link, the symbolic link, not the target, is copied by default. To copy the link target and not the link, specify the -L option.
+
+A colon (:) is used as a delimiter between CONTAINER and its path. You can also use : when specifying paths to a SRC_PATH or DEST_PATH on a local machine, for example file:name.txt. If you use a : in a local machine path, you must be explicit with a relative or absolute path, for example:
+
+`/path/to/file:name.txt` or `./file:name.txt`
+Options
+Option	Default	Description
+-a, --archive		Archive mode (copy all uid/gid information)
+-L, --follow-link		Always follow symbol link in SRC_PATH
+-q, --quiet		Suppress progress output during copy. Progress output is automatically suppressed if no terminal is attached
+Examples
+Copy a local file into container
+
+
+$ docker cp ./some_file CONTAINER:/work
+Copy files from container to local path
+
+
+$ docker cp CONTAINER:/var/logs/ /tmp/app_logs
+Copy a file from container to stdout. Note cp command produces a tar stream
+
+
+$ docker cp CONTAINER:/var/logs/app.log - | tar x -O | grep "ERROR"
+Corner cases
+It isn't possible to copy certain system files such as resources under /proc, /sys, /dev, tmpfs, and mounts created by the user in the container. However, you can still copy such files by manually running tar in docker exec. Both of the following examples do the same thing in different ways (consider SRC_PATH and DEST_PATH are directories):
+
+
+$ docker exec CONTAINER tar Ccf $(dirname SRC_PATH) - $(basename SRC_PATH) | tar Cxf DEST_PATH -
+
+$ tar Ccf $(dirname SRC_PATH) - $(basename SRC_PATH) | docker exec -i CONTAINER tar Cxf DEST_PATH -
+Using - as the SRC_PATH streams the contents of STDIN as a tar archive. The command extracts the content of the tar to the DEST_PATH in container's filesystem. In this case, DEST_PATH must specify a directory. Using - as the DEST_PATH streams the contents of the resource as a tar archive to STDOUT.

guest_agent

@@ -15,6 +15,7 @@ import (
 	"sync/atomic"
 	"time"
 
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials/insecure"
 )
@@ -466,6 +467,7 @@ func CopyFromInstance(ctx context.Context, vsockSocketPath string, opts CopyFrom
 
 	var currentFile *os.File
 	var currentHeader *CopyFromGuestHeader
+	var receivedFinal bool
 
 	// Ensure file is closed on error paths
 	defer func() {
@@ -492,7 +494,11 @@ func CopyFromInstance(ctx context.Context, vsockSocketPath string, opts CopyFrom
 			}
 
 			currentHeader = r.Header
-			targetPath := filepath.Join(opts.DstPath, r.Header.Path)
+			// Use securejoin to prevent path traversal attacks
+			targetPath, err := securejoin.SecureJoin(opts.DstPath, r.Header.Path)
+			if err != nil {
+				return fmt.Errorf("invalid path %s: %w", r.Header.Path, err)
+			}
 
 			if r.Header.IsDir {
 				if err := os.MkdirAll(targetPath, fs.FileMode(r.Header.Mode)); err != nil {
@@ -529,14 +535,17 @@ func CopyFromInstance(ctx context.Context, vsockSocketPath string, opts CopyFrom
 				currentFile.Close()
 				// Set modification time
 				if currentHeader != nil && currentHeader.Mtime > 0 {
-					targetPath := filepath.Join(opts.DstPath, currentHeader.Path)
-					mtime := time.Unix(currentHeader.Mtime, 0)
-					os.Chtimes(targetPath, mtime, mtime)
+					targetPath, err := securejoin.SecureJoin(opts.DstPath, currentHeader.Path)
+					if err == nil {
+						mtime := time.Unix(currentHeader.Mtime, 0)
+						os.Chtimes(targetPath, mtime, mtime)
+					}
 				}
 				currentFile = nil
 				currentHeader = nil
 			}
 			if r.End.Final {
+				receivedFinal = true
 				return nil
 			}
 
@@ -545,6 +554,9 @@ func CopyFromInstance(ctx context.Context, vsockSocketPath string, opts CopyFrom
 		}
 	}
 
+	if !receivedFinal {
+		return fmt.Errorf("copy stream ended without completion marker")
+	}
 	return nil
 }