search

guzba / zippy

Pure Nim implementation of deflate, zlib, gzip and zip.
MIT License
246 stars 29 forks source link

Fixes #29; filenames containing `..` being rejected. #30

Closed dom96 closed 2 years ago

dom96 commented 2 years ago

This is a 1am PR so review carefully :)

guzba commented 2 years ago

Thanks very much for the PR. I have one thought that I want to check with you.

As you have deduced, my primary goal with avoiding relative paths is to prevent extracting a zip or tarball from writing files outside of the directory you told it to put the files. No one probably wants extracting a zip to put files in random places on their machine.

The PR suggests changing the check to /../. My concern would be if the path is ../a/path/ where it starts with a ... This would/could write out of the directory but pass the test. Could I be wrong about that or does that make sense? It has been a bit for me since I was working on this haha.

If that is possible, would checking for ../ be a good middle ground? More specific than just .. but still catch the initial relative which /../ would miss? No reason not to take just one moment and get your thought on that.

dom96 commented 2 years ago

Thanks for the quick review! Definitely worth thinking about this, one thing that immediately comes to mind is what if there is a folder named test-..? I just tested and was able to create this path:

$ pwd
/mnt/c/Users/morfe/other_projects/zippy/test-../foo

Maybe we could check as well whether the path starts with ../?

guzba commented 2 years ago

Maybe we could check as well whether the path starts with ../?

Sure, that sounds good to me. Are you ok updating this PR to that check?

guzba commented 2 years ago

Merged this and adjusted to https://github.com/guzba/zippy/commit/246385f67a71979ea10da1a068ac6f7d94d73f7b

dom96 commented 2 years ago

Hey, sorry I didn't get a chance to adjust this. FWIW I think you want to also check for the string containing /../ and \..\ as you might get something like foobar/../../../../

guzba commented 2 years ago

Npnp, just was already working on zippy so I just grabbed it quick. Added the suggested check and tagged zippy 0.7.2.