Skip to content

archive/zip: produces incorrect zip archives #30627

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.

Sign up for GitHub

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

Jump to bottom
Closed
enchobelezirev opened this issue Mar 6, 2019 · 6 comments
Closed

archive/zip: produces incorrect zip archives #30627

enchobelezirev opened this issue Mar 6, 2019 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.13

Comments

@enchobelezirev
Copy link

enchobelezirev commented Mar 6, 2019 •
edited by ALTree
Loading

What version of Go are you using (go version)?

$ go version
go version go1.11.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
windows

What did you do?

I am using the archive/zip library in order to produce a zip archive from a given directory. In that directory I have nested directory structures. The problem comes when I build the archive, I am walking each element in the given directory and am I writing it to the zip archive. If the currently walking path element is a dir, I just add a os.PathSeparator to the header's Name.
When the build is finished and I start to read the zip, I see that the files which are read contain "\" - backslashes - default PathSeparator for Windows. As the ZIP Spec defines, each path separator in the ZIP should be a '/' - leading slash.

I saw that description in the FileHeader struct:

// It must be a relative path, not start with a drive letter (such as "C:"),
// and must use forward slashes instead of back slashes. A trailing slash
// indicates that this file is a directory and should have no data.
//
// When reading zip files, the Name field is populated from
// the zip file directly and is not validated for correctness.
// It is the caller's responsibility to sanitize it as
// appropriate, including canonicalizing slash directions,
// validating that paths are relative, and preventing path
// traversal through filenames ("../../../").

In my opinion, the library should take care of \\ - back slashes not the developer of the code :)
Tell me what do you think about this.
Do you accept PRs and fixes about such small changes?

What did you expect to see?

A zip archive containing only '/' as path separators

What did you see instead?

On windows I see the following path separators '\'
@ianlancetaylor
Copy link
Member

If you are running on Windows, then os.PathSeparator is going to be a backslash. If you don't want a backslash, then don't use os.PathSeparator.

It sounds like you are suggesting that the archive/zip package should automatically change backslashes to slashes. I don't see why that would be a good idea. The default behavior should be that the package trusts the user.

@ALTree ALTree changed the title archive/zip produces incorrect zip archives archive/zip: produces incorrect zip archives Mar 6, 2019
@ALTree ALTree added this to the Go1.13 milestone Mar 6, 2019
@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 6, 2019
@enchobelezirev
Copy link
Author

And by trusting the user there will be some errors like in my case. And luckily that the user does not have any bad intentions :)
I think that the library should handle the situation because otherwise this is not correct accordingly to the ZIP Spec.

@enchobelezirev
Copy link
Author

enchobelezirev commented Mar 7, 2019 •
edited
Loading

Here is an extraction from Zip specification:
4.4.17.1 The name of the file, with optional relative path. The path stored MUST NOT contain a drive or device letter, or a leading slash. All slashes MUST be forward slashes '/' as opposed to backwards slashes '\' for compatibility with Amiga and UNIX file systems etc.

@ianlancetaylor
Copy link
Member

If that is what the zip spec says, I would be open to having zip return an error if the filename has backslashes. I do not think that archive/zip should convert backslashes to forward slashes.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 8, 2019
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 8, 2019
@ianlancetaylor
Copy link
Member

CC @dsnet

@enchobelezirev
Copy link
Author

Two things.
First, I think that the spec is written very abstractly and that is why I interpreted the situation in the following way. In my opinion, this should be this way as this does not define who is responsible for handling such situations.
Second, checking with other programming languages, I saw that there is no handling of these slashes as well there.
So, I am closing the ticket, I will do it on my side.
Thanks for replying to this
:)

@golang golang locked and limited conversation to collaborators Mar 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
4 participants
@ianlancetaylor @ALTree @gopherbot @enchobelezirev