NetExec Refactor SSH Protocol for Improved Key Handling/Auth Checks

Description

Fixes #410, #596, and #605 by eliciting an AuthenticationException error from the SSH server (by supplying dummy credentials) and parsing out the allowed authentication methods. If password auth is not supported and --key-file is not supplied as a command-line arg then we skip SSH auth and display a more appropriate failure event.

The below Paramiko test case served as the basis for returning the collection of available SSH auth methods:

import paramiko

host = '10.10.11.230'
username = 'whatever'
port = 22

transport = paramiko.Transport((host, port))
transport.start_client()

try:
    transport.auth_none(username)
except paramiko.BadAuthenticationType as e:
    print("Available authentication methods:", e.allowed_types)
except Exception as ex:
    print("An error occurred:", ex)
finally:
    transport.close()

Type of change

Please delete options that are not relevant.

[x] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tested on HTB CozyHosting.

NXC Version: 1.3.0 Commit: f8293d15 Installation Method: pipx

Screenshots (if appropriate):

Before: before_ssh

After: ssh_after

Checklist:

[x] I have ran Ruff against my changes.
[x] New and existing e2e tests pass locally with my changes
[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas

Mar 13 '25 20:03 Mercury0

Converting to draft PR after encountering a few erroneous test cases. Updates to follow.

Mar 13 '25 20:03 Mercury0

Adjusted key auth success logger output so we don't mistakenly lead user to believe the password was validated. Previously the password would be displayed in the success message.

Additionally introduced logic to skip password auth check if --key-file is supplied as a command line arg.

ssh_updated2 ssh_updated3

ToDo: Alter logic to assess whether --key-file requires an accompanying passphrase.

Mar 14 '25 15:03 Mercury0

SSH keyfile passphrase validation checking implemented. If correct passphrase is supplied for an SSH key which is not correlated to the user in SSH connection object, we indicate a validation failure rather than incorrect passphrase.

ssh_passphrases_testing

Mar 14 '25 16:03 Mercury0

This is quickly turning into a major protocol rewrite. While testing against SSH on Windows hosts I'm encountering the Unknown exception: q must be exactly 160, 224, or 256 bits long error frequently. Research has led me to conclude that Paramiko has multiple issues in its handling of SSH keys, namely treating RSA keys as DSA.

Rather than use some of the hacky workarounds suggested in multiple issues, I'm working on a function to dynamically check the key type using the cryptography module which is already a project dependency.

Mar 14 '25 20:03 Mercury0

Thanks for the work!

This is quickly turning into a major protocol rewrite. While testing against SSH on Windows hosts I'm encountering the Unknown exception: q must be exactly 160, 224, or 256 bits long error frequently. Research has led me to conclude that Paramiko has multiple issues in its handling of SSH keys, namely treating RSA keys as DSA.

Rather than use some of the hacky workarounds suggested in multiple issues, I'm working on a function to dynamically check the key type using the cryptography module which is already a project dependency.

That is an issue known for a long time: https://github.com/Pennyw0rth/NetExec/issues/410 And will probably never been fixed by the paramiko maintainer, as the original issue addressing key identification is over 11 years old by now: https://github.com/paramiko/paramiko/issues/387

Mar 15 '25 14:03 NeffIsBack

This is quickly turning into a major protocol rewrite. While testing against SSH on Windows hosts I'm encountering the Unknown exception: q must be exactly 160, 224, or 256 bits long error frequently. Research has led me to conclude that Paramiko has multiple issues in its handling of SSH keys, namely treating RSA keys as DSA.

Rather than use some of the hacky workarounds suggested in multiple issues, I'm working on a function to dynamically check the key type using the cryptography module which is already a project dependency.

Thinking from the top of my head, if we already implement a detection for the keys, how hard would it be to fix it in paramiko itself, solving https://github.com/paramiko/paramiko/issues/387 and all the other issues? While the PR would be open, we could just merge it for ourselves like we do it sometimes for impacket, oscrypto or pynfsclient

Mar 15 '25 16:03 NeffIsBack

This is quickly turning into a major protocol rewrite. While testing against SSH on Windows hosts I'm encountering the Unknown exception: q must be exactly 160, 224, or 256 bits long error frequently. Research has led me to conclude that Paramiko has multiple issues in its handling of SSH keys, namely treating RSA keys as DSA. Rather than use some of the hacky workarounds suggested in multiple issues, I'm working on a function to dynamically check the key type using the cryptography module which is already a project dependency.

Thinking from the top of my head, if we already implement a detection for the keys, how hard would it be to fix it in paramiko itself, solving paramiko/paramiko#387 and all the other issues? While the PR would be open, we could just merge it for ourselves like we do it sometimes for impacket, oscrypto or pynfsclient

To me that makes the most sense -- to fix the issue at its source. The fact that the issues with Paramiko have been known for over a decade without resolution gives me concern that its not a simple fix. On the flip side, Paramiko has been around for a long time and has mature documentation. I'm going to keep making the most of it for now, but I do think migration to a different SSH library might make sense for long term stability.

Mar 16 '25 00:03 Mercury0

To me that makes the most sense -- to fix the issue at its source. The fact that the issues with Paramiko have been known for over a decade without resolution gives me concern that its not a simple fix. On the flip side, Paramiko has been around for a long time and has mature documentation. I'm going to keep making the most of it for now, but I do think migration to a different SSH library might make sense for long term stability.

Absolutely! If you know any library that could cover the needs for NetExec I am happy to switch immediately.

Mar 17 '25 01:03 NeffIsBack

nxc/helpers/ssh_key_utils.py added as a helper module to reduce line bloat in ssh.py. get_server_auth_methods() checks for server auth methods allowed by the SSH server. If password auth is not supported (and --key-file omitted), we skip the connection attempt. detect_key_type() function implemented which uses the cryptography module to identify key type. Current support: RSA, DSA, ECDSA, Ed25519, and Ed448. Also detects whether the keyFile requires a passphrase. get_key_owner() function parses keyfiles for comments which include key ownership information.

updated_ssh nxc_ssh_updated2 ssh_debug

Mar 17 '25 20:03 Mercury0

A good place to put stuff you don't want to have in the ssh protocol itself is the ssh folder in /protocols

Mar 18 '25 14:03 NeffIsBack

A good place to put stuff you don't want to have in the ssh protocol itself is the ssh folder in /protocols

Sounds good! I can move it out of /helpers soon once I'm done with my testing.

Mar 18 '25 18:03 Mercury0

Code is passing 90 pytest cases with 99% code coverage. Missing coverage pertains to DSA key auth which is deprecated and not supported on most modern OpenSSH implementations.

pytest

codecov

Mar 18 '25 18:03 Mercury0

All tests currently pass. Added custom markers in pytest.ini for quickly testing all code logic by category:

pytest -v -m authentication_checks
pytest -v -m key_loading
pytest -v -m key_detection
pytest -v -m key_conversion
pytest -v -m server_checks
pytest -v -m misc