-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix problems found by clang+UBSan on Ubuntu 20.04 #6614
base: development
Are you sure you want to change the base?
Changes from all commits
bc0ff75
4da2bd6
dd3c1a6
e304eab
ff88ece
1a6894c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 ), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
which has the properties: the words The only disadvantage (depending how understandable you find There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't look like #6609 touched There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tentative name |
||||||
output_size - default_iv_length, | ||||||
output_length ); | ||||||
|
||||||
exit: | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ) | ||
|
@@ -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 ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As with the introduction of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm on the fence. This is considerably simpler than 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A helper function would be useful here indeed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
output_size - accumulated_length, | ||
&olength ); | ||
if( status != PSA_SUCCESS ) | ||
|
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----- |
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----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The Jenkins logs don't show the ouput of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our Groovy code is silently skipping ubuntu-20.04 jobs 😦 Patch incoming. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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" | ||
|
@@ -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: | ||
|
There was a problem hiding this comment.
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.