Skip to content
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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oam7575
Copy link
Contributor

@oam7575 oam7575 commented Mar 18, 2024

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

… exception of always suggesting a _second_ option.

jtesta#166
@jtesta
Copy link
Owner

jtesta commented Mar 18, 2024

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
Copy link
Owner

jtesta commented Mar 18, 2024

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
Copy link
Contributor Author

oam7575 commented Mar 19, 2024

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

  • What about crypto with different names.
  • What about Dropbear vs OpenSSH with different crypto supported or the "same" crypto with slightly different names.
  • What if Dropbear releases a new version that suddenly supports a range of crypto that OpenSSH has had for ages, but is new for Dropbear.
  • What if a previous "good" cipher suddenly had a vulnerability discovered.
  • What if someone discovers some new magic next week and a new super awesome cant be broken cipher is implemented.
  • What if I want to expressly not suggest a specific cipher for my usage cases for reasons of policy / compliance.

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

  • Only suggest strong crypto, never weaker / older ( noting the code will currently suggest a maximum of 2x ciphers )
  • Supporting "equivalent" crypto between Dropbear, OpenSSH based on weights.
  • Easy way to deprecate a cipher ( single change - reduce the number )
  • Easy way to not suggest a cipher ( single change - set the weight to "WEIGHT_FAIL" )
  • Easy way to increase the preference of a cipher ( increase weight )
  • Easy way to add a new cipher that we want ( add to existing kexdb with details and appropriate weight ).

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', '[email protected]', '[email protected]']:
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants