-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
base: master
Are you sure you want to change the base?
Conversation
… exception of always suggesting a _second_ option. jtesta#166
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 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! |
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? |
Hi Joe,
Essentially yes - but a little more also. A bit of a brain dump follows. BeginsWhen 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? 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
As an example this line of code
Could be reduced to:
I think there are a few other places where similar changes could be made, such as:
That could probably be changed to:
If a new cipher fails for some reason, the rest of the code does not ( should not ) need to be changed. 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. 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 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. |
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