-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
guard Rex::Version.new against crashes on local modules #19813
base: master
Are you sure you want to change the base?
Conversation
All of the existing This code splits at the first Edit: modules/exploits/linux/local/vmwgfx_fd_priv_esc.rb does no prior parsing of The package parsing code does no prior parsing of the version before passing to |
The This is just want I was seeing in an engagement and wanted to fix some crashes that |
I'll be testing these changes on target to make sure its handling better, and get some more test data. |
return CheckCode::Vulnerable("IF host OS is Ubuntu, kernel version #{release} is vulnerable") | ||
begin | ||
release_short = Rex::Version.new(release.split('-').first) | ||
release_long = Rex::Version.new(release.split('-')[0..1].join('-')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we re-join with '-' (which works on ubuntu where I tested it), this will fail on amazon linux.
Fedora 31 target (vm) I'm getting the following:
This is caused by https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/post/linux/kernel.rb#L262 . Which looks like its checking on the LOCAL (metasploit) system, not the remote (exploited) system. Just wanted to confirm if that was right and if that was the expected behavior. |
That is definitely checking the local file which is definitely wrong. The |
@jvoisin any background on that change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like something that would be good functionality to move into post/linux/kernel
as a function. I see we already have kernel_version
but something with an intuitive name that would return an instance of Rex::Version
so this kind of normalization is centralized would be beneficial in the future. It would be a good idea to do the exception handling within the method as well and just return nil
to the caller so they can do what they want in the event that they can't get a Rex::Version
instance.
Maybe something like kernel_rex_version
?
I'm particularly concerned about this because of the one case where the string is rejoined in the case of Ubuntu.
Dang, I thought that |
I like the idea, but I think that needs to be handled outside of this PR. I think a bunch of different kernel releases from different flavors will be required in a pretty big spec to make sure we handle all the cases. For instance, Ubuntu has kernels like |
fixes #19812
Fixes some bugs and potential bugs where
Rex::Version.new
is fed data which may cause a crash. Guard these to prevent crashes on Amazon 2 Linux, and other systems (tomcat one should break on non dpkg based systems like fedora)