diff --git a/cmd/gitserver/server/vcs_syncer_jvm_packages.go b/cmd/gitserver/server/vcs_syncer_jvm_packages.go index e460213624d6..9189adb6ae23 100644 --- a/cmd/gitserver/server/vcs_syncer_jvm_packages.go +++ b/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 } diff --git a/cmd/gitserver/server/vcs_syncer_jvm_packages_test.go b/cmd/gitserver/server/vcs_syncer_jvm_packages_test.go index 2f00513d4e12..145564f1868a 100644 --- a/cmd/gitserver/server/vcs_syncer_jvm_packages_test.go +++ b/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)