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.

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

[TW#18008] Hardware MPI/Bignum causes heap corruption when mbedtls_ecp_mul() #1556

Closed
caffeine93 opened this issue Jan 28, 2018 · 2 comments
Closed
Assignees

Comments

@caffeine93
Copy link

caffeine93 commented Jan 28, 2018

The following code (when hardware MPI is enabled)

`

mbedtls_entropy_context ctxEntropy;
mbedtls_ctr_drbg_context ctxRandom;
const char* pers = "myecdsa";

mbedtls_entropy_init(&ctxEntropy);
mbedtls_ctr_drbg_init(&ctxRandom);
mbedtls_ctr_drbg_seed(&ctxRandom, mbedtls_entropy_func, &ctxEntropy,
		(const unsigned char*) pers, strlen(pers));

mbedtls_ecdsa_context ctxECDSA;
mbedtls_ecdsa_init(&ctxECDSA);
mbedtls_ecp_group_load(&ctxECDSA.grp, MBEDTLS_ECP_DP_SECP256K1);
mbedtls_mpi_copy(&ctxECDSA.d, node->privKey);

mbedtls_ecp_mul(&ctxECDSA.grp, &ctxECDSA.Q, &ctxECDSA.d, &ctxECDSA.grp.G,
		mbedtls_ctr_drbg_random, &ctxRandom);

`

causes heap corruption:

`
CORRUPT HEAP: multi_heap.c:369 detected at 0x3ffb3d30
abort() was called at PC 00x40085d48 on core 0
0x40085d48: multi_heap_assert at C:/msys32/home/Admin/esp/esp-idf/components/hea
p/multi_heap_platform.h:55
(inlined by) multi_heap_malloc_impl at C:/msys32/home/Admin/esp/esp-idf/compone
nts/heap/multi_heap.c:369

Backtrace: 0x400863e8:0x3ffb3c00 0x400864e7:0x3ffb3c20 0x40085d48:0x3ffb3c40 0x4
0081f10:0x3ffb3c60 0x40081f41:0x3ffb3c80 0x40082595:0x3ffb3ca0 0x4000bef5:0x3ffb
3cc0 0x400e25a5:0x3ffb3ce0 0x400e9173:0x3ffb3d00 0x400e72a1:0x3ffb3d30 0x400e3e7
d:0x3ffb3da0 0x400e4d07:0x3ffb3dc0 0x400e5839:0x3ffb3e00 0x400e591e:0x3ffb3e20 0
x400e24c9:0x3ffb3e40 0x400e1e7c:0x3ffb41d0 0x400d15c2:0x3ffb4200
0x400863e8: invoke_abort at C:/msys32/home/Admin/esp/esp-idf/components/esp32/pa
nic.c:572

0x400864e7: abort at C:/msys32/home/Admin/esp/esp-idf/components/esp32/panic.c:5
72

0x40085d48: multi_heap_assert at C:/msys32/home/Admin/esp/esp-idf/components/hea
p/multi_heap_platform.h:55
(inlined by) multi_heap_malloc_impl at C:/msys32/home/Admin/esp/esp-idf/compone
nts/heap/multi_heap.c:369

0x40081f10: heap_caps_malloc at C:/msys32/home/Admin/esp/esp-idf/components/heap
/heap_caps.c:116

0x40081f41: heap_caps_malloc_default at C:/msys32/home/Admin/esp/esp-idf/compone
nts/heap/heap_caps.c:145

0x40082595: _calloc_r at C:/msys32/home/Admin/esp/esp-idf/components/newlib/sysc
alls.c:52

0x400e25a5: mbedtls_mpi_grow at C:/msys32/home/Admin/esp/esp-idf/components/mbed
tls/library/bignum.c:2157

0x400e9173: mem_block_to_mpi at C:/msys32/home/Admin/esp/esp-idf/components/mbed
tls/port/esp_bignum.c:183
(inlined by) mbedtls_mpi_mul_mpi at C:/msys32/home/Admin/esp/esp-idf/components
/mbedtls/port/esp_bignum.c:565

0x400e72a1: ecp_mod_koblitz at C:/msys32/home/Admin/esp/esp-idf/components/mbedt
ls/library/ecp_curves.c:1271
(inlined by) ecp_mod_p256k1 at C:/msys32/home/Admin/esp/esp-idf/components/mbed
tls/library/ecp_curves.c:1323

0x400e3e7d: ecp_modp at C:/msys32/home/Admin/esp/esp-idf/components/mbedtls/libr
ary/ecp.c:660

0x400e4d07: ecp_check_pubkey_sw at C:/msys32/home/Admin/esp/esp-idf/components/m
bedtls/library/ecp.c:660

0x400e5839: mbedtls_ecp_check_pubkey at C:/msys32/home/Admin/esp/esp-idf/compone
nts/mbedtls/library/ecp.c:1877

0x400e591e: mbedtls_ecp_mul at C:/msys32/home/Admin/esp/esp-idf/components/mbedt
ls/library/ecp.c:1688 (discriminator 1)

0x400e24c9: generateMasterNode at C:/Users/Admin/Documents/EmbeddedProjects/ESP3
2/TestProject/main/NGen/NGen.c:302

0x400e1e7c: app_main at C:/Users/Admin/Documents/EmbeddedProjects/ESP32/NGen/
main/main.c:29
`

node is a struct that contains:
`

mbedtls_mpi* privKey;
mbedtls_mpi* pubKey;

`
both of which are properly allocated prior to the pasted code

Disabling the hardware accelerated MPI in the config menu ("make menuconfig") solves the issue and the multiplication produces the expected result

@negativekelvin
Copy link
Contributor

Something with word alignment perhaps?

@projectgus projectgus self-assigned this Jan 29, 2018
@projectgus
Copy link
Contributor

projectgus commented Jan 29, 2018

Thanks for the bug report. I was able to reproduce this problem.

The issue is that ecp_mod_koblitz() defines the buffer for an mbedtls_mpi number on the stack, Mp[P_KOBLITZ_MAX + P_KOBLITZ_R + 1]. This is preallocated on the stack because this function knows the maximum size of the result. However, the bignum hardware implementation conservatively tries to grow the result buffer to the size of a hardware bignum result - ie it allocates a new buffer, copies Mp's contents into the new buffer, and then tries to free Mp. The free() fails as the heap implementation sees this is not a heap allocated buffer (a situation which is indistinguishable from heap corruption.)

We will look into fixes for this, there are a few places where this "worst case" allocation takes place in the esp_bignum.c code. In the meantime, the workaround you have (disable hardware MPI) is the fastest way forward.

@FayeY FayeY changed the title Hardware MPI/Bignum causes heap corruption when mbedtls_ecp_mul() [TW#18008] Hardware MPI/Bignum causes heap corruption when mbedtls_ecp_mul() Jan 29, 2018
@igrr igrr closed this as completed in 961f59f Aug 30, 2018
catalinio pushed a commit to catalinio/pycom-esp-idf that referenced this issue Jun 28, 2019
…ration

Avoids growing the result of hardware bignum operations
(particularly for multiplication)

Fixes bugs where some Elliptic Curve operations fail or corrupt memory,
as they assume length of the number is never greater than the number of
non-zero limbs.

Includes some general refactoring to standardize terminology.

Closes espressif/esp-idf#1556

Fixes TW12984

Adds test cases for both these issues.
turmary pushed a commit to Seeed-Studio/Seeed_Arduino_mbedtls that referenced this issue Jan 22, 2020
…ration

Avoids growing the result of hardware bignum operations
(particularly for multiplication)

Fixes bugs where some Elliptic Curve operations fail or corrupt memory,
as they assume length of the number is never greater than the number of
non-zero limbs.

Includes some general refactoring to standardize terminology.

Closes espressif/esp-idf#1556

Fixes TW12984

Adds test cases for both these issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants