hrr_rb_ssh icon indicating copy to clipboard operation
hrr_rb_ssh copied to clipboard

Support openssl3

Open adfoster-r7 opened this issue 2 years ago • 21 comments

As noted by https://github.com/hirura/hrr_rb_ssh/issues/32 - it looks like OpenSSL 3.0 has backwards incompatible changes

This is an initial test PR to document some of the changes that have been required to get tests passing for OpenSSL v3, but there's still more work to go

adfoster-r7 avatar Jul 19 '22 02:07 adfoster-r7

@adfoster-r7 Wow, Great work again ... I didn't follow the recent Ruby/OpenSSL updates, so need clarification. The change loose Ruby 2.6 or earlier support, right ? I'd like to clarify this because, originally, I tried to keep this available for older Ruby versions.

hirura avatar Jul 19 '22 02:07 hirura

I didn't follow the recent Ruby/OpenSSL updates, so need clarification.

The main differences I'm seeing are:

  1. Old ciphers are not loaded by default, I still need a workaround for this - I believe OPENSSL_CONF could be used https://github.com/hirura/hrr_rb_ssh/issues/32
irb(main):003:0> OpenSSL::Cipher.new('BF-CBC')
(irb):2:in `initialize': unsupported (OpenSSL::Cipher::CipherError)
	from (irb):2:in `new'
	from (irb):2:in `<main>'
	from /var/lib/gems/3.0.0/gems/irb-1.3.6/exe/irb:11:in `<top (required)>'
	from /usr/local/bin/irb:25:in `load'
	from /usr/local/bin/irb:25:in `<main>'
  1. set_pqg throws an exception at runtime:
[1] pry(HrrRbSsh::OpenSSLCompat)> dh = OpenSSL::PKey::DH.new
=> #<OpenSSL::PKey::DH:0x00007f8e1c6d0298 oid=dhKeyAgreement>
[2] pry(HrrRbSsh::OpenSSLCompat)> dh.set_pqg p, nil, g
OpenSSL::PKey::PKeyError: dh#set_pqg= is incompatible with OpenSSL 3.0
from (pry):2:in `set_pqg'
  1. openssl objects are immutable:
[4] pry(HrrRbSsh::OpenSSLCompat)> dh.generate_key!
OpenSSL::PKey::DHError: OpenSSL::PKey::DH is immutable on OpenSSL 3.0; use OpenSSL::PKey.generate_key instead

The change loose Ruby 2.6 or earlier support, right ? I'd like to clarify this because, originally, I tried to keep this available for older Ruby versions.

I'm hoping this change will be backwards compatible with Ruby 2.6 - although it is now end of life https://www.ruby-lang.org/en/downloads/branches/

For the proof of concept I've added an if statement to verify if the native openssl library is greater than version 3.0.0 or not.

adfoster-r7 avatar Jul 19 '22 03:07 adfoster-r7

Thank you ! That's quite informative. You're right, Ruby 2.6 and earlier are already EOL. This is just for my interest. Please go ahead without backward compatibility consideration.

hirura avatar Jul 19 '22 03:07 hirura

Just two error groups left until the unit tests will pass green again with the current approach of skipping the disabled deprecated ciphers such as blowfish etc (for now)

I'm not sure why these are failing just yet, any guidance appreciated :)

  1. SshDss - This seems to fail on Ubuntu 20.04 too
Failures:
  1) HrrRbSsh::Transport::ServerHostKeyAlgorithm::SshDss when secret_key is not specified #verify with correct sign returns true
     Failure/Error: expect( server_host_key_algorithm.verify sign, data ).to be true
       expected true
            got false
     # ./spec/hrr_rb_ssh/transport/server_host_key_algorithm/ssh_dss_spec.rb:121:in `block (5 levels) in <top (required)>'
  1. DiffieHellmanGroup17Sha512 - I verified that P matches the specification that I found here https://datatracker.ietf.org/doc/html/rfc3526#page-6, and I'm not sure why it would be failing on OpenSSL 3 🤔
  1) HrrRbSsh::Transport::KexAlgorithms::DiffieHellmanGroup17Sha512#initialize can be instantiated
     Failure/Error: OpenSSL::PKey.generate_key(dh)
     OpenSSL::PKey::PKeyError:
       EVP_PKEY_keygen: modulus too small
     # ./lib/hrr_rb_ssh/open_ssl_compat.rb:18:in `generate_key'
     # ./lib/hrr_rb_ssh/open_ssl_compat.rb:18:in `new_dh_pkey'
     # ./lib/hrr_rb_ssh/transport/kex_algorithms/diffie_hellman.rb:11:in `initialize'
     # ./spec/hrr_rb_ssh/transport/kex_algorithms/diffie_hellman_group17_sha512_spec.rb:3:in `new'
     # ./spec/hrr_rb_ssh/transport/kex_algorithms/diffie_hellman_group17_sha512_spec.rb:3:in `block (2 levels) in <top (required)>'
     # ./spec/hrr_rb_ssh/transport/kex_algorithms/diffie_hellman_group17_sha512_spec.rb:76:in `block (3 levels) in <top (required)>'

adfoster-r7 avatar Jul 19 '22 04:07 adfoster-r7

The DSS server host key verification failure reproduced on my Mac. It might be due to Ruby 3.1.x, not OpenSSL version.

# RUBY_VERSION => 3.1.2
# OpenSSL::OPENSSL_VERSION => OpenSSL 1.1.1f  31 Mar 2020

Failures:

  1) HrrRbSsh::Transport::ServerHostKeyAlgorithm::SshDss when secret_key is not specified #verify with correct sign returns true
     Failure/Error: expect( server_host_key_algorithm.verify sign, data ).to be true
     
       expected true
            got false
     # ./spec/hrr_rb_ssh/transport/server_host_key_algorithm/ssh_dss_spec.rb:124:in `block (5 levels) in <top (required)>'

hirura avatar Jul 19 '22 13:07 hirura

I was able to re-enable legacy openssl providers for the test suite by setting ENV['OPENSSL_CONF'] to the new config file that I've added in spec/fixtures/openssl.conf. Users that want the legacy ciphers (blowfish-cbc, cast128-cbc, arcfour) will also have to opt-in. I think this information could be added to the hrr_rb_ssh README or a changelog somewhere - but let me know your thoughts :+1:

I'm now blocked on the following two issues

  • SshDss verify - which looks like an existing issue:
    • Fails on Ubuntu 20.04, Ruby 3.x, OpenSSLRubyVersion=3.0.0, OpenSSLVersionNumber=OpenSSL 1.1.1f 31 Mar 2020, OpenSSLLibraryVersionOpenSSL 1.1.1f 31 Mar 2020
    • Passes on Ubuntu 20.04, Ruby 2.7, OpenSSLRubyVersion=2.1.3, OpenSSLVersionNumber=OpenSSL 1.1.1f 31 Mar 2020, OpenSSLLibraryVersionOpenSSL 1.1.1f 31 Mar 2020
    • Test run: https://github.com/adfoster-r7/hrr_rb_ssh/runs/7410201720?check_suite_focus=true
    • Fails on your local set up too, thanks for testing!
  • DiffieHellmanGroup17Sha512 modulus errors

adfoster-r7 avatar Jul 19 '22 13:07 adfoster-r7

For the DiffieHellmanGroup17Sha512 modulus error, I found my wrong P definition. I will raise an issue and fix outside of this PR.

In diffie_hellman_group17_sha512.rb,

        P = \
          "FFFFFFFF" "FFFFFFFF" "C90FDAA2" "2168C234" "C4C6628B" "80DC1CD1 29024E08" \

should be

          "FFFFFFFF" "FFFFFFFF" "C90FDAA2" "2168C234" "C4C6628B" "80DC1CD1" "29024E08" \

hirura avatar Jul 19 '22 15:07 hirura

Awesome; nice catch! :+1:

When I compared hrr_rb_ssh's P definition to the RFC, I intentionally removed whitespace - so I unfortunately missed that 😄

adfoster-r7 avatar Jul 19 '22 16:07 adfoster-r7

DH group17's P is now fixed. Please see #36. Most probably I did the same thing...

hirura avatar Jul 19 '22 21:07 hirura

Awesome, thanks! I've just rebased against that commit; now CI only has one unit test failing across the Ruby/OpenSSL versions - which seems like a separate issue to OpenSSL 3 support

  1) HrrRbSsh::Transport::ServerHostKeyAlgorithm::SshDss when secret_key is not specified #verify with correct sign returns true
     Failure/Error: expect( server_host_key_algorithm.verify sign, data ).to be true
       expected true
            got false
     # ./spec/hrr_rb_ssh/transport/server_host_key_algorithm/ssh_dss_spec.rb:121:in `block (5 levels) in <top (required)>'

adfoster-r7 avatar Jul 19 '22 22:07 adfoster-r7

That's good to hear. Give me some moment so that I figure out the cause and fix the DSS issue.

hirura avatar Jul 19 '22 23:07 hirura

I took some time to dig in the dss issue: see #37. I could successfully run the test suite with this PR on Ubuntu 22.04 with system Ruby (3.0) \o/.

smortex avatar Jul 21 '22 00:07 smortex

Thanks for taking a look @smortex - that pointed me in a good direction to dig further 🕵️

Tests should be passing green now 🟢

adfoster-r7 avatar Jul 29 '22 17:07 adfoster-r7

For context on the final change to get the DSS tests working:

It looks like by default Ruby is generating q with different bit length between the OpenSSL versions:

# OpenSSL 1.1.1
2.7.2 :017 > OpenSSL::PKey::DSA.new(1024).q.to_s(2).bytes.count * 8
 => 160
# OpenSSL 3.x
irb(main):002:0> OpenSSL::PKey::DSA.new(1024).q.to_s(2).bytes.count
=> 224

RFC 4253 links to FIPS-186-2; Which has some requirements on the values of N and L. Since it looks like OpenSSL 3 now generated N as 224 by default, then L should be 2048:

https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf

4.2 Selection of Parameter Sizes and Hash Functions for DSA
This Standard specifies the following choices for the pair L and N (the bit lengths of p and q, respectively):
L = 1024, N = 160
L = 2048, N = 224
L = 2048, N = 256
L = 3072, N = 256

So we could most likely update the default secret key for OpenSSL 3 to be updated to match:

- SECRET_KEY = OpenSSL::PKey::DSA.new(1024).to_pem
+ SECRET_KEY = OpenSSL::PKey::DSA.new(2048).to_pem

However as noted by @smortex we'd also need to change these lines as well to handle the different byte size:

https://github.com/hirura/hrr_rb_ssh/pull/37/files#diff-9af7963c32cdeaf03421690df2a1bb0b89b0ba0b73751a6dd572139e3ba0a7cfL59-R60

To make things easier - I've just gone ahead and updated the key generation to use L=1024,N=160 for consistency - using the options from https://www.openssl.org/docs/man3.0/man1/openssl-genpkey.html

So the old OpenSSL::PKey::DSA.new(1024).to_pem call has now been swapped for something more explicit for the OpenSSL 3 codepath:

OpenSSL::PKey.generate_key(
    OpenSSL::PKey.generate_parameters('DSA', {
      # The number of bits in the generated prime
      'dsa_paramgen_bits' => 1024,
      # The number of bits in the q parameter. Must be one of 160, 224 or 256. If not specified 224 is used.
      # This must align with an allowed combination from Section 4.2 in
      # https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf
      'dsa_paramgen_q_bits' => 160
    })
).to_pem

I think a separate PR could clean up the existing hard coded 20 byte size in the sign/verify command so that L could be 2048 and and N could be 224, but I don't think that should be a blocker.

adfoster-r7 avatar Jul 29 '22 17:07 adfoster-r7

Awesome :heart_eyes: !

smortex avatar Jul 31 '22 20:07 smortex

@hirura Let me know if there's anything else I can do to help with landing this PR 💯

adfoster-r7 avatar Aug 04 '22 23:08 adfoster-r7

@hirura I've tidied up the PR to only include the openssl changes now. The unit tests are passing, and I've also tested it with your example test script from smortex's PR, as well as verifying these changes work with metasploit-framework on ubuntu 22.04.

I wasn't sure if there were any of the demo scripts that I should also test out, but let me know and I can see if there's anything else I need to change 💯

adfoster-r7 avatar Aug 15 '22 11:08 adfoster-r7

OpenSSL 1.1.x is EOL in September, fwiw, so support for OpenSSL 3 by then would help out distros a lot if possible.

thesamesam avatar Mar 31 '23 23:03 thesamesam

@hirura Let me know if there's anything else I can do to help here :+1:

adfoster-r7 avatar Jun 02 '23 13:06 adfoster-r7

@hirura Is there anything I can do to help here? 📈

adfoster-r7 avatar Aug 14 '23 09:08 adfoster-r7

@hirura I've updated this to include the tests passing on Ruby 3.3.0-preview1 and 3.3.0-preview2 :+1:

I think this would be good to release 🤞

adfoster-r7 avatar Sep 26 '23 09:09 adfoster-r7