Skip to content
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.

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

Zip import directory traversal mitigation #11716

Merged
merged 3 commits into from Apr 16, 2019

Conversation

sgonzalez-r7
Copy link
Contributor

@sgonzalez-r7 sgonzalez-r7 commented Apr 12, 2019

This adds mitigation for directory traversal attacks when importing a ZIP file.

Verification

Note - There are db_disconnect steps in the verification because of a bug when importing while connected to the Metasploit data service. I've talked with @mkienow-r7 about it, and I have a TODO to create an issue.

Before mitigation

  • Checkout master branch
  • Start msfconsole
  • Run db_disconnect if you're connected to the data service
  • Run db_import <path/to/test.zip>
  • Verify /tmp/exploit is created
  • Remove /tmp/exploit
  • Exit msfconsole

After mitigation

  • Checkout this PR branch
  • Start msfconsole
  • Run db_disconnect if you're connected to the data service
  • Run db_import <path/to/test.zip>
  • Verify /tmp/exploit is not created

@@ -236,4 +242,8 @@ def import_msf_zip(args={}, &block)
import_msf_collateral(new_args)
end
end

def is_child_of?(target_dir, target)
target.match?(/^#{target_dir}/)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

target.downcase.start_with?(target_dir.downcase) might be a better option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this check bypassed if if the target is target_dir/../file?
You might need to call 'cleanpath' from the Pathname object to normalize the specified path:

2.6.1 :006 > Pathname.new("cow/../dog").to_s
 => "cow/../dog" 
2.6.1 :007 > Pathname.new("cow/../dog").cleanpath.to_s
 => "dog" 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was working under the assumption that the target_dir is an absolute path, because it's generated on Line 169 . In the case of absolute path, the current form of
(Pathname.new("/cow/../dog") + '../../../../../goat').to_s
has a cleanpath behavior.

dropping into irb from msfconsole:
>> target_dir = Dir::tmpdir
=> "/var/folders/tc/597c6qh15hvf2cvrgmdgcsw19hn4xh/T"
>> (Pathname.new('/cow/dog/cat') + '../../../../../goat').to_s
=> "/goat"

I can change it to

>> File.expand_path('cow/dog/cat' + '../../../../../goat', '/').to_s
=> "/goat"
>> File.expand_path('/cow/dog/cat' + '../../../../../goat', '/').to_s
=> "/goat"

to cover the case of target_dir being specified as a relative path. And it also makes the code more explicit and better shows the intent, which I think is helpful for others reading the code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, the behavior doesn't happen in plain IRB outside of Metasploit. Does Metasploit actually somehow monkeypatch/change the stock behavior for Pathname? Or is the fact that I'm testing this on Ruby 2.4.2 indicate that this is a Ruby-version dependent behavior, and we should explicitly call cleanpath anyway to make it consistent?

2.4.2 :004 > (Pathname.new("/cow/../dog") + '../../../../../goat').to_s
 => "/cow/../../../../../goat" 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also observed that Dir::tmpdir doesn't work in regular IRB. I suspect it is monkey patched somewhere in Metasploit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're using tmpdir from the stdlib:

>> Dir.method(:tmpdir).source_location
=> ["/Users/sgonzalez/.rvm/rubies/ruby-2.6.2/lib/ruby/2.6.0/tmpdir.rb", 21]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from regular IRB:

ruby-2.6.2@metasploit-framework :001 > require 'tmpdir'
true
ruby-2.6.2@metasploit-framework :002 > Dir::tmpdir
"/var/folders/tc/597c6qh15hvf2cvrgmdgcsw19hn4xh/T"

@busterb
Copy link
Contributor

busterb commented Apr 12, 2019

Unrelated to this, but maybe related to db_import issues, I also see that there is a version check in import_msf_collateral that does not have a MetasploitV5 case, in case that is necessary.

@sgonzalez-r7
Copy link
Contributor Author

Unrelated to this, but maybe related to db_import issues, I also see that there is a version check in import_msf_collateral that does not have a MetasploitV5 case, in case that is necessary.

The XML I'm using is <MetasploitV4>

and better readable is_child_of? method
@sgonzalez-r7
Copy link
Contributor Author

Hi @lucaddepar. This PR addresses the directory traversal vulnerability you observed in Metasploit and disclosed to Rapid7. We appreciate your willingness to share your findings. We thought you'd might like to review the proposed mitigation.

@sgonzalez-r7
Copy link
Contributor Author

@busterb, @bcoles , Any more feedback? Do you think this is ready to land?

@busterb
Copy link
Contributor

busterb commented Apr 16, 2019

Go for it.

@busterb busterb added the bug label Apr 16, 2019
busterb added a commit to busterb/metasploit-framework that referenced this pull request Apr 16, 2019
@busterb busterb merged commit cf7096f into rapid7:master Apr 16, 2019
@tdoan-r7
Copy link
Contributor

tdoan-r7 commented Apr 17, 2019

Release Notes

Mitigation for directory traversal attacks when importing a ZIP file has been added, referenced as CVE-2019-5624.

@tdoan-r7 tdoan-r7 added the rn-fix release notes fix label Apr 17, 2019
@ikkisoft
Copy link

I just got notified via Twitter. Unfortunately this is my personal (and current) account so I didn’t see that you were working on a patch. Thanks for the fix!

As mentioned in the advisory, I would appreciate if you could credit the @doyensec team (https://doyensec.com/) in your blog post / release notes since all credits actually go to my team - I was just helping with the coordination.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug rn-fix release notes fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants