Fix memory leak in mbedtls_x509_csr_parse

[catenacyber]

Description

Fix memory leak in mbedtls_x509_csr_parse

Status

READY

Requires Backporting

Yes

Which branch? I did not check

Migrations

No

Additional comments

Found using oss-fuzz

Todos

• Tests
• Documentation
• Changelog updated
• Backported

Steps to test or reproduce

Allocated memory in previous call to mbedtls_pem_read_buffer does not get freed without this patch

[catenacyber]

CLA has just been submitted through mail

[RonEld]

@catenacyber Thank you for your contribution! As sent in mail, the CLA is being proccessed

[RonEld]

@catenacyber I have just raised on your behalf bug #1623 for tracking purposes

[irwir]

Could this be written in a shorter way?

#if defined(MBEDTLS_PEM_PARSE_C)
    /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */
    if( buf[buflen - 1] == '\0' )
    {
        mbedtls_pem_init( &pem );
        ret = mbedtls_pem_read_buffer( &pem,
                               "-----BEGIN CERTIFICATE REQUEST-----",
                               "-----END CERTIFICATE REQUEST-----",
                               buf, NULL, 0, &use_len );

        if( ret == 0 )
            /*
             * Was PEM encoded, parse the result
             */
            ret = mbedtls_x509_csr_parse_der( csr, pem.buf, pem.buflen );
        mbedtls_pem_free( &pem );
        if( ret != MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT )
            return( ret );
    }
#endif /* MBEDTLS_PEM_PARSE_C */

[simonbutcher]

Needs backporting to mbedtls-2.7 and mbedtls-2.1.

[andresag01]

@catenacyber Thank you for your contribution.

The changes look good to me. @catenacyber and @sbutcher-arm, I was looking at the surrounding lines of code when reviewing this changes and I think I spotted another issue. Do you think the following sequence could cause a leak? (line numbers on patched version presented in this PR)

• pem context is initialised in lines 281
• the value of ret after line 290 satisfies the following condition ret != 0 && ret == MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT
• Execution reaches the else in line 306
• The return statement in line 308 is executed without calling mbedtls_pem_free( &pem )

I might be missing something though...

[irwir]

@andresag01

the value of ret after line 290 satisfies the following condition ret != 0 && ret == MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT

Function call in the line 287 could return MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT, therefore memory leak is possible. The issue existed since polarssl days.

Refactored code in this message should never leak memory. If necessary, a new PR could be created.

Edit.
Non-issue, as explained below by @catenacyber; only a possibility.
Though it is very natural to think that 'init' and 'free' kind of functions should always be paired.

[catenacyber]

@andresag01
There is no such leak (for now) :

pem context is initialised in lines 281

There is no allocation, just a memset
The allocation take place in mbedtls_pem_read_buffer but only after we are sure we will not return MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT

[catenacyber]

The code proposed by @irwir looks good to me.

This pull request was the minimum change to fix the leak.
I let you decide if you want refactoring in the same or another pull request

[andresag01]

@catenacyber: Agreed that currently there is no leak, but as @irwir mentioned calls to init and free should be paired. Ideally the execution path that I pointed out would also be fixed to conform with that, but at the moment it is not harmful.

The changes that @catenacyber look good to me to fix the memory leak. So I am happy for them to be merged as is. However, there are other issues with the code. I also think this code is slightly more complicated that it needs to be and a simple rewrite of those few lines (along the lines of @irwir proposed) is ideal or at least fixed. @sbutcher-arm I will defer the decision to you regarding whether this is merged as is.

[simonbutcher]

Hi @catenacyber,

I'd like to go with @andresag01's suggestion, but I'm conscious that this is your PR. You can either fix it yourself, and resubmit, or if you're having more fun finding the problems than fixing them, @andresag01 can do the re-work. Either way, we will credit you for the work you did here!

Please let us know.

[catenacyber]

Here you go with one more commit to simplify the code.
I let you review it

[andresag01]

@catenacyber: Thanks for reworking the PR. The changes look good to me.

@sbutcher-arm: Could you please review the PR once again? Please note that a ChangeLog entry and backports to versions 2.1 and 2.7 are also needed.

[andresag01]

Please note that the Mbed TLS coding style is to place the { on the next line.

[andresag01]

@catenacyber: Thanks for fixing the coding style.

[simonbutcher]

I've backported this to mbedtls-2.7 with #1772, and mbedtls-2.1 with #1174.

Once those are approved, this can move to 'ready for merge'.