Directory traversal vulnerability

[wonda-tea-coffee]

Summary

In version 2.3.2, we are again experiencing the exact same problem as below.
#315

PoC

root@ea8d5cb86e9f:/work# ruby -v
ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [aarch64-linux]
root@ea8d5cb86e9f:/work# uname -a
Linux ea8d5cb86e9f 5.15.64-0-virt #1-Alpine SMP Mon, 05 Sep 2022 08:02:49 +0000 aarch64 GNU/Linux
root@ea8d5cb86e9f:/work# gem list | grep rubyzip
rubyzip (2.3.2)
root@ea8d5cb86e9f:/work# zipinfo traversal.zip 
Archive:  traversal.zip
Zip file size: 166 bytes, number of entries: 1
-rw-r--r--  5.2 unx        0 t- defN 22-Nov-15 07:57 ../../.././../../../../tmp/hacked
1 file, 0 bytes uncompressed, 2 bytes compressed:  0.0%
root@ea8d5cb86e9f:/work# ls -l /tmp
total 0
root@ea8d5cb86e9f:/work# ruby extract.rb 
Extracting ../../.././../../../../tmp/hacked
root@ea8d5cb86e9f:/work# ls -l /tmp
total 0
-rw-r--r-- 1 root root 0 Nov 16 00:55 hacked

extract.rb

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(entry.name)
  end
end

[wonda-tea-coffee]

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

[wonda-tea-coffee]

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

[hainesr]

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).

[wonda-tea-coffee]

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

[hainesr]

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

[hainesr]

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