Skip to content

Conversation

KonstantinKuklin
Copy link

Wrong relative path on unpacking zip archives with :/ character sequences

@petk petk added the Bug label May 14, 2019
@cmb69
Copy link
Member

cmb69 commented May 14, 2019

Hmm, simply removing the check for ':' is likely to break things on Windows (consider 'C:/foo').

@KonstantinKuklin
Copy link
Author

@cmb69 thank you for notification

@KonstantinKuklin KonstantinKuklin force-pushed the bug_77978 branch 8 times, most recently from 86c3850 to c073d66 Compare May 20, 2019 02:00
@KonstantinKuklin
Copy link
Author

ready for review @cmb69

@cmb69
Copy link
Member

cmb69 commented Jun 6, 2019

@KonstantinKuklin, please remove the changes to NEWS. This should be added by the person who merges the PR. Otherwise there will be merge conflicts al the time. :)

@remicollet , what do you think about the patch?

Wrong relative path on unpacking zip archives with ".","..",":" character sequences
on different platforms
@krakjoe
Copy link
Member

krakjoe commented Jul 3, 2019

@remicollet bump :)

@krakjoe
Copy link
Member

krakjoe commented Oct 2, 2019

@cmb69 are you happy with this ?

@cmb69
Copy link
Member

cmb69 commented Oct 2, 2019

ZipArchive::extractTo() is not supposed to extract any files outside the given $destination. The existing code ensures that by assuming that any ./ and :/ can be special (relative paths; drive spec on Windows), and simply chopping the leading chars off. The patch would make this check much stricter, so there might be some chance/trick to bypass the check.

A particular issue appears the special treatment for Windows. This could result in different extraction paths depending on the OS, what might generally be undesirable, and could cause BC issues.

So I'm uneasy to apply this to stable branches.

@krakjoe
Copy link
Member

krakjoe commented Oct 2, 2019

That sounds like rejection of the idea behind the pull request to me, so I'm happy to close this here.

Thanks for input everyone ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants