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.

Sign up for GitHub

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

Jump to bottom

Fix problems found by clang+UBSan on Ubuntu 20.04 #6614

Draft
wants to merge 6 commits into
base: development
Choose a base branch
from
+128 −74
Draft

Fix problems found by clang+UBSan on Ubuntu 20.04 #6614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc0ff75
Fix NULL+0 undefined behavior in ECB encryption and decryption
gilles-peskine-arm Oct 22, 2022
4da2bd6
Use modern Clang rather than old GCC for full-config ASan+UBSan test
gilles-peskine-arm Nov 16, 2022
dd3c1a6
Merge the two full-config build+test components
gilles-peskine-arm Nov 22, 2022
e304eab
Replace some SHA-1 signed certificates by SHA-2 signed
gilles-peskine-arm Nov 22, 2022
ff88ece
Fix GnuTLS priority handling
gilles-peskine-arm Nov 22, 2022
1a6894c
Update the Travis "full" build to use modern Clang
gilles-peskine-arm Nov 16, 2022
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
23 changes: 22 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,29 @@ jobs:
- tests/scripts/all.sh -k build_arm_linux_gnueabi_gcc_arm5vte build_arm_none_eabi_gcc_m0plus

- name: full configuration
os: linux
dist: focal
addons:
apt:
packages:
- clang-10
- gnutls-bin
script:
- tests/scripts/all.sh -k test_full_cmake_gcc_asan
# Do a manual build+test sequence rather than using all.sh,
# because there's no all.sh component that does what we want.
# Although test_full_make_clang_asan is close, it doesn't work:
# - The clang executable in the default PATH is Clang 7 on
# Travis's focal instances, but we want Clang >= 10.
# - The all.sh component needs an ancient version of OpenSSL
# which we'd have to install manually.
- make generated_files
- make CC=clang-10 CXX=clang-10 CFLAGS='-Werror -Wall -Wextra -fsanitize=address,undefined -fno-sanitize-recover=all -O2' LDFLAGS='-Werror -Wall -Wextra -fsanitize=address,undefined -fno-sanitize-recover=all'
- make test
- tests/ssl-opt.sh
# Modern openssl does not support fixed ECDH or null ciphers.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect that in 2.28, we'll need to exclude more stuff, because 2.28 supports more obsolete features that the non-antique OpenSSL and GnuTLS don't.

This means that the backport risks being an even longer chain of whack-a-mole to upgrade or work around our obsolescent test data and scripts. If this can be merged before the 3.3 freeze (depending on CI success and reviewer time), I'd like to go ahead and merge even if the backport misses the release freeze.

- tests/compat.sh -p "OpenSSL" -e 'NULL\|ECDH-'
- tests/compat.sh -p "GnuTLS mbedTLS"
- tests/context-info.sh

- name: Windows
os: windows
Expand Down
3 changes: 3 additions & 0 deletions ChangeLog.d/psa-ecb-ub.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Bugfix
* Fix undefined behavior (typically harmless in practice) in PSA ECB
encryption and decryption.
3 changes: 2 additions & 1 deletion library/psa_crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3467,7 +3467,8 @@ psa_status_t psa_cipher_encrypt( mbedtls_svc_key_id_t key,
status = psa_driver_wrapper_cipher_encrypt(
&attributes, slot->key.data, slot->key.bytes,
alg, local_iv, default_iv_length, input, input_length,
output + default_iv_length, output_size - default_iv_length,
( output == NULL ? NULL : output + default_iv_length ),
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just as a possibility, so we can have something concrete to discuss

Suggested change
( output == NULL ? NULL : output + default_iv_length ),
NULL_SAFE_PTR_ADD( output, default_iv_length ),

which has the properties: the words NULL and SAFE give an indication of WHY this is done; PTR_ADD should be sufficiently explanatory and easy to learn; doesn't require repeating output or NULL and is actually shorter.

The only disadvantage (depending how understandable you find PTR_ADD) is that the + sign disappears

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is from #6609 and I won't change it again here. I'm going to rebase those commits out, in fact, since they're causing confusion. (I hadn't rebased them because there are no merge conflicts and I thought keeping them would help incremental review.)

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like #6609 touched psa_crypto.c - what am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, my bad, only the bignum part was in the earlier PR, not the PSA crypto part. So this is still up for review.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, why not separate that bit out into an independent PR? Then this one really can be "get the CI running a later/better sanitiser"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally wanted to have non-regression testing for the issues I'm fixing. But given how hard getting even one of Travis or Jenkins to run on Ubuntu 20.04 is turning out, I'll do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tentative name mbedtls_buffer_offset. I made it a function rather than a macro because it's safer, but a downside is that it's typed so there needs to be a separate name for the const version.

output_size - default_iv_length,
output_length );

exit:
Expand Down
11 changes: 8 additions & 3 deletions library/psa_crypto_cipher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,8 @@ psa_status_t mbedtls_psa_cipher_encrypt(
goto exit;

status = mbedtls_psa_cipher_finish( &operation,
output + update_output_length,
( output == NULL ? NULL :
output + update_output_length ),
output_size - update_output_length,
&finish_output_length );
if( status != PSA_SUCCESS )
Expand Down Expand Up @@ -563,15 +564,19 @@ psa_status_t mbedtls_psa_cipher_decrypt(
goto exit;
}

status = mbedtls_psa_cipher_update( &operation, input + operation.iv_length,
status = mbedtls_psa_cipher_update( &operation,
( input == NULL ? NULL :
input + operation.iv_length ),
input_length - operation.iv_length,
output, output_size, &olength );
if( status != PSA_SUCCESS )
goto exit;

accumulated_length = olength;

status = mbedtls_psa_cipher_finish( &operation, output + accumulated_length,
status = mbedtls_psa_cipher_finish( &operation,
( output == NULL ? NULL :
output + accumulated_length ),
Copy link
Contributor

Choose a reason for hiding this comment

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

As with the introduction of mpi_sint_abs(), the obvious question here is: since this construct is repeated four times, is it worth abstracting it, either with a function or a macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm on the fence. This is considerably simpler than mpi_sint_abs. It feels like giving it a meaningful name will make the code longer and won't really be more readable.

Are you happy with seeing the code, or are you going “WTF why do we need a special case for NULL”? If the latter, then a macro/function with documentation that explains why it's needed sometimes is definitely warranted.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it's clear that it would be meaningless to compute an address based on NULL + offset, and so having a special case makes sense, but less obvious that it's UB (as opposed to simply a meaningless result) without the special case. So I think either way would be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's clear why it is done - no comment needed. It's just that the construct is clunky, and much less readable than the (UB-problematic) original code

Copy link
Contributor

Choose a reason for hiding this comment

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

A helper function would be useful here indeed

Copy link
Contributor Author

@gilles-peskine-arm gilles-peskine-arm Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This part is from #6609 and I won't change it again here. The pull request here is to arrange for such bugs to be caught by our CI.
My bad, only the bignum part was in the earlier PR, not the PSA crypto part. So this is still up for review.

output_size - accumulated_length,
&olength );
if( status != PSA_SUCCESS )
Expand Down
41 changes: 32 additions & 9 deletions tests/compat.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,32 @@ filter()
echo "$NEW_LIST" | sed -e 's/[[:space:]][[:space:]]*/ /g' -e 's/^ //' -e 's/ $//'
}

# List all the ciphers supported by gnutls-xxx, except for protocol versions.
# Output a colon-separated list.
list_gnutls_prio_all()
{
o=$("$1" --list | awk -F: -v ORS=: '
# "Category: ELEMENT, ELEMENT"
NF == 2 && $1 ~ /^[A-Z]/ &&
# Exclude categories that cannot be used in priority strings
$1 != "Certificate types" && $1 != "Public Key Systems" &&
# Exclude protocols (one will be enabled manually later)
$1 != "Protocols" {
sub(/^ +/, "+", $2); gsub(/[, ]+/, ":+", $2);
print $2
}')
o=${o#:}
o=${o%:}
echo "NORMAL:$o"
}

# GnuTLS rejects some ciphers by default, and has no option to
# simply enable them all. So list all supported ones and enable them.
set_gnutls_prio()
{
G_SERVER_PRIO_BASE=$(list_gnutls_prio_all "$GNUTLS_SERV")
}

# OpenSSL 1.0.1h with -Verify wants a ClientCertificate message even for
# PSK ciphersuites with DTLS, which is incorrect, so disable them for now
check_openssl_server_bug()
Expand Down Expand Up @@ -558,17 +584,10 @@ setup_arguments()
exit 1;
esac

# GnuTLS < 3.4 will choke if we try to allow CCM-8
if [ -z "${GNUTLS_MINOR_LT_FOUR-}" ]; then
G_PRIO_CCM="+AES-256-CCM-8:+AES-128-CCM-8:"
else
G_PRIO_CCM=""
fi

M_SERVER_ARGS="server_port=$PORT server_addr=0.0.0.0 force_version=$MODE"
O_SERVER_ARGS="-accept $PORT -cipher NULL,ALL -$O_MODE"
G_SERVER_ARGS="-p $PORT --http $G_MODE"
G_SERVER_PRIO="NORMAL:${G_PRIO_CCM}+NULL:+MD5:+PSK:+DHE-PSK:+ECDHE-PSK:+SHA256:+SHA384:+RSA-PSK:-VERS-TLS-ALL:$G_PRIO_MODE"
G_SERVER_PRIO="$G_SERVER_PRIO_BASE:$G_PRIO_MODE"

# The default prime for `openssl s_server` depends on the version:
# * OpenSSL <= 1.0.2a: 512-bit
Expand All @@ -593,7 +612,6 @@ setup_arguments()
M_CLIENT_ARGS="server_port=$PORT server_addr=127.0.0.1 force_version=$MODE"
O_CLIENT_ARGS="-connect localhost:$PORT -$O_MODE"
G_CLIENT_ARGS="-p $PORT --debug 3 $G_MODE"
G_CLIENT_PRIO="NONE:$G_PRIO_MODE:+COMP-NULL:+CURVE-ALL:+SIGN-ALL"

if [ "X$VERIFY" = "XYES" ];
then
Expand Down Expand Up @@ -943,7 +961,12 @@ if echo "$PEERS" | grep -i gnutls > /dev/null; then
echo "Command '$CMD' not found" >&2
exit 1
fi
set_gnutls_prio
done
else
# The variable needs to be set because it'll be used to set other
# variables, but those variables won't be used so the value doesn't matter.
G_SERVER_PRIO_BASE=
fi

for PEER in $PEERS; do
Expand Down
8 changes: 8 additions & 0 deletions tests/data_files/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,6 +1089,14 @@ server2-sha256.crt: server2.req.sha256
$(MBEDTLS_CERT_WRITE) request_file=server2.req.sha256 serial=2 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20190210144406 not_after=20290210144406 md=SHA256 version=3 output_file=$@
all_final += server2-sha256.crt

server2.ku-ke.crt: server2.req.sha256
$(MBEDTLS_CERT_WRITE) request_file=server2.req.sha256 serial=2 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20190210144406 not_after=20290210144406 md=SHA256 version=3 key_usage=key_encipherment output_file=$@
all_final += server2.ku-ke.crt

server2.ku-ds.crt: server2.req.sha256
$(MBEDTLS_CERT_WRITE) request_file=server2.req.sha256 serial=2 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20190210144406 not_after=20290210144406 md=SHA256 version=3 key_usage=digital_signature output_file=$@
all_final += server2.ku-ds.crt

# MD5 test certificate

cert_md_test_key = $(cli_crt_key_file_rsa)
Expand Down
27 changes: 13 additions & 14 deletions tests/data_files/server2.ku-ds.crt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,20 @@
-----BEGIN CERTIFICATE-----
MIIDijCCAnKgAwIBAgIBLDANBgkqhkiG9w0BAQUFADA7MQswCQYDVQQGEwJOTDER
MA8GA1UEChMIUG9sYXJTU0wxGTAXBgNVBAMTEFBvbGFyU1NMIFRlc3QgQ0EwHhcN
MTQwNDA5MDg0NDUxWhcNMjQwNDA2MDg0NDUxWjA0MQswCQYDVQQGEwJOTDERMA8G
A1UEChMIUG9sYXJTU0wxEjAQBgNVBAMTCWxvY2FsaG9zdDCCASIwDQYJKoZIhvcN
MIIDRzCCAi+gAwIBAgIBAjANBgkqhkiG9w0BAQsFADA7MQswCQYDVQQGEwJOTDER
MA8GA1UECgwIUG9sYXJTU0wxGTAXBgNVBAMMEFBvbGFyU1NMIFRlc3QgQ0EwHhcN
MTkwMjEwMTQ0NDA2WhcNMjkwMjEwMTQ0NDA2WjA0MQswCQYDVQQGEwJOTDERMA8G
A1UECgwIUG9sYXJTU0wxEjAQBgNVBAMMCWxvY2FsaG9zdDCCASIwDQYJKoZIhvcN
AQEBBQADggEPADCCAQoCggEBAMFNo93nzR3RBNdJcriZrA545Do8Ss86ExbQWuTN
owCIp+4ea5anUrSQ7y1yej4kmvy2NKwk9XfgJmSMnLAofaHa6ozmyRyWvP7BBFKz
NtSj+uGxdtiQwWG0ZlI2oiZTqqt0Xgd9GYLbKtgfoNkNHC1JZvdbJXNG6AuKT2kM
tQCQ4dqCEGZ9rlQri2V5kaHiYcPNQEkI7mgM8YuG0ka/0LiqEQMef1aoGh5EGA8P
hYvai0Re4hjGYi/HZo36Xdh98yeJKQHFkA4/J/EwyEoO79bex8cna8cFPXrEAjya
HT4P6DSYW8tzS1KW2BGiLICIaTla0w+w3lkvEcf36hIBMJcCAwEAAaOBnzCBnDAJ
BgNVHRMEAjAAMB0GA1UdDgQWBBSlBehkuNzfYA9QEk1gqGSvTYtDkzBjBgNVHSME
XDBagBS0WuSls97SUva51aaVD+s+vMf9/6E/pD0wOzELMAkGA1UEBhMCTkwxETAP
BgNVBAoTCFBvbGFyU1NMMRkwFwYDVQQDExBQb2xhclNTTCBUZXN0IENBggEAMAsG
A1UdDwQEAwIHgDANBgkqhkiG9w0BAQUFAAOCAQEAc4kubASrFXFtplkYp6FUcnUn
Pf/6laS1htI+3y+q1UHWe2PcagZtCHTCUGBSWLeUIiaIBheaIRqv+4sSFVuXB7hV
0PGXpO5btth4R8BHzGqCdObKvPujp5BDq3xgcAFicA3HUMNsJoTDv/RYXY7je1Q5
ntVyVPeji0AWMUYQjcqHTQQPGBgdJrRTMaYglZh15IhJ16ICNd9rWIeBA0h/+r0y
QuFEBz0nfe7Dvpqct7gJCv+7/5tCujx4LT17z7oK8BZN5SePAGU2ykJsUXk8ZICT
ongaQQVQwS6/GJ6A5V8ecaUvFrTby1h9+2sOW8n2NRGiaaG5gkvxVeayemcmOQ==
HT4P6DSYW8tzS1KW2BGiLICIaTla0w+w3lkvEcf36hIBMJcCAwEAAaNdMFswCQYD
VR0TBAIwADAdBgNVHQ4EFgQUpQXoZLjc32APUBJNYKhkr02LQ5MwHwYDVR0jBBgw
FoAUtFrkpbPe0lL2udWmlQ/rPrzH/f8wDgYDVR0PAQH/BAQDAgeAMA0GCSqGSIb3
DQEBCwUAA4IBAQBMhW/Q2znC/KjS4QrqTIqnn7pZ4uP6UqF01jgM45NMjHEijXhn
NPXFXCiWnU4V3x6HenwxlKtI5nUNxRvVPu8e1QJjNj/exUXihNS87BOpdzwJv6B5
CTZI1zjYLa9GcXgtoAnzF4HZfjtOIa216SZQctTl4QdWpy0Vf/RzhwvKVsIIrhso
YpCeziJq0TSGGy4h9JlaOA8WrjCpl2/v9EDJvh83+Zj5QUt1zTaDEQum5c9Dtuaz
FZ0WHyGkmTJzcXuVn6RNUGUYJAUcax/2mZV2LhokxVqf6yku6zv1/XvF1rfbORqA
sQ43oX9JC1fuhcD0jVlIWfHgd0z1n+nGaKX9
-----END CERTIFICATE-----
27 changes: 13 additions & 14 deletions tests/data_files/server2.ku-ke.crt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,20 @@
-----BEGIN CERTIFICATE-----
MIIDijCCAnKgAwIBAgIBKzANBgkqhkiG9w0BAQUFADA7MQswCQYDVQQGEwJOTDER
MA8GA1UEChMIUG9sYXJTU0wxGTAXBgNVBAMTEFBvbGFyU1NMIFRlc3QgQ0EwHhcN
MTQwNDA5MDg0NDM5WhcNMjQwNDA2MDg0NDM5WjA0MQswCQYDVQQGEwJOTDERMA8G
A1UEChMIUG9sYXJTU0wxEjAQBgNVBAMTCWxvY2FsaG9zdDCCASIwDQYJKoZIhvcN
MIIDRzCCAi+gAwIBAgIBAjANBgkqhkiG9w0BAQsFADA7MQswCQYDVQQGEwJOTDER
MA8GA1UECgwIUG9sYXJTU0wxGTAXBgNVBAMMEFBvbGFyU1NMIFRlc3QgQ0EwHhcN
MTkwMjEwMTQ0NDA2WhcNMjkwMjEwMTQ0NDA2WjA0MQswCQYDVQQGEwJOTDERMA8G
A1UECgwIUG9sYXJTU0wxEjAQBgNVBAMMCWxvY2FsaG9zdDCCASIwDQYJKoZIhvcN
AQEBBQADggEPADCCAQoCggEBAMFNo93nzR3RBNdJcriZrA545Do8Ss86ExbQWuTN
owCIp+4ea5anUrSQ7y1yej4kmvy2NKwk9XfgJmSMnLAofaHa6ozmyRyWvP7BBFKz
NtSj+uGxdtiQwWG0ZlI2oiZTqqt0Xgd9GYLbKtgfoNkNHC1JZvdbJXNG6AuKT2kM
tQCQ4dqCEGZ9rlQri2V5kaHiYcPNQEkI7mgM8YuG0ka/0LiqEQMef1aoGh5EGA8P
hYvai0Re4hjGYi/HZo36Xdh98yeJKQHFkA4/J/EwyEoO79bex8cna8cFPXrEAjya
HT4P6DSYW8tzS1KW2BGiLICIaTla0w+w3lkvEcf36hIBMJcCAwEAAaOBnzCBnDAJ
BgNVHRMEAjAAMB0GA1UdDgQWBBSlBehkuNzfYA9QEk1gqGSvTYtDkzBjBgNVHSME
XDBagBS0WuSls97SUva51aaVD+s+vMf9/6E/pD0wOzELMAkGA1UEBhMCTkwxETAP
BgNVBAoTCFBvbGFyU1NMMRkwFwYDVQQDExBQb2xhclNTTCBUZXN0IENBggEAMAsG
A1UdDwQEAwIFIDANBgkqhkiG9w0BAQUFAAOCAQEAqreLAIuxeLGKbhoEROYRqXxO
ndaC6uDcpxhgmEW7B2DW6ZtX8155v3ov61MuMas8fEQjD5STDP9qERxNTePnhW3m
kDZd2jUBE3ioHhTBv47i1PYU+DRe42kY6z0jUmNPK8TsTKfdbqTGXg9THe1KYB7q
hdljqGS08IgBl/q2lK2OOSycu27xhfb9Mo0BcLBab92WgyBu+cFPQsKiL4mD7QyJ
+73Ndb21EuANUjsRDQ3NPklssJcyJB2v85eekwk1acZUG21no3wdTvjxhVE/Xrdz
zUP9WkvAVfUrwGjUzG4YHE8wkHO7xKbKixNt+nQmDhe+tHVbztZjVwFJ8010gg==
HT4P6DSYW8tzS1KW2BGiLICIaTla0w+w3lkvEcf36hIBMJcCAwEAAaNdMFswCQYD
VR0TBAIwADAdBgNVHQ4EFgQUpQXoZLjc32APUBJNYKhkr02LQ5MwHwYDVR0jBBgw
FoAUtFrkpbPe0lL2udWmlQ/rPrzH/f8wDgYDVR0PAQH/BAQDAgUgMA0GCSqGSIb3
DQEBCwUAA4IBAQA4xoF8haz3yzTQdmujBfsWzEvpTpCibnkN/VIz4BOjdMkv/jTQ
3X1rHUW9io9YH3c+RearHbsJVk583/GxrnZvKI7lisMJhMfDFr5Vc4hdxTJMnqQ6
HgDEJIFrkelT5/hOv8X7YCNMkMCVCfUJecuRXCrutccXDlomVBprIL2qUDNma7yL
kUqarahUjTzAQvZhngQCggR+OOedImyIQwOrHTWWai6E+V+e6Z+our993ScELtW5
qdtmQPuHW1+7cdZLwKe74HqbA8PJu9eQIuM+ZLEOsPvsbPEERf09YCMRgsTyPpPv
cwpyDUBPf2Hy3x8bSFtS+MHSSbYqw9yR+F72
-----END CERTIFICATE-----
55 changes: 25 additions & 30 deletions tests/scripts/all.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,27 +924,47 @@ component_test_default_cmake_gcc_asan () {
tests/context-info.sh
}

component_test_full_cmake_gcc_asan () {
msg "build: full config, cmake, gcc, ASan"
component_test_full_make_clang_asan () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://ci.trustedfirmware.org/job/mbed-tls-pr-merge/job/PR-6614-merge/8/flowGraphTable/ doesn't list this job. What's going on?

The pre-test check should have caught test_full_make_clang_asan from all.sh --list-all-components, and should have either complained that none of Docker images can run it or decided to dispatch on one of the Docker images, based on all.sh --list-components output. And the Ubuntu 20.04 image should have included test_full_make_clang_asan in its all.sh --list-components.

The Jenkins logs don't show the ouput of all.sh --list-…, unfortunately. The pipeline has no branch for all_u20-test_full_make_clang_asan. So where did test_full_make_clang_asan go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our Groovy code is silently skipping ubuntu-20.04 jobs 😦 Patch incoming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix: Mbed-TLS/mbedtls-test#76

msg "build: full config, make, clang, ASan"
scripts/config.py full
CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan .
make
make CFLAGS="$ASAN_CFLAGS -O2" LDFLAGS="$ASAN_CFLAGS" TEST_CPP=1

msg "test: main suites (inc. selftests) (full config, ASan build)"
make test

msg "test: selftest (ASan build)" # ~ 10s
programs/test/selftest

msg "test: cpp_dummy_build (full config, clang)" # ~ 1s
programs/test/cpp_dummy_build

msg "test: psa_constant_names (full config, clang)" # ~ 1s
tests/scripts/test_psa_constant_names.py

msg "test: ssl-opt.sh (full config, ASan build)"
tests/ssl-opt.sh

msg "test: compat.sh (full config, ASan build)"
msg "test: compat.sh default set (full config, ASan build)"
tests/compat.sh

msg "test: compat.sh modern set (full config, ASan build)"
OPENSSL_CMD="$OPENSSL_NEXT" tests/compat.sh -e '^$' -f 'ARIA\|CHACHA'

msg "test: compat.sh antique set (full config, ASan build)"
OPENSSL_CMD="$OPENSSL_LEGACY" tests/compat.sh -e '^$' -f 'NULL'

msg "test: context-info.sh (full config, ASan build)" # ~ 15 sec
tests/context-info.sh
}
# Insist on a reasonably modern Clang. New versions find more problems,
# especially with UBSan.
support_test_full_make_clang_asan () {
local version
version="$(echo __clang_major__ | clang -E -)"
version="${version##*$'\n'}"
# Require at least the version in Ubuntu 20.04
[[ $version != [!0-9]* && $version -ge 10 ]]
}

component_test_psa_crypto_key_id_encodes_owner () {
msg "build: full config + PSA_CRYPTO_KEY_ID_ENCODES_OWNER, cmake, gcc, ASan"
Expand Down Expand Up @@ -1539,31 +1559,6 @@ component_test_psa_collect_statuses () {
rm -f tests/statuses.log
}

component_test_full_cmake_clang () {
msg "build: cmake, full config, clang" # ~ 50s
scripts/config.py full
CC=clang CXX=clang cmake -D CMAKE_BUILD_TYPE:String=Release -D ENABLE_TESTING=On -D TEST_CPP=1 .
make

msg "test: main suites (full config, clang)" # ~ 5s
make test

msg "test: cpp_dummy_build (full config, clang)" # ~ 1s
programs/test/cpp_dummy_build

msg "test: psa_constant_names (full config, clang)" # ~ 1s
tests/scripts/test_psa_constant_names.py

msg "test: ssl-opt.sh default, ECJPAKE, SSL async (full config)" # ~ 1s
tests/ssl-opt.sh -f 'Default\|ECJPAKE\|SSL async private'

msg "test: compat.sh NULL (full config)" # ~ 2 min
env OPENSSL_CMD="$OPENSSL_LEGACY" GNUTLS_CLI="$GNUTLS_LEGACY_CLI" GNUTLS_SERV="$GNUTLS_LEGACY_SERV" tests/compat.sh -e '^$' -f 'NULL'

msg "test: compat.sh ARIA + ChachaPoly"
env OPENSSL_CMD="$OPENSSL_NEXT" tests/compat.sh -e '^$' -f 'ARIA\|CHACHA'
}

component_test_memsan_constant_flow () {
# This tests both (1) accesses to undefined memory, and (2) branches or
# memory access depending on secret values. To distinguish between those:
Expand Down
4 changes: 2 additions & 2 deletions tests/suites/test_suite_psa_crypto.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3962,7 +3962,7 @@ void cipher_alg_without_iv( int alg_arg, int key_type_arg, data_t *key_data,
TEST_LE_U( length, output_buffer_size );
output_length += length;
PSA_ASSERT( psa_cipher_finish( &operation,
output + output_length,
output == NULL ? NULL : output + output_length,
output_buffer_size - output_length,
&length ) );
output_length += length;
Expand All @@ -3980,7 +3980,7 @@ void cipher_alg_without_iv( int alg_arg, int key_type_arg, data_t *key_data,
TEST_LE_U( length, output_buffer_size );
output_length += length;
PSA_ASSERT( psa_cipher_finish( &operation,
output + output_length,
output == NULL ? NULL : output + output_length,
output_buffer_size - output_length,
&length ) );
output_length += length;
Expand Down