Fix corner case uses of memory_buffer_alloc.c #778

andresag01 · 2017-01-30T14:59:16Z

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.
The changes need to be backported to 2.7 (use this PR) and 2.1 (Backport 2.1: Fix corner case uses of memory_buffer_alloc.c #1009)

andresag01 · 2017-01-30T15:15:40Z

library/memory_buffer_alloc.c

@@ -247,6 +249,8 @@ static void *buffer_alloc_calloc( size_t n, size_t size )

    if( n != 0 && len / n != size )


In #639 it was proposed that if the size of the requested allocation is 0 then the allocator should return NULL. The C99 standard states the following:

If the size of the space requested is zero, the behavior is implementation- defined: either a null pointer is returned, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object.

The problem is that some parts of the library assume that allocating 0 will return a non-NULL value on success. Making this change causes test failures, so the behaviour is not changed as it is compliant with the C standard. Nevertheless, I went through the code, and it seems that this allocator actually tries to allocate a 0 length buffer, which is completely unnecessary. I think this behaviour should be modified. Any thoughts @sbutcher-arm and @yanesca?

@andresag01 In my opinion, we should treat 0 size allocation as NULL, and fix the code where it is treated different

@andresag01 - I read the bug differently. That's because calloc() can return NULL or non-NULL values, we need to be able to handle both because we can't always depend on the underlying implementation to do what we expect. If, as you say, "some parts of the library assume that allocating 0 will return a non-NULL value", surely we need to fix this to accommodate both types of behaviour?

@andresag01 - I think you'd best raise a new bug to address other parts of the library.

I also think we need to treat a size 0 allocation as NULL.

simonbutcher · 2017-02-05T00:44:33Z

library/memory_buffer_alloc.c

@@ -247,6 +249,8 @@ static void *buffer_alloc_calloc( size_t n, size_t size )

    if( n != 0 && len / n != size )


@andresag01 - I read the bug differently. That's because calloc() can return NULL or non-NULL values, we need to be able to handle both because we can't always depend on the underlying implementation to do what we expect. If, as you say, "some parts of the library assume that allocating 0 will return a non-NULL value", surely we need to fix this to accommodate both types of behaviour?

@andresag01 - I think you'd best raise a new bug to address other parts of the library.

I also think we need to treat a size 0 allocation as NULL.

andresag01 · 2017-02-06T12:15:06Z

@RonEld, @sbutcher-arm: I have modified the allocator to return NULL when allocating 0 bytes. And fixed two places in the library (pem.c and x509_crl.c) that worked under the assumption that allocating 0 returns a non-NULL value.

andresag01 · 2017-07-05T10:21:00Z

@RonEld: Could you take a look a this again?

RonEld · 2017-07-05T11:03:05Z

library/memory_buffer_alloc.c


-    if( verify_header( heap.first ) != 0 )
+    if( heap.first == NULL || verify_header( heap.first ) != 0 )


although same as previous implementation, but I'll reuse prv variable, instead of using heap.first

RonEld · 2017-07-05T12:08:09Z

library/pem.c

@@ -410,7 +410,7 @@ int mbedtls_pem_write_buffer( const char *header, const char *footer,
        return( MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL );
    }

-    if( ( encode_buf = mbedtls_calloc( 1, use_len ) ) == NULL )
+    if( use_len != 0 && ( encode_buf = mbedtls_calloc( 1, use_len ) ) == NULL )


to make it more readable, please surround the statement ( encode_buf = mbedtls_calloc( 1, use_len ) ) == NULL with parentheses

RonEld · 2017-07-05T12:09:23Z

library/x509_crl.c

@@ -257,7 +257,7 @@ int mbedtls_x509_crl_parse_der( mbedtls_x509_crl *chain,
 {
    int ret;
    size_t len;
-    unsigned char *p, *end;
+    unsigned char *p = NULL, *end;


I would also initialize end

RonEld · 2017-07-05T12:09:55Z

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 )


to make it more readable, please surround the statement ( p = mbedtls_calloc( 1, buflen ) ) == NULL with parentheses

RonEld · 2017-07-05T12:12:46Z

library/memory_buffer_alloc.c

@@ -581,20 +584,24 @@ void mbedtls_memory_buffer_alloc_init( unsigned char *buf, size_t len )
    mbedtls_platform_set_calloc_free( buffer_alloc_calloc, buffer_alloc_free );
 #endif

-    if( (size_t) buf % MBEDTLS_MEMORY_ALIGN_MULTIPLE )
+    if( len < sizeof( memory_header ) + MBEDTLS_MEMORY_ALIGN_MULTIPLE )


Is this the correct behavior? This function doesn't have a return code, but eventually, this is an error case. A user calling this function doesn't know that the input is incorrect

I think this is the correct behaviour. The buffer provided to memory manage must be at least as high as the memory header. If this is not the case, the internal buffer_alloc_ctx will be zeroized before returning and further calls to, say mbedtls_calloc() will signal allocation failed by returning a NULL pointer.

ok thanks, I think you mean buffer_alloc_calloc() though

andresag01 · 2017-07-06T09:15:23Z

@RonEld, @sbutcher-arm: I have reworked the PR to address comments. Please review.

gilles-peskine-arm · 2017-11-24T14:57:15Z

CI failed only due to timing tests (known issue), ok to merge.

gilles-peskine-arm

I am rejecting this PR for the same reason as #1009. In the 2.1 and 1.3 backports, ASan highlighted that we call memcpy(NULL, …, 0) which is UB. For some reason ASan doesn't complain here, but the problem is present as well.

gilles-peskine-arm · 2017-11-24T17:12:49Z

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 ) )


The check for buflen == 0 should be made earlier. See https://github.com/ARMmbed/mbedtls/pull/1009/files#r153007520

andresag01 · 2017-12-06T09:45:38Z

@gilles-peskine-arm: I have fixed the issue.

gilles-peskine-arm

Thanks for updating. The behavior is ok but I would prefer a different way to write the code for clarity.

gilles-peskine-arm · 2017-12-11T09:51:54Z

library/x509_crl.c

-    if( buflen != 0 && ( ( p = mbedtls_calloc( 1, buflen ) ) == NULL ) )
+    if( buflen == 0 )
+        return( MBEDTLS_ERR_X509_INVALID_FORMAT );
+    else if( ( p = mbedtls_calloc( 1, buflen ) ) == NULL )


It doesn't change the behavior, but I'd prefer not to put an else here, because it's misleading for readers: if buflen==0, the code below the else is no executed either. And since the result of the allocation is stored in a variable, there's no call for putting the assignment inside the if, so I'd prefer

if( buflen == 0 ) return( MBEDTLS_ERR_X509_INVALID_FORMAT ); p = mbedtls_calloc( 1, buflen ); if( ( p == NULL ) return( MBEDTLS_ERR_X509_ALLOC_FAILED );

Ok, I think this is a really minor thing about coding style. I prefer the way I originally wrote it, but I changed the code to suit your preferred style in the latest commit.

gilles-peskine-arm · 2017-12-14T15:06:39Z

CI failed only due to timing tests (known issue), ok to merge.

gilles-peskine-arm

After merging this PR with development, there is a test failure.

scripts/config.pl full
make clean; make
cd tests
./test_suite_pkparse -v

One test case fails:

Key ASN1 (Incorrect first tag) .................................... FAILED
  mbedtls_pk_parse_key( &pk, buf, data_len, NULL, 0 ) == ( result )
  at line 132, suites/test_suite_pkparse.function

To make this PR mergeable, please merge development into it and fix this test failure.

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.

andresag01 · 2018-01-23T20:01:41Z

@gilles-peskine-arm: Thanks for letting me know. The problem is yet another piece of code in the library attempting to allocate a 0-length buffer. I fixed it and will push the changes soon. I have also rebased the PR and run all.sh locally before passing it back to you.

andresag01 · 2018-01-23T20:49:41Z

@gilles-peskine-arm: I have rebased on top of development and fixed the issue you raised. I will now update the backports

gilles-peskine-arm

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

andresag01 · 2018-03-09T14:25:54Z

@gilles-peskine-arm: Thanks for the review. I will do the backport to 2.7 as soon as I can.

gilles-peskine-arm · 2018-03-09T14:27:32Z

@andresag01 Since this PR forked before 2.7.0, you don't need to make a 2.7 backport: the same commit can be merged into 2.7 and development. We'll merge this PR into both mbedtls-2.7 and development.

andresag01 · 2018-03-09T14:28:57Z

@gilles-peskine-arm: Alright thanks. I will not do the backport then.

RonEld

Approving after the backport and additional changes

andresag01 added bug mbed TLS team labels Jan 30, 2017

andresag01 commented Jan 30, 2017

View reviewed changes

simonbutcher suggested changes Feb 5, 2017

View reviewed changes

andresag01 force-pushed the iotssl-1033-memory-buffer-alloc branch from ebd3a96 to ec683cd Compare February 6, 2017 12:12

RonEld suggested changes Jul 5, 2017

View reviewed changes

RonEld approved these changes Jul 11, 2017

View reviewed changes

andresag01 requested a review from yanesca July 12, 2017 10:37

This was referenced Jul 12, 2017

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

Merged

Backport 1.3: Fix corner case uses of memory_buffer_alloc.c #1010

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

simonbutcher added approved Design and code approved - may be waiting for CI or backports and removed needs-design-approval labels Nov 24, 2017

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed approved Design and code approved - may be waiting for CI or backports labels Nov 24, 2017

gilles-peskine-arm requested changes Nov 24, 2017

View reviewed changes

gilles-peskine-arm added needs-work and removed approved Design and code approved - may be waiting for CI or backports runCI labels Nov 24, 2017

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

View reviewed changes

gilles-peskine-arm added the runCI label Dec 11, 2017

gilles-peskine-arm added 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 requested changes Jan 22, 2018

View reviewed changes

Andres AG and others added 5 commits January 23, 2018 19:37

Test corner case uses of memory_buffer_alloc.c

8ec3bfe

Style fixes in pem, x509_crl and buf_alloc

f1ee635

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

cb5123f

Change formatting of allocation check in x509_crl

c9d6226

Ensure that mbedtls_pk_parse_key() does not allocate 0 bytes

e9124b9

andresag01 force-pushed the iotssl-1033-memory-buffer-alloc branch from 3d06b73 to e9124b9 Compare January 23, 2018 20:47

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 e9124b9 into Mbed-TLS:development Mar 13, 2018

gilles-peskine-arm mentioned this pull request Mar 13, 2018

memory_buffer_alloc.c corner cases and fixes #639

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix corner case uses of memory_buffer_alloc.c #778

Fix corner case uses of memory_buffer_alloc.c #778

andresag01 commented Jan 30, 2017 •

edited by gilles-peskine-arm

Loading

andresag01 Jan 30, 2017 •

edited

Loading

RonEld Jan 31, 2017

simonbutcher Feb 5, 2017

simonbutcher Feb 5, 2017

andresag01 commented Feb 6, 2017

andresag01 commented Jul 5, 2017

RonEld Jul 5, 2017

RonEld Jul 5, 2017

RonEld Jul 5, 2017

RonEld Jul 5, 2017

RonEld Jul 5, 2017

andresag01 Jul 6, 2017

RonEld Jul 11, 2017

andresag01 commented Jul 6, 2017

gilles-peskine-arm commented Nov 24, 2017

gilles-peskine-arm left a comment

gilles-peskine-arm Nov 24, 2017

andresag01 commented Dec 6, 2017

gilles-peskine-arm left a comment

gilles-peskine-arm Dec 11, 2017

andresag01 Dec 12, 2017

gilles-peskine-arm commented Dec 14, 2017

gilles-peskine-arm left a comment

andresag01 commented Jan 23, 2018

andresag01 commented Jan 23, 2018

gilles-peskine-arm left a comment

andresag01 commented Mar 9, 2018

gilles-peskine-arm commented Mar 9, 2018

andresag01 commented Mar 9, 2018

RonEld left a comment

		@@ -247,6 +249,8 @@ static void *buffer_alloc_calloc( size_t n, size_t size )

		if( n != 0 && len / n != size )


		if( verify_header( heap.first ) != 0 )
		if( heap.first == NULL \|\| verify_header( heap.first ) != 0 )

Fix corner case uses of memory_buffer_alloc.c #778

Fix corner case uses of memory_buffer_alloc.c #778

Conversation

andresag01 commented Jan 30, 2017 • edited by gilles-peskine-arm Loading

andresag01 Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 commented Feb 6, 2017

andresag01 commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 commented Jul 6, 2017

gilles-peskine-arm commented Nov 24, 2017

gilles-peskine-arm left a comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Dec 14, 2017

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

andresag01 commented Jan 23, 2018

andresag01 commented Jan 23, 2018

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

andresag01 commented Mar 9, 2018

gilles-peskine-arm commented Mar 9, 2018

andresag01 commented Mar 9, 2018

RonEld left a comment

Choose a reason for hiding this comment

andresag01 commented Jan 30, 2017 •

edited by gilles-peskine-arm

Loading

andresag01 Jan 30, 2017 •

edited

Loading