codeintel: fix panic in extracting JARs with absolute paths (#24652)

Author: Strum355

cmd/gitserver/server/vcs_syncer_jvm_packages.go

@@ -345,18 +345,7 @@ func unzipJarFile(jarPath, destination string) (err error) {
 	defer reader.Close()
 	destinationDirectory := strings.TrimSuffix(destination, string(os.PathSeparator)) + string(os.PathSeparator)
 
-	var (
-		_file      *zip.File
-		outputPath string
-	)
-	defer func() {
-		if r := recover(); r != nil {
-			err = errors.Newf("panic in extracting JAR: jarPath=%s file=%s jarFilesCount=%d outputPath=%s panic=%v", jarPath, _file.Name, len(reader.File), outputPath, r)
-		}
-	}()
-
 	for _, file := range reader.File {
-		_file = file
 		if strings.HasPrefix(file.Name, ".git/") {
 			// For security reasons, don't unzip files under the `.git/`
 			// directory. See https://github.com/sourcegraph/security-issues/issues/163
@@ -368,14 +357,19 @@ func unzipJarFile(jarPath, destination string) (err error) {
 			// `file.Name` docstring.
 			continue
 		}
-		outputPath = path.Join(destination, file.Name)
+		if strings.HasPrefix(file.Name, "/") {
+			// Skip absolute paths. While they are extracted relative to `destination`,
+			// they should be unimportant. Related issue https://github.com/golang/go/issues/48085#issuecomment-912659635
+			continue
+		}
+		outputPath := path.Join(destination, file.Name)
 		if !strings.HasPrefix(outputPath, destinationDirectory) {
 			// For security reasons, skip file if it's not a child
 			// of the target directory. See "Zip Slip Vulnerability".
 			continue
 		}
 
-		err := copyZipFileEntry(reader, file, outputPath)
+		err := copyZipFileEntry(file, outputPath)
 		if err != nil {
 			return err
 		}
@@ -384,8 +378,8 @@ func unzipJarFile(jarPath, destination string) (err error) {
 	return nil
 }
 
-func copyZipFileEntry(reader *zip.ReadCloser, entry *zip.File, outputPath string) (err error) {
-	inputFile, err := reader.Open(entry.Name)
+func copyZipFileEntry(entry *zip.File, outputPath string) (err error) {
+	inputFile, err := entry.Open()
 	if err != nil {
 		return err
 	}

cmd/gitserver/server/vcs_syncer_jvm_packages_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	"github.com/sourcegraph/sourcegraph/internal/codeintel/stores/dbstore"
+	"github.com/sourcegraph/sourcegraph/internal/conf/reposource"
 	"github.com/sourcegraph/sourcegraph/internal/extsvc/jvmpackages/coursier"
 	"github.com/sourcegraph/sourcegraph/internal/vcs"
 	"github.com/sourcegraph/sourcegraph/schema"
@@ -117,6 +118,51 @@ func (s JVMPackagesSyncer) runCloneCommand(t *testing.T, bareGitDirectory string
 	assert.Nil(t, cmd.Run())
 }
 
+func TestNoMaliciousFiles(t *testing.T) {
+	dir, err := os.MkdirTemp("", "")
+	assert.Nil(t, err)
+	defer os.RemoveAll(dir)
+
+	jarPath := path.Join(dir, "sampletext.zip")
+	extractPath := path.Join(dir, "extracted")
+	assert.Nil(t, os.Mkdir(extractPath, os.ModePerm))
+
+	createMaliciousJar(t, jarPath)
+
+	s := JVMPackagesSyncer{
+		Config:  &schema.JVMPackagesConnection{Maven: &schema.Maven{Dependencies: []string{}}},
+		DBStore: &simpleJVMPackageDBStoreMock{},
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel() // cancel now  to prevent any network IO
+	err = s.commitJar(ctx, reposource.MavenDependency{}, extractPath, jarPath, &schema.JVMPackagesConnection{Maven: &schema.Maven{}})
+	assert.NotNil(t, err)
+
+	files, err := os.ReadDir(extractPath)
+	assert.Nil(t, err)
+	assert.Equal(t, 2, len(files))
+}
+
+func createMaliciousJar(t *testing.T, name string) {
+	f, err := os.Create(name)
+	assert.Nil(t, err)
+	defer f.Close()
+	writer := zip.NewWriter(f)
+	defer writer.Close()
+
+	_, err = writer.Create("/")
+	assert.Nil(t, err)
+	_, err = writer.Create("/hello/burger")
+	assert.Nil(t, err)
+	_, err = writer.Create("/hello/../../burger")
+	assert.Nil(t, err)
+	_, err = writer.Create("sample/burger")
+	assert.Nil(t, err)
+	_, err = writer.Create(".git/test")
+	assert.Nil(t, err)
+}
+
 func TestJVMCloneCommand(t *testing.T) {
 	dir, err := os.MkdirTemp("", "")
 	assert.Nil(t, err)