Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions library/ssl_tls12_client.c
Original file line number Diff line number Diff line change
Expand Up @@ -1645,17 +1645,27 @@ static int ssl_parse_server_ecdh_params(mbedtls_ssl_context *ssl,
* 5+ ECPoint contents
*/
if (end - *p < 4) {
MBEDTLS_SSL_DEBUG_MSG(2,
("bad server key exchange message: too short (%u)",
(unsigned) (end - *p)));
return MBEDTLS_ERR_SSL_DECODE_ERROR;
}

/* First byte is curve_type; only named_curve is handled */
if (*(*p)++ != MBEDTLS_ECP_TLS_NAMED_CURVE) {
MBEDTLS_SSL_DEBUG_MSG(2,
("bad server key exchange message: not named_curve (%u != %u)",
(unsigned) (*p)[-1],
(unsigned) MBEDTLS_ECP_TLS_NAMED_CURVE));
return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE;
}

/* Next two bytes are the namedcurve value */
tls_id = MBEDTLS_GET_UINT16_BE(*p, 0);
*p += 2;
MBEDTLS_SSL_DEBUG_MSG(3,
("server key exchange message: server picked ECDHE curve 0x%04x",
(unsigned) tls_id));

/* Check it's a curve we offered */
if (mbedtls_ssl_check_curve_tls_id(ssl, tls_id) != 0) {
Expand All @@ -1676,13 +1686,23 @@ static int ssl_parse_server_ecdh_params(mbedtls_ssl_context *ssl,
/* Keep a copy of the peer's public key */
ecpoint_len = *(*p)++;
if ((size_t) (end - *p) < ecpoint_len) {
MBEDTLS_SSL_DEBUG_MSG(2,
("bad server key exchange message: too short for point (%u < %u)",
(unsigned) (end - *p),
(unsigned) ecpoint_len));
return MBEDTLS_ERR_SSL_DECODE_ERROR;
}

if (ecpoint_len > sizeof(handshake->xxdh_psa_peerkey)) {
MBEDTLS_SSL_DEBUG_MSG(2,
("bad server key exchange message: ecpoint_len too long "
"(%" MBEDTLS_PRINTF_SIZET " > %" MBEDTLS_PRINTF_SIZET ")",
ecpoint_len,
sizeof(handshake->xxdh_psa_peerkey)));
return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE;
}

MBEDTLS_SSL_DEBUG_BUF(3, "server ephemeral public key", *p, ecpoint_len);
memcpy(handshake->xxdh_psa_peerkey, *p, ecpoint_len);
handshake->xxdh_psa_peerkey_len = ecpoint_len;
*p += ecpoint_len;
Expand Down
4 changes: 4 additions & 0 deletions library/ssl_tls13_generic.c
Original file line number Diff line number Diff line change
Expand Up @@ -1505,6 +1505,10 @@ int mbedtls_ssl_tls13_read_public_xxdhe_share(mbedtls_ssl_context *ssl,
sizeof(handshake->xxdh_psa_peerkey)));
return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE;
}
MBEDTLS_SSL_DEBUG_BUF(3, (mbedtls_ssl_conf_get_endpoint(ssl->conf) == MBEDTLS_SSL_IS_CLIENT ?
"server ephemeral public key" :
"client ephemeral public key"),
p, peerkey_len);
memcpy(handshake->xxdh_psa_peerkey, p, peerkey_len);
handshake->xxdh_psa_peerkey_len = peerkey_len;

Expand Down
76 changes: 74 additions & 2 deletions tests/opt-testcases/sample.sh
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ run_test "Sample: ssl_server, gnutls client, TLS 1.3" \

run_test "Sample: ssl_fork_server, ssl_client2" \
-P 4433 \
"$PROGRAMS_DIR/ssl_fork_server" \
"server_with_own_seedfile $PROGRAMS_DIR/ssl_fork_server" \
"$PROGRAMS_DIR/ssl_client2" \
0 \
-s "[1-9][0-9]* bytes read" \
Expand All @@ -206,9 +206,81 @@ run_test "Sample: ssl_fork_server, ssl_client2" \
-S "error" \
-C "error"

two_clients () (
"$PROGRAMS_DIR/ssl_client2" "$@" &&
"$PROGRAMS_DIR/ssl_client2" "$@"
)

run_test "Sample: ssl_fork_server, 2 successive clients, TLS 1.2: unique random" \
-P 4433 \
"server_with_own_seedfile $PROGRAMS_DIR/ssl_fork_server" \
"two_clients force_version=tls12 debug_level=3" \
0 \
-S "error" \
-C "error" \
-v 'distinct_server_random'

run_test "Sample: ssl_fork_server, 2 successive clients, TLS 1.3: unique random" \
-P 4433 \
"server_with_own_seedfile $PROGRAMS_DIR/ssl_fork_server" \
"two_clients force_version=tls13 debug_level=3" \
0 \
-S "error" \
-C "error" \
-v 'distinct_server_random'

two_clients_same_seed () (
cp seedfile seedfile1 &&
"$PROGRAMS_DIR/ssl_client2" "$@" &&
mv seedfile1 seedfile &&
"$PROGRAMS_DIR/ssl_client2" "$@"
)

requires_config_enabled MBEDTLS_ENTROPY_NO_SOURCES_OK
requires_config_disabled MBEDTLS_PSA_BUILTIN_GET_ENTROPY
requires_config_disabled MBEDTLS_PSA_DRIVER_GET_ENTROPY
run_test "Sample: ssl_fork_server, 2 successive clients with same seed, TLS 1.2: unique random" \
-P 4433 \
"server_with_own_seedfile $PROGRAMS_DIR/ssl_fork_server" \
"two_clients_same_seed force_version=tls12 debug_level=3" \
0 \
-S "error" \
-C "error" \
-v 'distinct_server_random'

run_test "Sample: ssl_fork_server, 2 successive clients with same seed, TLS 1.3: unique random" \
-P 4433 \
"server_with_own_seedfile $PROGRAMS_DIR/ssl_fork_server" \
"two_clients_same_seed force_version=tls13 debug_level=3" \
0 \
-S "error" \
-C "error" \
-v 'distinct_server_random'

# Pick ECDHE-RSA, not ECDHE-ECDSA, because ssl_fork_server only loads one key,
# and it uses an RSA key if both RSA and ECDSA are available.
run_test "Sample: ssl_fork_server, 2 successive clients, TLS 1.2: unique ephemeral ECDHE" \
-P 4433 \
"server_with_own_seedfile $PROGRAMS_DIR/ssl_fork_server" \
"two_clients force_version=tls12 force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-128-GCM-SHA256 debug_level=3" \
0 \
-S "error" \
-C "error" \
-v 'distinct_server_ephemeral'

requires_config_enabled MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED
run_test "Sample: ssl_fork_server, 2 successive clients, TLS 1.3: unique ephemeral" \
-P 4433 \
"server_with_own_seedfile $PROGRAMS_DIR/ssl_fork_server" \
"two_clients force_version=tls13 tls13_kex_modes=ephemeral debug_level=3" \
0 \
-S "error" \
-C "error" \
-v 'distinct_server_ephemeral'

run_test "Sample: ssl_client1 with ssl_fork_server" \
-P 4433 \
"$PROGRAMS_DIR/ssl_fork_server" \
"server_with_own_seedfile $PROGRAMS_DIR/ssl_fork_server" \
"$PROGRAMS_DIR/ssl_client1" \
0 \
-s "[1-9][0-9]* bytes read" \
Expand Down
24 changes: 24 additions & 0 deletions tests/scripts/components-configuration-platform.sh
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,30 @@ component_test_psa_driver_get_entropy()
$MAKE_COMMAND test
}

component_test_entropy_nv_seed_only () {
msg "build: NV seed only (NV seed only)"
scripts/config.py full
scripts/config.py unset MBEDTLS_PSA_BUILTIN_GET_ENTROPY
scripts/config.py set MBEDTLS_ENTROPY_NO_SOURCES_OK

cmake -D CMAKE_BUILD_TYPE:String=Check .
make

# Check that the library seems to refer to the seedfile, but not to
# platform entropy sources.
grep seedfile tf-psa-crypto/platform/CMakeFiles/platform.dir/platform.c.o
not grep getrandom tf-psa-crypto/drivers/builtin/CMakeFiles/builtin.dir/src/entropy*.o platform/CMakeFiles/platform.dir/platform*.o

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.

So, I may have not understood the operation of this. However as far as I can tell platform/CMakeFiles/platform.dir/platform*.o doesn't exist. In which case these chacks fail open? Or have I missed something somewhere?

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.

Accidentally approved instead of commented, so have re requested the review. Let me know what you think on the above?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In which case these chacks fail open?

Yes, they do. We've had a fix waiting for review for a while: Mbed-TLS/mbedtls-framework#268 . And it keeps breaking when I re-run the CI because we add new failures.

I'll fix it here.

not grep /dev/random tf-psa-crypto/drivers/builtin/CMakeFiles/builtin.dir/src/entropy*.o platform/CMakeFiles/platform.dir/platform*.o
not grep /dev/.random tf-psa-crypto/drivers/builtin/CMakeFiles/builtin.dir/src/entropy*.o platform/CMakeFiles/platform.dir/platform*.o
not grep mbedtls_platform_get_entropy tf-psa-crypto/drivers/builtin/CMakeFiles/builtin.dir/src/entropy*.o platform/CMakeFiles/platform.dir/platform*.o

msg "test: NV seed only"
make test

msg "test: NV seed only - ssl-opt on sample programs"
tests/ssl-opt.sh -f 'Default\|Sample'
}

component_build_no_sockets () {
# Note, C99 compliance can also be tested with the sockets support disabled,
# as that requires a POSIX platform (which isn't the same as C99).
Expand Down
55 changes: 49 additions & 6 deletions tests/ssl-opt.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1564,6 +1564,12 @@ check_test_failure() {
return
fi
;;
"-v")
if ! ${PYTHON:-} ../framework/scripts/validate_ssl_logs.py "$CLI_OUT" "$SRV_OUT" $2; then
fail "Validation failed on client and server output"
return
fi
;;

*)
echo "Unknown test: $1" >&2
Expand Down Expand Up @@ -1637,6 +1643,35 @@ do_run_test_once() {
fi
}

# In builds with an NV seed, and especially with _only_ an NV seed,
# the fact that the client and the server use the same seedfile may be
# disruptive. In particular, it could cause a buggy server to pass just
# because the client has rewritten the seed file between connections.
#
# So when we're specificially testing randomness, make the server use
# its own seedfile. The seedfile name is hardcoded as "seedfile", but
# we can change the current directory. This is easy for the sample
# programs that don't use external files (it would be harder for those that
# are called with arguments referencing files under data_files/).
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"
cd "$subdirectory_for_server_seedfile"
# Assume that the path to the program is a relative path, and
# that the directory is a simple subdirectory.
server="../$1"
shift
# This process must become the server process, otherwise
# wait_server_start won't notice when the server starts listening.
exec "$server" "$@"
}

# Directory for running a server with its own seedfile (see above).
# Define the variable here so that it's always available, in particular
# in cleanup(). But only create and populate the directory on demand.
subdirectory_for_server_seedfile="tmp-ssl-opt-wd-$$"

# Detect if the current test is going to use TLS 1.3 or TLS 1.2.
# $1 and $2 contain the server and client command lines, respectively.
#
Expand Down Expand Up @@ -1954,11 +1989,14 @@ cleanup() {
rm -f $CLI_OUT $SRV_OUT $PXY_OUT $SESSION
rm -f context_srv.txt
rm -f context_cli.txt
if [ -d "$subdirectory_for_server_seedfile" ]; then
rm -f "$subdirectory_for_server_seedfile/seedfile"
rmdir "$subdirectory_for_server_seedfile"
fi
test -n "${SRV_PID:-}" && kill $SRV_PID >/dev/null 2>&1
test -n "${PXY_PID:-}" && kill $PXY_PID >/dev/null 2>&1
test -n "${CLI_PID:-}" && kill $CLI_PID >/dev/null 2>&1
test -n "${DOG_PID:-}" && kill $DOG_PID >/dev/null 2>&1
exit 1
}

#
Expand Down Expand Up @@ -2144,7 +2182,9 @@ SESSION="session.$$"

SKIP_NEXT="NO"

trap cleanup INT TERM HUP
trap 'cleanup; trap - HUP; kill -HUP $$' HUP
trap 'cleanup; trap - INT; kill -INT $$' INT
trap 'cleanup; trap - TERM; kill -TERM $$' TERM

# Basic test

Expand Down Expand Up @@ -14565,9 +14605,12 @@ EOF
fi
fi

if [ $FAILS -gt 255 ]; then
# Clamp at 255 as caller gets exit code & 0xFF
# (so 256 would be 0, or success, etc)
FAILS=255
cleanup

if [ $FAILS -gt 125 ]; then
# Clamp at 125 as caller gets exit code & 0xFF
# (so 256 would be 0, or success, etc), and 126 and above have
# conventional meanings (fork/exec failure, signal).
FAILS=125
fi
exit $FAILS