jtesta / ssh-audit

SSH server & client security auditing (banner, key exchange, encryption, mac, compression, compatibility, security, etc)
MIT License
3.36k stars 176 forks source link

DRAFT 1 - Attempt to implement never suggest weaker ciphers #259

Open oam7575 opened 6 months ago

oam7575 commented 6 months ago

with the exception of always suggesting a second option.

166

@jtesta

Hello All.

This is a first draft pull request attempting to address #166 There have been wholesale changes to the ssh2_kexdb.py file to include "preference weights" for all algorithms. algorithms.py has been updated to make use of these weights.

WARNING / CAUTION Preference weights have been selected by me for testing purposes. While I have attempted to put some thought into these weights I cannot make any sort of guarantee that they are appropriate.

Essentially anything already marked as FAIL = low score ( 1 ) For everything else if it was something that I am somewhat familiar with I tended to weight larger numbers = better 128 <-- 256 <-- 512 group15 < -- group16 <-- group17 <-- group18

jtesta commented 6 months ago

Thanks for putting in the time for this PR! Seems like you did a lot of work.

I do think, however, that this could have been solved in a much easier way. The reporter for issue #166 had a concern that the tool was recommending algorithms that were weaker than was enabled on his target server. For example, it was recommending that they enable AES128 when AES256 was already supported. The reporter misunderstood the tool's output; it wasn't saying that you must enable AES128, but rather that it is an option to maximize compatibility. To clear up this type of confusion, instead of telling the user (rec) +aes128-ctr -- enc algorithm to append, perhaps change the text to enc algorithm to optionally append for better compatibility.

Originally, I had thought of a different way to avoid this confusion, which would have resulted in more significant changes. That's why I had added the "help wanted" label. But now that I thought about it again, I'm thinking just a one-line code change might be all that we need to solve the root cause.

Again, I do appreciate the effort you've invested in this!

jtesta commented 6 months ago

I'm curious, though, about this system of weighted algorithms: could you describe more what the ultimate goal of them is? Is it to only suggest algorithms stronger than what the target supports?

oam7575 commented 6 months ago

Hi Joe,

Is it to only suggest algorithms stronger than what the target supports?

Essentially yes - but a little more also. A bit of a brain dump follows.

Begins

When I first read #166 , the name of the issue is "Don't recommend weaker crypto", and the OP directly mentions AES 256 and AES 128 ; I thought, that seems simple enough, just match strings / numbers in the suggested crypto and if we are suggesting 128 , then dont do that because it would be weaker.

Then the more I thought about it, the more I considered

How do we deal with all of these different cases in one go? Can it be done?

The more I thought about it, the more a weighted scale seemed ( naively ? ) to be one approach to catch most of those issues in one go.

I am prepared to freely admit there might be issues with my approach, however I think it allows for the following

Easy way to not suggest a cipher ( single change - set the weight to "WEIGHT_FAIL" )

As an example this line of code

MIN_SUGGEST_WEIGHT = 200 if faults > 0 or (alg_type == 'key' and (('-cert-' in alg_name) or (alg_name.startswith('sk-')))) or empty_version or sugg_weight < MIN_SUGGEST_WEIGHT: continue

Could be reduced to:

if sugg_weight < MIN_SUGGEST_WEIGHT: continue

I think there are a few other places where similar changes could be made, such as:

if alg_name in ['diffie-hellman-group-exchange-sha256', 'rsa-sha2-256', 'rsa-sha2-512', 'rsa-sha2-256-cert-v01@openssh.com', 'rsa-sha2-512-cert-v01@openssh.com']: rec[sshv][alg_type]['chg'][alg_name] = faults

That could probably be changed to:

WARN_WEIGHT = 750 if sugg_weight > MIN_SUGGEST_WEIGHT and sugg_weight < WARN_WEIGHT : rec[sshv][alg_type]['chg'][alg_name] = faults

If a new cipher fails for some reason, the rest of the code does not ( should not ) need to be changed. Simply adjust the weight value in the kexdb and the cipher should now be warned against.

Potentially this could be expanded to specifically fail a cipher; that is to say to force a cipher onto the to the delete list for reasons of policy / compliance. Even if the cipher is considered OK for most cases, an individual user would be able to simply change the weighted scale of the cipher, perhaps to "-500".

Then with some hopefully similar / small changes around the delete list, the otherwise good, but undesired cipher would be added to the "delete" list to prompt the user to remove that cipher from their server.

Further, while I haven't looked closely these changes should tie in with #211 and #212 Add the warnings and adjust the weights in kexdb, suggestion to add / del / change should(?) take care of themselves.

Obviously I have made a range of assumptions with the code and the weights I have assigned during initial testing, but they seemed reasonable enough at the time for initial testing and development.

Happy to discuss further as needed. If nothing else, I got a bit of practice from the exercise. Cheers.