Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

archive/zip: don't read directories containing file data #54801

Closed
neild opened this issue Aug 31, 2022 · 12 comments
Closed

archive/zip: don't read directories containing file data #54801

neild opened this issue Aug 31, 2022 · 12 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1 release-blocker
Milestone

Comments

@neild
Copy link
Contributor

neild commented Aug 31, 2022

The archive/zip package forbids writing data to directory files: Writing to the io.Writer returned by w.Create("dir/") fails with zip: write to directory.

However, archive/zip permits reading data from directory files. This means there are zip archives that can be read by archive/zip, but not rewritten. In addition, the zip specification doesn't permit directories to contain file data, so these archives are invalid:

Zero-byte files, directories, and other file types that contain no content MUST NOT include file data.

We should return an error when parsing a zip file that contains a directory that contains data.

Thanks to Adam Korczynski (ADA Logics) and OSS-Fuzz for the report.

@heschi heschi added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 1, 2022
@heschi heschi added this to the Backlog milestone Sep 1, 2022
@heschi
Copy link
Contributor

heschi commented Sep 1, 2022

cc @dsnet @bradfitz

@amarjeetanandsingh
Copy link
Contributor

May I take this up?

@dsnet
Copy link
Member

dsnet commented Sep 2, 2022

Sure, go for it!

@amarjeetanandsingh
Copy link
Contributor

@neild Is it possible to for you to share the zip file to reproduce the issue?

@amarjeetanandsingh
Copy link
Contributor

amarjeetanandsingh commented Sep 14, 2022

@dsnet @neild

  • I created file.txt with content = "abcd"
  • zipped this file(say it file.zip)
  • opened file.zip in a hex editor and changed the internal file name from file.txt to fileetx/ (in central repo also).

This altered file.zip can be considered as the test file for this issue?

@gopherbot
Copy link

Change https://go.dev/cl/449955 mentions this issue: archive/zip: don't read directories containing file data

@gopherbot
Copy link

Change https://go.dev/cl/450280 mentions this issue: go1.20: add release notes for archive/zip, encoding/binary, mime

gopherbot pushed a commit that referenced this issue Nov 15, 2022
For #48866
For #54139
For #54801

Change-Id: Iafe72ccc7e756ec1edb5bb7e8e90d385458cff29
Reviewed-on: https://go-review.googlesource.com/c/go/+/450280
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@heschi
Copy link
Contributor

heschi commented Nov 30, 2022

It seems that the Java jar tool creates directories with file data:

~/tmp $ ls
~/tmp $ mkdir A
~/tmp $ touch A/foo.txt
~/tmp $ jar cf foo.jar .
~/tmp $ unzip -lv foo.jar
Archive:  foo.jar
 Length   Method    Size  Cmpr    Date    Time   CRC-32   Name
--------  ------  ------- ---- ---------- ----- --------  ----
       0  Defl:N        2   0% 2022-11-22 15:21 00000000  META-INF/
      60  Defl:N       59   2% 2022-11-22 15:21 af937e93  META-INF/MANIFEST.MF
       0  Stored        0   0% 2022-11-22 15:21 00000000  A/
       0  Stored        0   0% 2022-11-22 15:21 00000000  A/foo.txt
--------          -------  ---                            -------
      60               61  -2%                            4 files

(Note the size 2 on META-INF.)

I don't know how common it is to process JAR files with Go, but everyone who does will need to make a code change. Is that okay? Seems worth updating the release notes, and maybe we should add a GODEBUG to opt out?

Reopening to track as a release blocker, but okay after RC1.

@heschi heschi reopened this Nov 30, 2022
@heschi heschi added release-blocker okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1 labels Nov 30, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Dec 1, 2022
@heschi
Copy link
Contributor

heschi commented Dec 7, 2022

http://go.dev/cl/454475 and http://go.dev/cl/455523 should've been attached to this issue, I believe. @rolandshoemaker please confirm it's fixed?

@rolandshoemaker
Copy link
Member

Ah yes, sorry, didn't realize this got reopened. This should be fixed (slightly more lenient) now.

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Dec 7, 2022

(We've also sent an upstream patch to OpenJDK which fixes the jar behavior.)

@bjkail
Copy link

bjkail commented Dec 28, 2022

For reference, openjdk/jdk#11441.

@golang golang locked and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1 release-blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants