Directory traversal vulnerability about rubyzip HOT 6 CLOSED

Hi @wonda-tea-coffee and thanks for raising this again.

The above code clearly short-circuits around the protections that Rubyzip has for traversal. For example the following code would be safe, simply by changing entry.extract(entry.name) to entry.extract:

require 'zip'

Zip::File.open('traversal.zip') do |zip_file|
  # Handle entries one by one
  zip_file.each do |entry|
    # Extract to file/directory/symlink
    puts "Extracting #{entry.name}"
    entry.extract
  end
end

A developer would need to specifically write the unsafe version that you supply above, but I agree that it's probably easy to do so without knowing you've done it.

I do think it should be possible for a developer to knowingly and deliberately allow extracting to potentially unsafe names, if for their particular situation it is perfectly safe, but:

• it should not be the default;
• the default should be to not allow this, and the developer should have to specifically allow it themselves; and
• we should never allow extracting to an unsafe place directly to the name provided in zip archive itself

Do you think that would be acceptable?

I will think on how to implement that and get it into version 3 - which is getting closer (honest).

from rubyzip.

OK, having now tried a somewhat naive implementation based solely on that bulleted list above, I see a rather large UX hole. The above prevents one from extracting to, say, /tmp unless you were to completely turn off checks for unsafe paths. Anything starting with a / is currently considered unsafe - which it would be if the leading / was in the entry name in the zip file, but it should be OK to extract to /tmp.

I think the solution then is to add a parameter to the extract method which specifies a target directory for the extraction. This would be combined with the entry name and then we'd check that the result was safe. For example:

• /tmp joined with etc/passwd is safe
• /tmp joined with ../etc/passwd is not safe

This seems to be how other libraries manage this situation: https://github.com/commonsguy/cwac-security/blob/master/security/src/main/java/com/commonsware/cwac/security/ZipUtils.java#L171-L181

from rubyzip.

@hainesr
Thanks for the reply.
I completely agree with the above 👍

from rubyzip.

Hi @wonda-tea-coffee, I have finally raised a PR to implement the above (#554).

from rubyzip.

Sorry.
I missed reading the following.
https://github.com/rubyzip/rubyzip/blob/v2.3.2/lib/zip/entry.rb#L174

from rubyzip.

Is there any particular plan to correct this issue?
If not, we will close this Issue and propose a fix in the Security Advisory.
github/advisory-database#832

from rubyzip.