Backport 2.1: Fix corner case uses of memory_buffer_alloc.c #1009

andresag01 · 2017-07-12T11:11:57Z

This change fixes the following corner cases in memory_buffer_alloc.c:

Calling verify_chain() on an uninitialized chain. Would previously cause a NULL dereference.
Allocating a memory block large enough to overflow the length variable.
Initializing the allocator with a buffer smaller than the size of a memory header.

This PR is based on the information recorded in #639.

NOTES:

This PR includes tests.
This is a backport for Fix corner case uses of memory_buffer_alloc.c #778
This PR backports a number of changes from @redtangent to mbed TLS 2.1 to support the additional tests in test/suites/test_suite_memory_buffer_alloc.{function,data}

RonEld

Why is this PR has much more changes (in the test code) than #778 ?

andresag01 · 2017-07-13T08:01:13Z

@RonEld: It seems that overtime, more tests have been added to memory_buffer_alloc module. However, these were never backported to mbed TLS 2.1 and 1.3. So I backported the tests and also some changes to the generate_code.pl script as the tests were not working correctly because changes to the script werent backported either. I tried to do a similar operation in mbed TLS 1.3, but there are too many changes introduced in the API meaning that it was very difficult to backport the tests, so I did not do that.

RonEld · 2017-07-13T08:03:21Z

@andresag01 Thanks for the clarification. In this case, we should have a task to write the tests for 1.3

andresag01 · 2017-07-13T08:08:17Z

@RonEld: I have created the new GitHub issue for backporting the tests to mbed TLS 1.3: #1014

simonbutcher · 2017-10-13T17:47:17Z

Ok - the backporting of additional test cases is worthwhile, but it would have been better to do it in a separate PR. PR's should be as simple as they can be, self-contained and aim to change only one feature or function, whether big or small.

However, I'm less keen on backporting test framework changes. You've done that work now, and I see little risk in it, but it's not something I would have planned.

These are mainly observations, and otherwise I approved the PR.

andresag01 · 2017-10-17T18:23:16Z

@sbutcher-arm @RonEld: Thanks for the reviews! With regards to:

I'm less keen on backporting test framework changes. You've done that work now, and I see little risk in it, but it's not something I would have planned.

I agree that backporting large infrastructure changes is, perhaps, not ideal as it increases the risk of breaking something. However, I like to backport simple changes like this one because in future PRs it is much easier to backport other changes around this area as the development and the maintenance branch has not diverged so much. Just a thought...

gilles-peskine-arm · 2017-11-24T15:01:15Z

Failure in x509parse-suite (https://jenkins-internal.mbed.com/job/mbedtls-pr/424/execution/node/205/log/). DO NOT MERGE pending investigation.

gilles-peskine-arm · 2017-11-24T17:07:39Z

library/x509_crl.c

@@ -294,7 +294,7 @@ int mbedtls_x509_crl_parse_der( mbedtls_x509_crl *chain,
    /*
     * Copy raw DER-encoded CRL
     */
-    if( ( p = mbedtls_calloc( 1, buflen ) ) == NULL )
+    if( buflen != 0 && ( ( p = mbedtls_calloc( 1, buflen ) ) == NULL ) )
        return( MBEDTLS_ERR_X509_ALLOC_FAILED );

    memcpy( p, buf, buflen );


The test suite test_suite_x509parse fails in the clang-asan build (https://jenkins-internal.mbed.com/job/mbedtls-pr/425/execution/node/192/log/). I can reproduce this locally and identified the culprit as the change above.

X509 CRL ASN1 (Incorrect first tag) ............................... /home/gilpes01/z/dev/mbedtls/gate/library/x509_crl.c:300:13: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:43:28: note: nonnull attribute specified here SUMMARY: AddressSanitizer: undefined-behavior /home/gilpes01/z/dev/mbedtls/gate/library/x509_crl.c:300:13 in

This test case calls mbedtls_x509_crl_parse with buflen = 0. This in turn calls mbedtls_x509_crl_parse_der with buflen = 0, and this PR changed the way the function behaves in this case.

It's good that we don't rely on the behavior of calloc with a size of 0, but the way this was fixed, we're now calling memcpy with a size of 0. Calling memcpy with a null pointer and a size of 0 probably works on most platforms, but it's technically undefined behavior, and Clang-Asan rejects it.

I think that memcpy(NULL, buf, 0) is poor practice even if it works, and a rejection from Asan is not good, so I am not merging this PR.

Technically this is a pre-existing bug, since we returned MBEDTLS_ERR_X509_ALLOC_FAILED instead of MBEDTLS_ERR_X509_INVALID_FORMAT on platforms where calloc(…, 0) returns NULL. But I think the best solution would be to add

if( buflen == 0 ) return( MBEDTLS_ERR_X509_INVALID_FORMAT );

near the top of this function and remove the check for buflen != 0 here.

gilles-peskine-arm

I am rejecting this PR because it causes the Asan check to fail and highlights a bug on exotic platforms. Please fix as described here or however you see fit.

andresag01 · 2017-12-06T09:44:05Z

@gilles-peskine-arm: Thanks for pointing this out. I have now fixed the issue.

gilles-peskine-arm

Approved, but with the same suggested improvement as the main PR: #778 (comment)

andresag01 · 2017-12-12T20:22:16Z

Changed the code in x509_crl.c according to the latest review comments.

gilles-peskine-arm · 2017-12-14T15:11:46Z

Automatic CI run picked an old version. Running manually: https://jenkins-internal.mbed.com/job/mbedtls-commit-tests/45/ → only failed due to known issues (timing tests on FreeBSD, compat.sh slow start), ok to merge.

gilles-peskine-arm

PR held due to a test failure after merging into mbedtls-2.1. See #778 (review)

The corner cases fixed include: * Allocating a buffer of size 0. With this change, the allocator now returns a NULL pointer in this case. Note that changes in pem.c and x509_crl.c were required to fix tests that did not work under this assumption. * Initialising the allocator with less memory than required for headers. * Fix header chain checks for uninitialised allocator.

Adds additional tests to the test suite for memory_buffer_alloc.c

Added comments to explain purpose and usage of generate_code.pl

Added to generate_code.pl: - support for per test suite helper functions - description of the structure of the files the script uses to construct the test suite file - delimiters through the source code to make the machine generated code easier to understand

Restructed test suite helper and main code to support tests suite helper functions, changed C++ comments to C-style, and made the generated source code more navigable.

andresag01 · 2018-01-23T21:23:42Z

@gilles-peskine-arm: Rebased and fixed reported issue in pkparse.c

gilles-peskine-arm

I approve the rebasing from my previous approval (pr_1009-1) to af77213 based on the output of interdiff <(git diff mbedtls-2.1...af77213) <(git diff mbedtls-2.1...pr_1009-1) which shows only benign differences.
I approve the one additional commit 133ab2c.
This fixes the build failure Fix corner case uses of memory_buffer_alloc.c #778 (review).

RonEld

Approving after the rebase and additional changes done after my last review

Mbedtls 2.28.3rc0 pr - DO NOT MERGE

andresag01 added bug mbed TLS team labels Jul 12, 2017

andresag01 requested review from yanesca, simonbutcher and RonEld July 12, 2017 11:11

RonEld reviewed Jul 12, 2017

View reviewed changes

RonEld approved these changes Jul 13, 2017

View reviewed changes

andresag01 mentioned this pull request Jul 13, 2017

mbed TLS 1.3: test_suite_memory_buffer_alloc tests not backported #1014

Closed

Patater added the needs-review Every commit must be reviewed by at least two team members, label Sep 28, 2017

simonbutcher approved these changes Oct 13, 2017

View reviewed changes

simonbutcher added needs-design-approval and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 13, 2017

gilles-peskine-arm added the runCI label Nov 23, 2017

gilles-peskine-arm reviewed Nov 24, 2017

View reviewed changes

gilles-peskine-arm requested changes Nov 24, 2017

View reviewed changes

gilles-peskine-arm added needs-work and removed runCI needs-design-approval labels Nov 24, 2017

andresag01 mentioned this pull request Dec 11, 2017

Fix corner case uses of memory_buffer_alloc.c #778

Merged

gilles-peskine-arm approved these changes Dec 11, 2017

View reviewed changes

gilles-peskine-arm added runCI needs-design-approval and removed needs-work labels Dec 11, 2017

gilles-peskine-arm approved these changes Dec 14, 2017

View reviewed changes

gilles-peskine-arm added backport-2.1 and removed backport-2.1 labels Jan 4, 2018

gilles-peskine-arm requested changes Jan 22, 2018

View reviewed changes

Andres AG and others added 12 commits January 23, 2018 21:03

Style fixes in pem, x509_crl and buf_alloc

5dc2fe7

Additional tests to test stack buffer allocator

aff7992

Adds additional tests to the test suite for memory_buffer_alloc.c

Widens test bounds on memory alloc tests

e9cfe14

Test corner case uses of memory_buffer_alloc.c

9b9ae0d

Clarified purpose and usage of generate_code.pl

e39088a

Added comments to explain purpose and usage of generate_code.pl

Refactored test suite template code

8bcd549

Restructed test suite helper and main code to support tests suite helper functions, changed C++ comments to C-style, and made the generated source code more navigable.

Fix typos and add copyright statement to generate_code.pl

60411a8

Ensure memcpy is not called with NULL and 0 args in x509 module

fb023c1

Change formatting of allocation check in x509_crl

af77213

Ensure that mbedtls_pk_parse_key() does not allocate 0 bytes

133ab2c

andresag01 force-pushed the mbedtls-2.1-iotssl-1033-memory-buffer-alloc branch from ede3f1b to 133ab2c Compare January 23, 2018 21:22

gilles-peskine-arm approved these changes Mar 9, 2018

View reviewed changes

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-design-approval runCI labels Mar 9, 2018

RonEld approved these changes Mar 11, 2018

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 11, 2018

gilles-peskine-arm merged commit 133ab2c into Mbed-TLS:mbedtls-2.1 Mar 13, 2018

paul-elliott-arm added a commit that referenced this pull request Mar 28, 2023

Merge pull request #1009 from paul-elliott-arm/mbedtls-2.28.3rc0-pr

981743d

Mbedtls 2.28.3rc0 pr - DO NOT MERGE

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport 2.1: Fix corner case uses of memory_buffer_alloc.c #1009

Backport 2.1: Fix corner case uses of memory_buffer_alloc.c #1009

andresag01 commented Jul 12, 2017

RonEld left a comment

andresag01 commented Jul 13, 2017

RonEld commented Jul 13, 2017

andresag01 commented Jul 13, 2017

simonbutcher commented Oct 13, 2017

andresag01 commented Oct 17, 2017 •

edited

Loading

gilles-peskine-arm commented Nov 24, 2017

gilles-peskine-arm Nov 24, 2017 •

edited

Loading

gilles-peskine-arm left a comment

andresag01 commented Dec 6, 2017

gilles-peskine-arm left a comment

andresag01 commented Dec 12, 2017

gilles-peskine-arm commented Dec 14, 2017 •

edited

Loading

gilles-peskine-arm left a comment

andresag01 commented Jan 23, 2018

gilles-peskine-arm left a comment

RonEld left a comment

Backport 2.1: Fix corner case uses of memory_buffer_alloc.c #1009

Backport 2.1: Fix corner case uses of memory_buffer_alloc.c #1009

Conversation

andresag01 commented Jul 12, 2017

RonEld left a comment

Choose a reason for hiding this comment

andresag01 commented Jul 13, 2017

RonEld commented Jul 13, 2017

andresag01 commented Jul 13, 2017

simonbutcher commented Oct 13, 2017

andresag01 commented Oct 17, 2017 • edited Loading

gilles-peskine-arm commented Nov 24, 2017

gilles-peskine-arm Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

andresag01 commented Dec 6, 2017

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

andresag01 commented Dec 12, 2017

gilles-peskine-arm commented Dec 14, 2017 • edited Loading

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

andresag01 commented Jan 23, 2018

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

RonEld left a comment

Choose a reason for hiding this comment

andresag01 commented Oct 17, 2017 •

edited

Loading

gilles-peskine-arm Nov 24, 2017 •

edited

Loading

gilles-peskine-arm commented Dec 14, 2017 •

edited

Loading