-
Notifications
You must be signed in to change notification settings - Fork 14k
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
Fix a crash with the admin/dcerpc/icpr_cert
module and OpenSSL 3.4.0
#19624
Conversation
@@ -256,7 +256,6 @@ def do_request_cert(icpr, opts) | |||
# @return [OpenSSL::X509::Request] The request object. | |||
def build_csr(cn:, private_key:, dns: nil, msext_sid: nil, msext_upn: nil, algorithm: 'SHA256', application_policies: []) | |||
request = OpenSSL::X509::Request.new | |||
request.version = 1 |
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.
Just to confirm; is this version
setting entirely wrong for newer versions of OpenSSL - or is the issue something else?
Just asking as it looks like we've got some other code similar to this
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.
According to the discussion in the OpenSSL issue, setting the version on CSR's has always been wrong. RFC 2986 only defines a single version for CSRs: v1(0). The CSR could have an invalid version and they decided to totally remove the ability to set the version on CSR in OpenSSL 3.4.0. So, yes, this should be removed or it will throw an exception.
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.
Awesome thanks; Some other places in framework:
$ rg -C 3 'OpenSSL::X509::Request'
modules/auxiliary/server/relay/esc8.rb
128-
129- def create_csr(private_key, cert_template)
130- vprint_status('Generating CSR...')
131: request = OpenSSL::X509::Request.new
132- request.version = 1 <---------------------------
133- request.subject = OpenSSL::X509::Name.new([
134- ['CN', cert_template, OpenSSL::ASN1::UTF8STRING]
modules/exploits/windows/backupexec/ssl_uaf.rb
586- def handle_a_csr(s, ca_cert, ca_key)
587- resp = do_simple_ssl_request(s, SSLRequest::Opcode.get_csr_req, 0)
588- return nil if resp.nil?
589: request = OpenSSL::X509::Request.new(resp.unknown2)
590-
591- agent_cert = OpenSSL::X509::Certificate.new
592- agent_cert.version = 3 <---------------------------
modules/exploits/multi/veritas/beagent_sha_auth_rce.rb
364- end
365-
366- def sign_agent_csr(csr)
367: o_csr = OpenSSL::X509::Request.new(csr)
368- @be_agent_cert = OpenSSL::X509::Certificate.new
369- @be_agent_cert.version = 2 <---------------------------
370- @be_agent_cert.serial = rand(2**32..2**64 - 1)
There might be more 🕵️
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 code was copied into the new ESC8 module which will also need to be updated.
metasploit-framework/modules/auxiliary/server/relay/esc8.rb
Lines 129 to 140 in 03dc231
def create_csr(private_key, cert_template) | |
vprint_status('Generating CSR...') | |
request = OpenSSL::X509::Request.new | |
request.version = 1 | |
request.subject = OpenSSL::X509::Name.new([ | |
['CN', cert_template, OpenSSL::ASN1::UTF8STRING] | |
]) | |
request.public_key = private_key.public_key | |
request.sign(private_key, OpenSSL::Digest.new('SHA256')) | |
vprint_status('CSR Generated') | |
request | |
end |
I think at this point, it would make sense to make a create_csr
method probably in Rex::Proto::CryptAsn1::X509
. Placing it in Rex would put us in a position to test it more easily in the future too.
Also update `ms_icpr.rb` to use it
It also fixes some unrelated minor issues found in the module and the documentation
567fbd9
to
24e19e4
Compare
Thank you @smcintyre-r7 and @adfoster-r7 for the review. I added a The Also, I fixed some unrelated minor issues in the |
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 the test failure needs some kind of change to address. I'm not sure why it's popping up now, it probably has something to do with the require
order but it should probably be addressed.
Once that's done, I'm happy to land this. I tested both the esc8 relay module as well as the icpr_cert module. In both cases, I was able to successfully issue a certificate and then use the certificate to issue a TGT, thus proving it's validity.
Thank you @smcintyre-r7 for your review. I fixed the |
@@ -5,7 +5,8 @@ | |||
# | |||
# -*- coding: binary -*- | |||
|
|||
require 'windows_error/h_result' | |||
require 'windows_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.
I'm not sure why it breaks now since it is unrelated to the changes in this PR. Probably some lib autoloading that used to be in place and that are not anymore.
It looks like we still have a related failure:
|
It looks like I missed this one. It should be good now. |
Release NotesThis fixes a bug that would occur when generating CSRs for AD CS with OpenSSL 3.4.0. The bug was related to an attribute in the request that can no longer be explicitly set. |
This fixes an issue wit the
ms_icpr
library that crashes with OpenSSL 3.4.0. tis version introduced a change in the Certificate Signing Request library that reject setting versions on CSR's. See the related discussion in this issue and the related PR.Verification
msfconsole
use admin/dcerpc/icpr_cert
run verbose=true CA=<target CA name> RHOSTS=<remote IP> username=<username> password=<password> CERT_TEMPLATE=User
Scenarios
Without the fix
With the fix
TODO