Test "nonexistent" in psa its suite causes NULL p_data to propagate to fwrite in psa_its_set #203

New issue

Closed

opened 2019-08-05 21:35:00 -05:00 by nanokatze · 5 comments

nanokatze commented

2019-08-05 21:35:00 -05:00

(Migrated from github.com)

Description

Mbed Crypto 1.1.1
Toolchain GCC 9.1.1
Target Linux 5.1.20 x86_64 (Fedora 30 5.1.20-300)

Test
https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/tests/suites/test_suite_psa_its.function#L167
will propagate NULL p_data to fwrite
https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/library/psa_its_file.c#L212
which must not be NULL. This is reproduced with appending sanitizer flags -fsanitize=address,undefined flags to CFLAGS in library/Makefile and tests/Makefile and building and running the tests with sanitizers enabled.

Issue request type

[ ] Question
[ ] Enhancement
[X] Bug

# Description Mbed Crypto 1.1.1 Toolchain GCC 9.1.1 Target Linux 5.1.20 x86_64 (Fedora 30 5.1.20-300) Test https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/tests/suites/test_suite_psa_its.function#L167 will propagate NULL p_data to fwrite https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/library/psa_its_file.c#L212 which must not be NULL. This is reproduced with appending sanitizer flags -fsanitize=address,undefined flags to CFLAGS in library/Makefile and tests/Makefile and building and running the tests with sanitizers enabled. ### Issue request type [ ] Question [ ] Enhancement [X] Bug

ciarmcom commented

2019-08-06 00:59:37 -05:00

(Migrated from github.com)

Internal Jira reference: https://jira.arm.com/browse/IOTCRYPT-850

*Internal Jira reference: https://jira.arm.com/browse/IOTCRYPT-850*

gilles-peskine-arm commented

2019-08-06 04:17:13 -05:00

(Migrated from github.com)

psa_its_set_wrap( uid, 0, NULL, 0 )

ends up calling psa_its_set( uid, 0, NULL, 0 ). Since the buffer size is 0, NULL is a valid buffer pointer.

The C standard doesn't see it that way (at least for mem* functions, NULL/0 is UB; for stdio functions, it doesn't seem to be specified). So we should indeed avoid calling fwrite in this case.

Oddly, we do test with Asan and our CI is passing. But I can reproduce the failure locally with gcc5 and make CFLAGS='-O2 -fsanitize=address,undefined'. So we should figure out what options we're using, and tweak or add to the CI job.

``` psa_its_set_wrap( uid, 0, NULL, 0 ) ``` ends up calling `psa_its_set( uid, 0, NULL, 0 )`. Since the buffer size is 0, `NULL` is a valid buffer pointer. The C standard doesn't see it that way (at least for `mem*` functions, NULL/0 is UB; for stdio functions, it doesn't seem to be specified). So we should indeed avoid calling `fwrite` in this case. Oddly, we _do_ test with Asan and our CI is passing. But I can reproduce the failure locally with gcc5 and `make CFLAGS='-O2 -fsanitize=address,undefined'`. So we should figure out what options we're using, and tweak or add to the CI job.

gilles-peskine-arm commented

2019-08-06 04:40:19 -05:00

(Migrated from github.com)

Ok, I understand why CI is passing. I thought we always used Asan and UBsan together (-fsanitize=address,undefined), but in CMakelists.txt, we activate UBSan with Clang but not with GCC. And we only run that test with ASan on CI through cmake with GCC, so we don't run it with UBSan. https://github.com/ARMmbed/mbedtls/issues/2780

Ok, I understand why CI is passing. I thought we always used Asan and UBsan together (`-fsanitize=address,undefined`), but in `CMakelists.txt`, we activate UBSan with Clang but not with GCC. And we only run that test with ASan on CI through cmake with GCC, so we don't run it with UBSan. https://github.com/ARMmbed/mbedtls/issues/2780

AndrzejKurek commented

2019-08-28 01:35:06 -05:00

(Migrated from github.com)