Skip to content

Backport 4.1: ssl_fork_server RNG testing - #10858

Open
gilles-peskine-arm wants to merge 13 commits into
Mbed-TLS:mbedtls-4.1from
gilles-peskine-arm:ssl_fork_server-rng-test-4.1
Open

Backport 4.1: ssl_fork_server RNG testing#10858
gilles-peskine-arm wants to merge 13 commits into
Mbed-TLS:mbedtls-4.1from
gilles-peskine-arm:ssl_fork_server-rng-test-4.1

Conversation

@gilles-peskine-arm

@gilles-peskine-arm gilles-peskine-arm commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test that ssl_fork_server has a different RNG state in each client. Fixes #10664.

Forward port of #10666. Differences:

  • "SSL debug: dump the server ephemeral public key on the client": a chunk of code that's specific to legacy ECDH is omitted in 4.x.
  • "Test for distinct server ephemeral public keys in ssl_fork_server": Remove TLS 1.2 DHE test case since DHE was removed in 4.0.
  • "More testing of NV-seed only builds: run some ssl-opt, do legacy crypto": very different structure. In 4.x, component_test_entropy_nv_seed_only already exists in crypto but we now add it to mbedtls so that we can run ssl-opt.sh. Also, in 4.x, there's only one variant because MBEDTLS_USE_PSA_CRYPTO is always enabled.
  • "ssl_fork_server: Assert unique server random values with identical client random values": adapt test case dependencies for an NV-seed-only build.

PR checklist

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
`ssl_fork_server` reseeds its RNG, so in a build with a nonvolatile seed, it
re-reads the seed file midway. If the client also rewrites the seed file
while the server is running, that causes the test conditions to not reflect
real-world conditions for the server.

Fix this by running `ssl_fork_server` in its own directory when we're
asserting properties of the random generator. This was a relatively easy
way to separate the client seed file from the server seed file, since the
seed file name `"seedfile"` is not configurable at runtime.

This mostly matters for builds with no actual entropy source, only an NV seed.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Assert that when two clients connect to ssl_fork_server, they see different
random values from the server. This validates that each child process has
its own random generator state.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Assert that when two clients connect to ssl_fork_server, they see different
ephemeral public keys from the server. This validates that each child
process has its own random generator state.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Run some of `ssl-opt.sh` on builds with only an NV seed as the entropy
source. There isn't much impact outside of the crypto unit tests, but do
test that it works in a "real" program. In particular, run `ssl_fork_server`
which has a peculiar challenge since its RNG state is replicated.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
…ient random values

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Fix a bug introduced in "Fix temporary directory left behind on normal exit"
that caused the exit code of `ssl-opt.sh` to be 1 on success.

Also, when exiting on a trapped signal, re-raise the signal rather than
exiting with status 1.

Also, on failure, where we report the number of failures through the exit
code, clamp the exit code to 125 to avoid confusion with standard values.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
gilles-peskine-arm force-pushed the ssl_fork_server-rng-test-4.1 branch from 5bbac53 to 1e55066 Compare August 5, 2026 17:25
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Aug 5, 2026
bensze01
bensze01 previously approved these changes Aug 7, 2026

@bensze01 bensze01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - faithful forward port minus the differences discussed in the description.

@bjwtaylor bjwtaylor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small suggestion, that I would be interested in what you think to.

Comment thread tests/ssl-opt.sh
server_with_own_seedfile () {
set -eu
test -d "$subdirectory_for_server_seedfile" || mkdir "$subdirectory_for_server_seedfile"
test -s "$subdirectory_for_server_seedfile/seedfile" || cp seedfile "$subdirectory_for_server_seedfile"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unconditional copy requires provision of the seedfile prior to running tests/ssl-opt.sh. Currently this seedfile is created in all.sh, which means that this then succeeds. However this will prevent the running of ssl-opt.sh without the provisioning of this file. Given that the command is a very short lived dd from urandom and as far as I can tell there are only 8 uses, is it worth moving this dd somewhere within ssl-opt. As this will make it easier to debug and maintain ssl-opt.sh?

The test case passes even if the two clients have different RNG seeds, but
in that case, it doesn't do what it claims, but instead it duplicates the
"2 successive clients, TLS 1.3" test case above. So run
"2 successive clients with same seed, TLS 1.3" only when there is no entropy,
like "2 successive clients with same seed, TLS 1.3".

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
bjwtaylor
bjwtaylor previously approved these changes Aug 13, 2026
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component-tls needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)

Projects

Development

Successfully merging this pull request may close these issues.

3 participants