-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
A more robust implementation for Windows version comparisons #17336
Conversation
The updates to TODO:
|
I fixed the linting issues in this PR - #17338 Should be able to rebase this PR against the latest master to get CI happy again (hopefully) |
lib/msf/core/windows_version.rb
Outdated
Win81 = "Windows 8.1" | ||
Server2012R2 = "Windows Server 2012 R2" | ||
|
||
Win10Plus = "Windows 10+" |
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.
Don't we want "Windows 11" as a separate entity?
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.
On one hand this makes the most intuitive sense... on the other, the way Microsoft actually treats their version numbers makes this a less obvious choice in my opinion: Windows 10 is 10.0.x whereas Windows 11 is... also 10.0.x. Combine that with the versioning of Windows Server lately, wherein Server 2016 is effectively in line with Windows 10 v1607 (in terms of a shared build number), Server 2019 with Win10 1809 and Server 2022... well it's no longer in line with a specific workstation version, in terms of its build number... but they're all still "Windows 10.0.x". It seems to me that, if we ignore the marketing of what they call it, we're kind of in an era of incremental releases, rather than the former monolithic quantum leap.
The other consideration I had is around forwards-compatibility, which is what this PR aims to resolve. It's clear that Microsoft are changing their version numbering strategy, where everything is now 10.0.x; but it's not clear to me what it will be in the future, and so I can't say with confidence what a check for "Windows 11 in general" would actually look like? Would it check for "10.0.x where x is greater than 22000"? Could that misidentify something as Windows 12 in the future? I know playing the what-if game is maybe a little fraught, and hey, you can always change code in the future... but in the past, it was easy: a new "Major Release" means a new minor version number at least (Vista == 6.0, Win7 == 6.1, Win8 == 6.2, Win8.1 == 6.3). Now it seems much less clear.
I guess tl;dr I'm keen to see the dust settle a bit more on the version numbers before confidently proclaiming "This is Windows 11".
79d448e
to
5d39414
Compare
Should be good for review now. Note that some of the modules that were modified have had subtle changes to the versions supported - I've tried to comment these in the relevant commit messages as I went, about the justifications for these. Verification
|
@old_os = true | ||
unless sysinfo['OS'].include?('5.2 Build 3790, Service Pack 2') | ||
if version.build_number < Msf::WindowsVersion::Server2003_SP1 |
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.
I tested the persistence_exe
module that uses this mixin, and the /F
flag worked on 2003 SP1 as well, so I adjusted the range to include that too.
unless @rtf_path.nil? | ||
write_file(@rtf_path, @original_data) | ||
end |
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.
Meterpreter crashes if these are nil
, which it is if the exploit didn't even start - decided to fix it while I was here.
if version.build_number < Msf::WindowsVersion::Win8 && !version.windows_server? | ||
print_bad("Operating system: #{version.product_name}") |
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.
Changed to include Windows 2000 and earlier, based on the comment below "Windows 7 and previous...not vulnerable"
5d39414
to
1966cb2
Compare
version = get_version_info | ||
unless version.build_number.between?(Msf::WindowsVersion::Vista_SP0, Msf::WindowsVersion::Win81) | ||
fail_with(Failure::NotVulnerable, "#{version.product_name} is not vulnerable.") |
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 is technically a change in behaviour, since it never used to specify Server 2012, but it does support the equivalent workstation versions; and I actually tested it on Server 2012, and it works fine. I presume just a regex oversight - what this PR is intended to help with.
I added a quick Windows 10x64 releasesMeterpreter
Shell
Powershell
|
Windows 10 x86 ReleasesMeterpreter
Shell
Powershell
|
XP:
|
Windows Servers with Meterpreter
|
Windows Servers: Shell
Windows Servers: Powershell
|
I noticed that this fails to recognize Windows 2000 because the |
I have some ideas on how to fix this, but I don't have time right now. I'm going to mark this as delayed and try and get some time next week to work on it. |
I've just converted the PR to a draft for now until the PR is rebased against master and the smarter fallback logic is implemented, as well as language-pack agnostic version detection 👍 I'll not add the Hopefully Smashery will be free in a few months to pick this up again 🤞 |
Utilised it in various existing modules - this should fix some subtle bugs in specific modules' version detection.
…nt language packs.
Alright, I re-worked this to succeed on non-English locales. The approach has been to make requests of the registry, as these values are not locale-dependent. Most are just numbers, with the exception of the "Service Pack" registry key, which in my testing on various different OSes and language packs, always says "Service Pack". Given the duration since the initial work, there was a bit of merging - mostly rubocop stuff. I squashed, rebased and force-pushed since the initial work, but it's all in the first new commit. I coded up a module like @bwatters-r7 did above, to test just this bit of work, and ran it on various OSes and locales: ShellWindows Server 2022 (Spanish)
Windows 10 22H2 (French)
Windows Server 2012 (English)
Windows Server 2008 R2 SP1 (English)
Windows Server 2008 SP 2(English)
Windows XP SP3 (Italian)
Windows Server 2003
Windows 2000 (German)
PowerShellWindows Server 2022 (Spanish)
Windows Server 2012 (English)
Windows Server 2008 R2 SP1 (English)
MeterpreterWindows Server 2022 (Spanish)
Windows 10 22H2 (French)
Windows Server 2012 (English)
Windows Server 2008 R2 SP1 (English)
Windows Server 2008 (English)
Windows XP SP3 (Italian)
Windows Server 2003
|
Testing on modern OSs with meterpreter Windows 10x64
Windows 10x86
Servers
Windows 8
|
@smashery Just for visibility; Is it possible to correctly map the details of the 2022 boxes, i.e.
From:
To be |
@adfoster-r7 Yes, and no. I can speak from the Meterpreter side, which is what I think @smashery is mimicking. Previously, we would get the friendly name (at least in Meterpreter) by grabbing the versions and then entering a case statement to find the friendly name based off the numeric version. |
Russian
French:
|
I think this is ready to land, but to be super safe I'm going to hold off until tomorrow after we cut the release. That'll give us a couple more opportunities to shake out anything before we tag it. |
@adfoster-r7 - Yeah, we could certainly look up the |
lib/msf/core/post/windows/version.rb
Outdated
result = get_version_info_impl | ||
if result.nil? | ||
print_error("Couldn't retrieve the target's build number!") | ||
raise RuntimeError.new("Couldn't retrieve the target's build number!") |
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.
I'm wondering if this would be useful to use specific exception classes instead of the generic RuntimeError
? Maybe something like Msf::Post::Windows::Version::Error
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.
Good call - have done this.
lib/msf/core/post/windows/version.rb
Outdated
return nil | ||
end | ||
|
||
major, minor, build, unused, revision = groups.captures |
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.
Not a big deal, but the convention in ruby is to use underscore for unused variables:
major, minor, build, unused, revision = groups.captures | |
major, minor, build, _unused, revision = groups.captures |
or
major, minor, build, unused, revision = groups.captures | |
major, minor, build, _, revision = groups.captures |
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.
I just realized the capture group is there only because it is optional and it is never used. You can use (?:…)
syntax to group without capturing:
groups = build_num_raw.match(/.*Version\s+(\d+)\.(\d+)\.(\d+)(?:\.(\d+))?/)
major, minor, build, revision = groups.captures
lib/msf/core/post/windows/version.rb
Outdated
end | ||
|
||
major, minor, build, unused, revision = groups.captures | ||
revision = 0 if revision.nil? |
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.
It looks like revision
is not used anywhere. I might be missing something though.
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.
Ah yes, there are a few places throughout the rest of the codebase where Revision is actually used. I intend to integrate it into the WindowsVersion
code, and then use it in those specific locations, but this PR is getting big enough that I will circle back around and just do that. For now I'll remove it.
lib/msf/core/post/windows/version.rb
Outdated
when /WinNT/ | ||
product_type = Msf::WindowsVersion::VER_NT_WORKSTATION | ||
when /LanmanNT/ | ||
product_type = Msf::WindowsVersion::VER_NT_DOMAIN_CONTROLLER | ||
when /ServerNT/ |
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.
Do we need regex here? I'm under the impression this is a simple string comparison, which is usually preferred when not needed:
when /WinNT/ | |
product_type = Msf::WindowsVersion::VER_NT_WORKSTATION | |
when /LanmanNT/ | |
product_type = Msf::WindowsVersion::VER_NT_DOMAIN_CONTROLLER | |
when /ServerNT/ | |
when 'WinNT' | |
product_type = Msf::WindowsVersion::VER_NT_WORKSTATION | |
when 'LanmanNT' | |
product_type = Msf::WindowsVersion::VER_NT_DOMAIN_CONTROLLER | |
when 'ServerNT' |
also, maybe using constants would make sense instead of raw strings here.
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.
Good call.
lib/msf/core/post/windows/version.rb
Outdated
else | ||
# Pre-Windows 10 | ||
service_pack_raw = shell_registry_getvaldata('HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion', 'CSDVersion', Msf::Post::Windows::Registry::REGISTRY_VIEW_NATIVE) | ||
if service_pack_raw.nil? and major >= 6 |
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.
if service_pack_raw.nil? and major >= 6 | |
if service_pack_raw.nil? && major >= 6 |
lib/msf/core/post/windows/version.rb
Outdated
private | ||
|
||
def empty_os_version_info_ex | ||
result = [0, |
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.
The result
local variable doesn't seem to be used.
result = [0, | |
[ | |
0, |
There is also an indentation issue, it should be 2 spaces:
[
0,
0,
0,
...
lib/msf/core/post/windows/version.rb
Outdated
0, | ||
0, | ||
0, | ||
"", |
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.
"", | |
'', |
def get_version_info_impl | ||
if session.type == 'meterpreter' | ||
result = session.railgun.ntdll.RtlGetVersion(input_os_version_info_ex) | ||
os_version_info_ex = unpack_version_info(result['VersionInformation']) |
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.
Is there a possibility that Railgun raise an exception? or maybe result
could be nil
? I believe this should be handled to avoid breaking.
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.
It's certainly conceivable that railgun fails. Looking through the code, this could perhaps be either a Rex::TimeoutError
(for a dead session), or a RequestError
(e.g. extension failed to load); maybe some other ones - I'd have to look further.
Given it won't be a graceful handling (i.e. we'd need to throw an exception anyway), do you think it's much clearer handling and re-throwing?
I'm also a bit hesitant just because, looking at other usages of railgun throughout the codebase, I don't see that pattern anywhere else; most other places I looked let the exception bubble up, and handle it at the module level anyway.
begin | ||
return session.railgun.shell32.IsUserAnAdmin()['return'] | ||
rescue StandardError | ||
true |
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 might be out of scope since it was the previous behavior, but I'm wondering if there is a way to distinguish between an error in case the API is not exposed and a Railgun error due to something else (communication failure, process died, etc.). This might not be possible, but I'm worried about always assuming the session is an admin session if something goes wrong.
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.
Yeah, I'm a bit concerned about how much this PR already contains - happy to look at this afterwards.
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.
Although thinking about this a bit more, the one case that this is described as being applicable (Windows 2000) doesn't support Meterpreter anymore anyway. It feels like the exception handling here is a bit misguided - it's inferring from the presence of an exception that it's on Windows 2000, and returning true, perhaps because Windows 2000 just treated everyone as an admin? I'd be inclined just to remove that exception handling, given the documented reason for it existing no longer applies.
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.
It feels like the exception handling here is a bit misguided - it's inferring from the presence of an exception that it's on Windows 2000
The intention of the rescue
here is to presume the user is an admin if shell32.IsUserAnAdmin()
fails. Windows 2000 is not inferred.
This code is in the is_admin?
method which is used as a privilege check to bail out prior to attempting operations on the host which are doomed to failure. Historically, in instances where the check failed, it was expected to fail safe so as to not prevent further execution, hence the greedy rescue.
One potential reason that IsUserAnAdmin
may fail is due to using an old operating system such as Windows 2000.
Granted, the code is sloppy. If we wanted to always fail safe, we should rescue from method.
Thanks for looking at this @cdelafuente-r7 . Most of these came out in a Rubocop; a few other responses above. |
Release NotesThis PR adds new code to simplify and standardize windows version checking and comparisons. |
This change resolves #16389, by implementing a new class that has logic and constants to sort out
To make sure that the API made sense, I replaced a bunch of usages of the previous pattern (e.g.
if sysinfo['OS'] =~ /Windows [Vista|2008|7]
) with this new approach.In doing so, I discovered a bunch of modules that had incorrect checks - both compared against MSRC's advisories, and tested directly in a range of OSes. Largely these were modules that were configured to run on Workstation builds, but the implementation overlooked the Server versions. For example:
windows/local/bypass_injection_winsxs was configured to only run against Workstation builds, but I tested against 2016 and 2019 and it works fine. On 2012, the required binary (dccw.exe) wasn't present. Didn't test on 2012 R2. Worked all the way up until latest Win10 (22H2), although not against Server 2022.
appx_svc_hard_link_privesc
crashed when it doesn't succeed, and complained about incorrect session types/platform, because those settings hadn't been configured. So I fixed that, though it uncovered a separate bug in meterp, which I'll log.bypassuac_sluihijack
was configured to only work against workstation builds, and every Win10. But I tested it against 2016 and 2019, and it worked fine. UACME gives a fix version for this technique, so I added that in.bypassuac_dotnet_profiler
seemed to overlook Server >2012. But I tested on 2016, 2019 and 2022 and it works fine.I'm leaving this as a draft initially, as I'd appreciate feedback about whether the API is sufficiently intuitive.
I also need to go back through a bunch of the modules I changed, and do the needful with respect to rubocop.