archive/zip: EOCDR comment length handling is inconsistent with other ZIP implementations

[ouuan]

This has been reported in email and accepted as a PUBLIC track security issue.

Go version

go version go1.22.2 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.2'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build216392132=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Use archive/zip (actually, a thin wrapper library https://github.com/evilsocket/islazy/blob/master/zip/unzip.go) to extract the attached ZIP archive: poc.zip (Warning: It contains malware. Do not open the extracted exe file!)

It is constructed as illustrated below:

normal ZIP archive with a virus

    LFH           ─╮
    data (virus)   ├─→──┐
    CDH            │    │
    EOCDR         ─╯    │ copy the inner
                        ↓ archive with virus
the final ZIP archive   │ as the data of
                        │ the outer archive
    LFH (size set to 1) │
    data (copied) ←─────┘
    CDH (size set to 1)
    EOCDR (comment length set to 1 with empty comment content)

What did you see happen?

It can get the malware inside the ZIP archive. This is caused by the following logic, which skips EOCDR with a bogus comment length field and continues to search for the next one:

go/src/archive/zip/reader.go

Lines 702 to 704 in c51f6c6

	if n+directoryEndLen+i <= len(b) {
	return i
	}

However, this is inconsistent with most other ZIP implementations, so they are using different EOCDRs and get different files extracted. Most other ZIP implementations are not able to get the virus. The PoC file is flagged by only 1/62 security vendor on VirusTotal.

This inconsistency can also be used in other scenarios depending on the specific use case of the package, such as hiding add-on files from linter and reviewers.

What did you expect to see?

archive/zip should return an error when a bogus EOCDR is encountered.

[cherrymui]

cc @dsnet @bradfitz

[gopherbot]

Change https://go.dev/cl/585397 mentions this issue: archive/zip: treat truncated EOCDR comment as an error

[neild]

This parser misalignment is a PUBLIC track security issue. We have assigned this CVE-2024-24789.

@gopherbot please open backport issues. This is a security issue.

[gopherbot]

Backport issue(s) opened: #67553 (for 1.21), #67554 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[gopherbot]

Change https://go.dev/cl/588795 mentions this issue: [release-branch.go1.21] archive/zip: treat truncated EOCDR comment as an error

[gopherbot]

Change https://go.dev/cl/588796 mentions this issue: [release-branch.go1.22] archive/zip: treat truncated EOCDR comment as an error