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

ssl-opt.sh requirement checker #4767

Draft
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Jul 9, 2021

When reviewing #4695, I started to write increasingly complex perl one-long-liners to check that the new requires_xxx calls matched the needs. Eventually I decided that this was getting too complex for one-liners, so here's a reasonably well-structured script that does some analysis of requires_xxx calls vs run_test parameters.

This script has a framework that can be extended easily, but currently it only checks a few easy-to-identify boolean options and requires_max_content_len (with false positives).

Typical usage (from a worktree with the ssl-opt.sh you want to check — note that the script defaults to checking its own worktree, not the worktree of the current directory):

../worktrees/check-ssl-opt-initial/tests/scripts/check_ssl_opt.py tests/ssl-opt.sh

This is nowhere near production ready and I currently have no plans to do further work except to add further ad hoc checks or improve the documentation (I'll try to do that while it's still reasonably fresh in my mind). I think that rather than improve this script, we can improve the logic inside ssl-opt.sh itself to reduce the number of ad hoc requirements. There's already logic to analyze ciphersuite= on the command line, and even with the limitations of shell scripting, it would be easy to analyze several other options.

The output as of a0c54c3 is

DTLS fragmenting: openssl server, DTLS 1.2;requires_max_content_len 2048 but max_frag_len=0, mtu=512
DTLS fragmenting: 3d, gnutls server, DTLS 1.2;requires_max_content_len 2048 but max_frag_len=0, mtu=512
DTLS fragmenting: 3d, openssl server, DTLS 1.2;requires_max_content_len 2048 but max_frag_len=0, mtu=512
Handshake memory usage initial (MFL 16384 - default);requires_max_content_len 16384 for no discernible reason
Handshake memory usage initial (MFL 16384 - default);requires_max_content_len 16384 but max_frag_len=0, mtu=0
Handshake memory usage (MFL 4096);max_frag_len=4096 but no requires_max_content_len
Handshake memory usage (MFL 2048);max_frag_len=2048 but no requires_max_content_len
Handshake memory usage (MFL 1024);max_frag_len=1024 but no requires_max_content_len
Handshake memory usage (MFL 512);max_frag_len=512 but no requires_max_content_len

and I am satisfied that these are false positives due to limitations of the analysis in this script (it doesn't understand the diversity of requirements related to mtu values, and it doesn't figure out the “factored out” requires_max_content_len call for the "Handshake memory usage" scripts).

Patch ssl-opt.sh to make it just print information about each run_test call
and the applicable requires_xxx calls.

Analyze the resulting information to detect missing or spurious
requirements.

This commit analyzes:
* requires_config_enabled for MBEDTLS_SSL_DTLS_CONNECTION_ID,
  MBEDTLS_SSL_CONTEXT_SERIALIZATION and
  MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK. The analysis matches the
  script.
* requires_max_content_len. The analysis has some false positives.

This is a work in progress, fragile and not always well documented.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@tom-daubney-arm tom-daubney-arm added the historical-reviewed Reviewed & agreed to keep legacy PR/issue label May 18, 2023
@tom-daubney-arm
Copy link
Contributor

As part of our review of historical PRs we have made the decision to convert older PRs that have not been updated in 3 months into drafts until they are worked on again.

@tom-daubney-arm tom-daubney-arm marked this pull request as draft May 18, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-test Test framework and CI scripts component-tls DO-NOT-MERGE historical-reviewed Reviewed & agreed to keep legacy PR/issue needs-work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants