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: off-by-one writing directory64 records #14186

Closed
rsc opened this issue Feb 2, 2016 · 4 comments
Closed

archive/zip: off-by-one writing directory64 records #14186

rsc opened this issue Feb 2, 2016 · 4 comments
Assignees
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Feb 2, 2016

archive/zip/writer.go says:

if records > uint16max || size > uint32max || offset > uint32max {

but each of these > should probably be >=.

See the similar bug fixed in https://go-review.googlesource.com/#/c/18317/ for writing data files of size uint32max.

@rsc rsc added this to the Go1.7 milestone Feb 2, 2016
@rsc rsc modified the milestones: Go1.8, Go1.7 May 16, 2016
@rsc rsc added the NeedsFix label Sep 29, 2016
@rsc rsc modified the milestones: Go1.8Maybe, Go1.8 Sep 29, 2016
@quentinmit
Copy link
Contributor

@quentinmit quentinmit commented Nov 8, 2016

records should definitely be >= since there is an additional end record. I think the size needs to account for directoryEndLen extra bytes. I don't see why offset needs to be >= however. So I propose this set of rules:

if records > uint16max-1 || size > uint32max-directoryEndLen || offset > uint32max 

ping @rsc

@rsc
Copy link
Contributor Author

@rsc rsc commented Nov 9, 2016

In the zip format encoding, the ^0 value means "there is a bigger value encoded next". If you encode offset 0xFFFFFFFF meaning 0xFFFFFFFF, a decoder will see it as "there's a 64-bit value coming next". If the encoding does not include that 64-bit value, the decoder will get confused. See CL 18317 for an example of this with the file data size field.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 9, 2016

CL https://golang.org/cl/33012 mentions this issue.

@rsc rsc modified the milestones: Go1.8, Go1.8Maybe Nov 11, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 15, 2016

CL https://golang.org/cl/33291 mentions this issue.

@gopherbot gopherbot closed this in c1e9760 Nov 17, 2016
@golang golang locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.