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

Color all ciphers #905

Merged
merged 1 commit into from
Dec 4, 2017
Merged

Color all ciphers #905

merged 1 commit into from
Dec 4, 2017

Conversation

dcooper16
Copy link
Collaborator

This PR adds an additional COLOR level (3). If color is set to 3 then all ciphers are printed according to pr_cipher_quality() rather than just the "Negotiated cipher" in run_server_preference().

@dcooper16 dcooper16 force-pushed the color_all_ciphers branch 9 times, most recently from dd677b5 to 1778f6a Compare December 1, 2017 20:45
@drwetter
Copy link
Collaborator

drwetter commented Dec 1, 2017

Yeah, now I remember we discussed that a while back. And IIRC I personally wasn't so in favor of having this as a default. :) I realize though that it is a matter of taste. --color=3 seems absolutely fine as less obtrusive and it doesn't collide with a white background. Thanks for the PR!

The thing though I would like to have explained is the color rating, see picture.
screenshot_20171201_214220

anon seems like an accident to me. That needs to be in light red. AES256-SHA256/TLS_RSA_WITH_AES_256_CBC_SHA256 is the the very best but I wouldn't pick the color for medium, same with CAMELLIA. Yellow seems better to me.

@dcooper16
Copy link
Collaborator Author

Hi Dirk,

I've been maintaining the code to color all the ciphers for a while now in the branch that I've been working on for #333, and decided to create a PR with that code as a result of #891. I just changed it to only color the ciphers if --color=3 based on our earlier discussion.

This PR doesn't change the color ratings at all. It just uses pr_cipher_quality() for the cipher ratings, and pr_cipher_quality() was developed using the cipher ratings that were previously embedded in run_server_preference() (PR #637). So, it seems that changing the color ratings should be a separate PR.

TLS_RSA_WITH_AES_256_CBC_SHA256 isn't rated as the best since it is a CBC cipher. The comment in pr_cipher_quality() implies that this is because of BEAST. However, if TLSv1.2 isn't vulnerable to BEAST, then perhaps TLSv1.2-only CBC ciphers should be upgraded.

The color ratings don't really matter to me, though. In the branch for #333, pr_cipher_quality() uses a completely different set of rules for rating ciphers if the --SP800-52 flag is used:

  • If the cipher is permitted by SP 800-52 Revision 2, it is rated a 5, 6, or 7 (ok, good, best).
  • If the cipher is not permitted by SP 800-52 Revision 2, but was permitted by SP 800-52 Revision 1, it is rated a 3 (medium severity), unless the cipher uses 3DES. (3DES is rated worse since the Department of Homeland Security issued a Binding Operational Directive mandating that U.S. Federal agencies disable 3DES: https://cyber.dhs.gov/assets/report/bod-18-01.pdf).
  • If the cipher is not permitted by either SP 800-52 Revision 2 or Revision 1, then it is rated a 1 (critical).

This PR adds an additional COLOR level (3). If color is set to 3 then all ciphers are printed according to pr_cipher_quality() rather than just the "Negotiated cipher" in run_server_preference().
drwetter added a commit that referenced this pull request Dec 4, 2017
See pending PR #905 / issue #333.

There's still lots of work needed and probably the function
needs to be completely rewritten and to be in sync with
other parts of the program.
@drwetter drwetter merged commit cf89488 into testssl:2.9dev Dec 4, 2017
@drwetter
Copy link
Collaborator

drwetter commented Dec 4, 2017

Thank you David.

pr_cipher_quality() was kind of a forgotten function, at least for my weak memory. :-)

As stated in the commit (7f6ff5d) there's some work ahead to get the color rating consistent throughout testssl.sh. That is some work which needs to be done for a release. I need to look at your branch.

For now I guess I am fine to leave --color=3 as a hidden tweak (i.e. not appearing in help() or in the man page).

Thx again, Dirk

@dcooper16 dcooper16 deleted the color_all_ciphers branch December 4, 2017 21:22
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