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

Client renego refactoring #2598

Open
wants to merge 7 commits into
base: 3.2
Choose a base branch
from

Conversation

Tazmaniac
Copy link

Describe your changes

To correctly fix #2590 and potentially other reals corner cases, a complete refactoring of the test is needed.
It remove all the calls to openssl s_client other than the one in the big events driven testing loop.
The result is more robust, faster and cleaner.
It fixes a bad interaction bug with the --openssl-timeout option too.

As a side note, Akamai started to implement a mitigation (before renegotiation could only be enabled or disabled) . It close the connection after a completely unpredictable number of renegotiation (up to more than 30 seen) from the client point of view. So it could lead to potentially "false positive" (well it depend of your point of view...).
I will try later to reduce this noise, but this is not a regression.

What is your pull request about?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Typo fix
  • Documentation update
  • Update of other files

If it's a code change please check the boxes which are applicable

  • For the main program: My edits contain no tabs and the indentation is five spaces
  • I've read CONTRIBUTING.md and Coding_Convention.md
  • I have tested this fix against >=2 hosts and I couldn't spot a problem
  • I have tested this new feature against >=2 hosts which show this feature and >=2 host which does not (in order to avoid side effects) . I couldn't spot a problem
  • For the new feature I have made corresponding changes to the documentation and / or to help()
  • If it's a bigger change: I added myself to CREDITS.md (alphabetical order) and the change to CHANGELOG.md

All cases could be handled by the single openssl s_client invocation
loop:
- dispatch and adjust comments to not loose them
- remove the first s_client invocation: stuck connections are allready
  handled by the main loop
- remove the second s_client invocation: normal case and server closed
  connections are allready handled by the main loop. The loop take care
  of the race between server connection close and s_client terminating
  too by doing another loop run, not closing STDIN.
- special non HTTP case equivalent to ssl_reneg_attempts=2
- specialcase only the HTTP result printing to not change the output

- openssl-timeout option clashe badly with the main loop logic:
  Introduce $OPENSSL_NOTIMEOUT
In the wait loop, I was relying on a 1s sleep to eliminate a possible
late zero return value server close on the last attempt.
- do globaly one more harmless "for" iteration
  and remove the sleep 1 for faster and more robust result
- correct the non HTTP case iteration value
- adjust the timeout to the conservative 6s in the non HTTP case,
  for HTTP case it become 33s
- improve comments
- Recover the "not vulnerable" case (no mitigation) printing, cosmetic
  fix.
- With the removing of all s_client invocation other than the main loop
  one, fix the init of the ERRFILE and TMPFILE: no need to append, no
  need to remove, inconditionally zap the content before the loop.
@Tazmaniac
Copy link
Author

I don't understand the CI failed tests. The "broken pipe" error from subshells should not be propagated and affect the main shell script return code and does not in "normal" run. Does the CI test modifying the shell 'pipefail' 'errtrace' or 'errexit' options ?

@drwetter
Copy link
Owner

Haven´t really looked at it . Just re-ran the test once. I also don´t know what's going on.

I would recommend to checkout and run the script from the "home dir"

@drwetter
Copy link
Owner

Is #2597 obsolete?

@Tazmaniac
Copy link
Author

Yes #2598 is the complete fix. #2597 was a quick minimal workaroud for the reported defect.

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.

[BUG] Secure Client-Initiated Renegotiation Testing Yields False Vulnerability
3 participants