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

Out of range memory access in MbedOS CoAP library parser #12925

Closed
mjurczak opened this issue May 5, 2020 · 3 comments
Closed

Out of range memory access in MbedOS CoAP library parser #12925

mjurczak opened this issue May 5, 2020 · 3 comments

Comments

@mjurczak
Copy link

mjurczak commented May 5, 2020

Description of defect

References:

https://github.com/ARMmbed/mbed-os/tree/mbed-os-5.15.3/features/frameworks/mbed-coap

https://github.com/ARMmbed/mbed-coap/tree/v5.1.5

File:

sn_coap_parser.c

Example Trace:
    #0  sn_coap_parser_options_parse_uint()
        sn_coap_parser.c:257
    #1  sn_coap_parser_options_parse()
        sn_coap_parser.c:531
    #2  sn_coap_parser()
        sn_coap_parser.c:161

Analysis:

If a packet with option delta lower than 13 is parsed, the remaning message length is incorrectly calculated in the line:

message_left = packet_len - ((*packet_data_pptr) - packet_data_start_ptr);

The packet data pointer is not incremented prior to the remaining message length calculation and therefore it does not account for the option byte. This allows a malformed message with not option value following to pass through message length check in the line:

if (message_left == 0){
/* packet_data_pptr would overflow! */
tr_error("sn_coap_parser_options_parse - **packet_data_pptr overflow !");
return -1;
}

For short options length the remaining message length is unchanged after option lengh processing:

message_left = packet_len - (*packet_data_pptr - packet_data_start_ptr);

Allowing to pass throught final message length check:

if (message_left < option_len){
/* packet_data_pptr would overflow! */
tr_error("sn_coap_parser_options_parse - **packet_data_pptr would overflow when parsing options!");
return -1;
}

In option processing code the packet data pointer is incremented beyond the input buffer boundary and passed for further processing, e.g. in:

The invalid pointer is then accessed by functions called from option processing code, e.g. in:

value |= *(*packet_data_pptr)++;

Type:

  • Integer Overflow or Wraparound
  • Out-of-bounds Read

Result:

  • Parsing data out of input bounds
  • Possible crash due out-of-bound memory access

Target(s) affected by this defect ?

  • MbedOS mbed-coap library 5.1.5
  • MbedOS 5.15.3

Toolchain(s) (name and version) displaying this defect ?

N/A

What version of Mbed-os are you using (tag or sha) ?

MbedOS 5.15.3

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

N/A

How is this defect reproduced ?

Parsing the provided input example input with sn_coap_parser() function.

#include <stdint.h>
#include <stdlib.h>
#include <stdio.h>
#include "sn_coap_protocol.h"
#include "sn_coap_header.h"

struct coap_s* coapHandle;
coap_version_e coapVersion = COAP_VERSION_1;

void* coap_malloc(uint16_t size){
    return malloc(size);
}

void coap_free(void* addr){
    free(addr);
}

uint8_t coap_tx_cb(uint8_t *arg_a, uint16_t arg_b, sn_nsdl_addr_s *arg_c, void *arg_d){
    return 0;
}

int8_t coap_rx_cb(sn_coap_hdr_s *arg_a, sn_nsdl_addr_s *arg_b, void *arg_c){
    return 0;
}

int main(int argc, const char* argv[])
{
    FILE *fp;
    size_t read_bytes;
    size_t input_size;
    uint8_t *message_buffer;
    if (argc != 2)
    {
       return 1;
    }
    
    fp = fopen(argv[1], "r");
    if (fp == NULL)
    {
      return 2;
    }
     
    fseek (fp , 0 , SEEK_END);
    input_size = ftell(fp);
    rewind (fp);
     
    if (input_size > 65527)
    {
      return 3;
    }
     
    message_buffer = malloc(input_size);
    read_bytes = fread(message_buffer, 1, input_size, fp);
    fclose(fp);
    coapHandle = sn_coap_protocol_init(&coap_malloc, &coap_free, &coap_tx_cb, &coap_rx_cb);
    sn_coap_hdr_s* parsed = sn_coap_parser(coapHandle, read_bytes, message_buffer, &coapVersion);
    sn_coap_parser_release_allocated_coap_msg_mem(coapHandle, parsed);
    free(message_buffer);
    
    return 0;
}

sn_coap_parser.c:257__read_buffer_overflow.log

@mjurczak
Copy link
Author

mjurczak commented May 5, 2020

As a general advice, a single function to increment the current packet data buffer with boundary check (under/over-flow) is recommended.

A structure with incoming packet buffer boundaries (start pointer, length) and current position could then be passed around in the parser. Each function willing to increment the current pointer would be required to do it by calling a function that increments the pointer and validates it.

@ciarmcom
Copy link
Member

ciarmcom commented May 6, 2020

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers.
Internal Jira reference: https://jira.arm.com/browse/MBOTRIAGE-2664

@mjurczak
Copy link
Author

mjurczak commented May 8, 2020

#12928 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants