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

mpi_write_hlp buffer overflow #2404

Closed
guidovranken opened this issue Feb 1, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@guidovranken
Copy link

commented Feb 1, 2019

Google oss-fuzz found a buffer overflow in mpi_write_hlp. This is most likely due to 91af329#diff-c252711ea8b2703e393502bfe46bff62

The overflow occurs at library/bignum.c:553.

See: https://oss-fuzz.com/testcase-detail/5167698789531648

@hanno-arm

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

Hi @guidovranken,

thanks for your report! However, I'm lacking access to the page you linked. Could you please provide more details here? So far, I don't see how the loop in 91af329#diff-c252711ea8b2703e393502bfe46bff62 would lead to an overflow.

Thanks!
Hanno

@guidovranken

This comment has been minimized.

Copy link
Author

commented Feb 1, 2019

Hi Hanno,

your team should have access to the ClusterFuzz interface via support-mbedtls@arm.com, see also: #2038

It happens with the number is negative:

Compile mbed tls and the following file with address sanitizer (-fsanitize=address) to verify.

#include <mbedtls/bignum.h>
#include <stdlib.h>

static int bignum_from_string(const char* input, mbedtls_mpi** output)
{
    mbedtls_mpi* mpi = NULL;

    *output = NULL;

    if ( (mpi = malloc(sizeof(*mpi))) == NULL ) {
        goto error;
    }

    mbedtls_mpi_init(mpi);

    if ( mbedtls_mpi_read_string(mpi, 10, input) != 0 ) {
        goto error;
    }

    *output = mpi;
    return 0;

error:
    free(mpi);

    return -1;
}

static int string_from_bignum(mbedtls_mpi* mpi, char** output)
{
    size_t olen;
    int ret;

    *output = NULL;

    ret = mbedtls_mpi_write_string(mpi, 10, NULL, 0, &olen);
    if ( ret != MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL ) {
        goto error;
    }

    if ( (*output = malloc(olen)) == NULL ) {
        goto error;
    }

    if ( mbedtls_mpi_write_string(mpi, 10, *output, olen, &olen) != 0 ) {
        goto error;
    }

    return 0;
error:
    free(*output);
    *output = NULL;
    return -1;
}

int main(void)
{
    const char* instr = "-1";
    mbedtls_mpi* outmpi;
    char* outstr;
    if ( bignum_from_string(instr, &outmpi) == -1 ) {
        return 0;
    }
    if ( string_from_bignum(outmpi, &outstr) == 0 ) {
        free(outstr);
    }
    mbedtls_mpi_free(outmpi);
    return 0;
}

@guidovranken guidovranken reopened this Feb 1, 2019

@hanno-arm

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

Hi @guidovranken,

thanks, that clarifies things. The bug is not in mpi_write_hlp(), it's in the caller mbedtls_mpi_write_string():

mbedtls/library/bignum.c

Lines 604 to 605 in f352f75

if( X->s == -1 )
*p++ = '-';

This increases the output pointer but doesn't decrease buflen. Luckily, it only occurs for negative numbers, and for those it's questionable why they are supported in the first place...

I'll open a PR with the fix.

Thanks again for your report,
Hanno

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

ARM Internal Ref: IOTSSL-2760

@ciarmcom ciarmcom added the mirrored label Feb 1, 2019

@guidovranken

This comment has been minimized.

Copy link
Author

commented Mar 1, 2019

When will you merge the fix?

yanesca added a commit to yanesca/mbedtls that referenced this issue Mar 6, 2019

Fix 1-byte buffer overflow in mbedtls_mpi_write_string()
This can only occur for negative numbers. Fixes ARMmbed#2404.

yanesca added a commit to yanesca/mbedtls that referenced this issue Mar 6, 2019

Fix 1-byte buffer overflow in mbedtls_mpi_write_string()
This can only occur for negative numbers. Fixes ARMmbed#2404.

@Patater Patater closed this in #2405 Apr 8, 2019

Patater added a commit to Patater/mbedtls that referenced this issue Apr 8, 2019

Give credit to OSS-Fuzz for ARMmbed#2404
Add "Credit to OSS-Fuzz", in addition to Guido Vranken, for identifying
bug ARMmbed#2404.

Patater added a commit to Patater/mbedtls that referenced this issue Apr 8, 2019

Give credit to OSS-Fuzz for ARMmbed#2404
Add "Credit to OSS-Fuzz", in addition to Guido Vranken, for identifying
bug ARMmbed#2404.

Patater added a commit to Patater/mbedtls that referenced this issue Apr 8, 2019

Give credit to OSS-Fuzz for ARMmbed#2404
Add "Credit to OSS-Fuzz", in addition to Guido Vranken, for identifying
bug ARMmbed#2404.

Patater added a commit that referenced this issue Apr 16, 2019

Merge remote-tracking branch 'origin/pr/2555' into development
* origin/pr/2555:
  Give credit to OSS-Fuzz for #2404

Patater added a commit that referenced this issue Apr 16, 2019

Merge remote-tracking branch 'origin/pr/2556' into mbedtls-2.7
* origin/pr/2556:
  Give credit to OSS-Fuzz for #2404

Patater added a commit that referenced this issue Apr 16, 2019

Merge remote-tracking branch 'origin/pr/2557' into mbedtls-2.16
* origin/pr/2557:
  Give credit to OSS-Fuzz for #2404

Patater added a commit to Patater/mbedtls that referenced this issue Apr 17, 2019

Merge remote-tracking branch 'tls/development' into development
Merge Mbed TLS at f790a6c into Mbed Crypto.

Resolve conflicts by performing the following:
    - Reject changes to README.md
    - Don't add crypto as a submodule
    - Remove test/ssl_cert_test from programs/Makefile
    - Add cipher.nist_kw test to tests/CMakeLists.txt
    - Reject removal of crypto-specific all.sh tests
    - Reject update to SSL-specific portion of component_test_valgrind
      in all.sh
    - Reject addition of ssl-opt.sh testing to component_test_m32_o1 in
      all.sh

* tls/development: (87 commits)
  Call mbedtls_cipher_free() to reset a cipher context
  Don't call mbedtls_cipher_setkey twice
  Update crypto submodule
  Minor fixes in get certificate policies oid test
  Add certificate policy oid x509 extension
  cpp_dummy_build: Add missing header psa_util.h
  Clarify comment mangled by an earlier refactoring
  Add an "out-of-box" component
  Run ssl-opt.sh on 32-bit runtime
  Don't use debug level 1 for informational messages
  Skip uncritical unsupported extensions
  Give credit to OSS-Fuzz for ARMmbed#2404
  all.sh: remove component_test_new_ecdh_context
  Remove crypto-only related components from all.sh
  Remove ssl_cert_test sample app
  Make CRT callback tests more robust
  Rename constant in client2.c
  Document and test flags in x509_verify
  Fix style issues and a typo
  Fix a rebase error
  ...

sbutcher-arm added a commit to sbutcher-arm/mbedtls that referenced this issue Apr 24, 2019

Merge remote-tracking branch 'public/mbedtls-2.16' into baremetal
* public/mbedtls-2.16: (40 commits)
  Clarify comment mangled by an earlier refactoring
  Add an "out-of-box" component
  Run ssl-opt.sh on 32-bit runtime
  Fix typo in data_file generator code
  Give credit to OSS-Fuzz for ARMmbed#2404
  Remove ssl_cert_test sample app
  Fix the proxy seed in Travis runs
  Update library version to 2.16.1
  Fix errors in AEAD test function
  x509.c: Fix potential memory leak in X.509 self test
  Remove Circle CI script
  Fix ChangeLog entry ordering
  Fix typo
  Add non-regression test for buffer overflow
  Improve documentation of mbedtls_mpi_write_string()
  Adapt ChangeLog
  Fix 1-byte buffer overflow in mbedtls_mpi_write_string()
  Change Perl to Python in test builds
  Fix default port number information
  Silence pylint
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.