igrr / axtls-8266

axTLS port for ESP8266
Other
79 stars 33 forks source link

Store the bytes needed for the HMAC calculation before the buffer res… #65

Closed slaff closed 4 years ago

slaff commented 4 years ago

…izing.

Otherwise it may result in invalid memory read because buf may point to freed/invalid memory address if the original buffer was moved from realloc to another address.

In the beginning of basic_read buf is set to point to the current memory address stored in ssl->bm_data

int basic_read(SSL *ssl, uint8_t **in_data)
{
    int ret = SSL_OK;
    int read_len, is_client = IS_SET_SSL_FLAG(SSL_IS_CLIENT);
    uint8_t *buf = ssl->bm_data;

further in the same function more memory might be requested, as shown below.

        if (ssl->need_bytes > ssl->max_plain_length+RT_EXTRA-BM_RECORD_OFFSET)
        {
            printf("ssl->need_bytes=%d > %d\r\n", ssl->need_bytes, ssl->max_plain_length+RT_EXTRA-BM_RECORD_OFFSET);
            ret = increase_bm_data_size(ssl, ssl->need_bytes + BM_RECORD_OFFSET - RT_EXTRA);

In the increase_bm_date_size function, the memory allocated in the ssl->bm_data may be reallocated, as shown below:

int increase_bm_data_size(SSL *ssl, size_t size)
{
    if (ssl->max_plain_length == RT_MAX_PLAIN_LENGTH) {
        return SSL_OK;
    }
    if (ssl->can_free_certificates) {
        certificate_free(ssl);
    }
    size_t required = (size + 1023) & ~(1023); // round up to 1k
    required = (required < RT_MAX_PLAIN_LENGTH) ? required : RT_MAX_PLAIN_LENGTH;
    uint8_t* new_bm_all_data = (uint8_t*) realloc(ssl->bm_all_data, required + RT_EXTRA);

thus the original buf pointer will start pointing to a freed or modified block of data. Which will result in bad HMAC and/or memory corruption.

This PR fixes this.

slaff commented 4 years ago

@igrr Can you take a look at this PR?

igrr commented 4 years ago

Thanks!