NULL dereference inside BLE_Beacon examples when invoking hci_le_set_scan_response_data(...)

[tim-nordell-nimbelink]

API being invoked that does the dereference:

$ git grep hci_le_set_scan_response_data Projects/
Projects/P-NUCLEO-WB55.Nucleo/Applications/BLE/BLE_Beacon/STM32_WPAN/App/eddystone_tlm_service.c:  hci_le_set_scan_response_data(0, NULL);
Projects/P-NUCLEO-WB55.Nucleo/Applications/BLE/BLE_Beacon/STM32_WPAN/App/eddystone_uid_service.c:  hci_le_set_scan_response_data(0, NULL);
Projects/P-NUCLEO-WB55.Nucleo/Applications/BLE/BLE_Beacon/STM32_WPAN/App/eddystone_url_service.c:  hci_le_set_scan_response_data(0, NULL);
Projects/P-NUCLEO-WB55.Nucleo/Applications/BLE/BLE_Beacon/STM32_WPAN/App/ibeacon_service.c:  hci_le_set_scan_response_data(0, NULL);

hci_le_set_scan_response_data(...) implementation:

tBleStatus hci_le_set_scan_response_data( uint8_t Scan_Response_Data_Length,
                                          const uint8_t* Scan_Response_Data )
{
  struct hci_request rq;
  uint8_t cmd_buffer[BLE_CMD_MAX_PARAM_LEN];
  hci_le_set_scan_response_data_cp0 *cp0 = (hci_le_set_scan_response_data_cp0*)(cmd_buffer);
  tBleStatus status = 0;
  int index_input = 0;
  cp0->Scan_Response_Data_Length = Scan_Response_Data_Length;
  index_input += 1;
  Osal_MemCpy( (void*)&cp0->Scan_Response_Data, (const void*)Scan_Response_Data, 31 );
  index_input += 31;
  Osal_MemSet( &rq, 0, sizeof(rq) );
  rq.ogf = 0x08;
  rq.ocf = 0x009;
  rq.cparam = cmd_buffer;
  rq.clen = index_input;
  rq.rparam = &status;
  rq.rlen = 1;
  if ( hci_send_req(&rq, FALSE) < 0 )
    return BLE_STATUS_TIMEOUT;
  return status;
}

You can see Osal_MemCpy(...) is invoked with the passed in data structure even when it's NULL, such as in the BLE_Beacon example code. The hci_le_set_scan_response_data(...) implementation probably should do:

  Osal_MemSet( (void*)&cp0->Scan_Response_Data, 31);
  Osal_MemCpy( (void*)&cp0->Scan_Response_Data, (const void*)Scan_Response_Data, Scan_Response_Data_Length <= 31 ? Scan_Response_Data_Length : 31);

instead of assuming it can read 31 bytes from the source. The function implementation as-is could cause a bus fault, too, depending on where the source data is located in memory if it was less than 31 bytes originally for someone who isn't adding a NULL/sized 0 data entry.

[ASELSTM]

Hi @tim-nordell-nimbelink,

Thank you for this report. It has been forwarded to our development teams. We will get back to you as soon as they provide us with their feedback.

With regards,

[EZIMM]

The same issue is seen in hci_le_set_advertising_data(...)

[ASELSTM]

ST Internal Reference: 110621

[ASELSTM]

Hi @tim-nordell-nimbelink,

It is actually specified in BLE Wireless interface application note that the size is fixed and must be kept equal to 31.

Unfortunately your request can't be addressed, please allow me then to close this thread.

With regards,

[EZIMM]

Hi @ALABSTM, I understand it is this way in the application note, but just curious why ST would not add the extra protection regardless since the input data to the function is Osal_MemCpy'd to a local cmd_buffer array anyways? And we must pass in the Scan_Response_Data_Length as an argument to the hci_le_set_scan_response_data function. Couldn't the function fill the buffer with "insignificant octets" (0's) and copy Scan_Response_Data_Length "significant octets"? I would consider this an quality-of-life enhancement. Regards,

[tim-nordell-nimbelink]

Hi @ALABSTM -

Note that there were two issues here really:

1. A NULL passed dereference in the example code. If the documentation specifies that it must be allocated, this example code is clearly wrong since it's not allocating.
2. The function takes in data past the length passed in. If the function didn't behave this way, there wouldn't have been a NULL dereference.

Clearly you can't have it both ways - either issue 1 above is valid, or issue 2 above is valid. Issue 1 is invalid due to the current documentation for usage of the function from issue 2.

However, both @EZIMM and I agree that it's a design issue to take in a pointer and length, and not respect the length passed in. That's a poor interface and that stance can lead to security issues, and code issues that are non-obvious during a code review. A person casually reviewing the code would be "oh, it's passing in data and length", which is probably why the following all of the following examples passed through code inspection:

Projects/P-NUCLEO-WB55.Nucleo/Applications/BLE/BLE_Beacon/STM32_WPAN/App/eddystone_tlm_service.c:  hci_le_set_scan_response_data(0, NULL);
Projects/P-NUCLEO-WB55.Nucleo/Applications/BLE/BLE_Beacon/STM32_WPAN/App/eddystone_uid_service.c:  hci_le_set_scan_response_data(0, NULL);
Projects/P-NUCLEO-WB55.Nucleo/Applications/BLE/BLE_Beacon/STM32_WPAN/App/eddystone_url_service.c:  hci_le_set_scan_response_data(0, NULL);
Projects/P-NUCLEO-WB55.Nucleo/Applications/BLE/BLE_Beacon/STM32_WPAN/App/ibeacon_service.c:  hci_le_set_scan_response_data(0, NULL);

A casual inspection there would assume the NULL is okay since the size being passed in is 0, but clearly that example code is violating the current documentation.

If you really want to enforce this passed in length in C code (much easier to do in C++ code without changing the interface), you really need to have the user pass in a structure with rigid allocation, like the following in C:

struct hci_le_set_scan_response_args
{
    uint8_t scan_response_data_length;
    uint8_t scan_response_data[31];
};
tBleStatus hci_le_set_scan_response_data( const struct hci_le_set_scan_response_args *args );

Then it'd be really hard for the user to use the function incorrectly (they'd have to pass in an invalid pointer for args which would be clear to anyone inspecting the usage of the function); the data is always allocated. However, that'd break existing users to change to that argument paradigm.

It'd be much easier to have the function validate the length. The code looks to be auto generated, and it'd be easy to modify the memcpy portion of the autogeneration to actually only copy in the specified length. It looks like this assumed length exists in 49 interfaces:

Side note (and I haven't used this, and with testing GCC doesn't seem to emit an error but clang does), from the C99 standard, page 119, §6.7.5.3, paragraph 7:

A declaration of a parameter as ‘‘array of type’’ shall be adjusted to ‘‘qualified pointer to type’’, where the type qualifiers (if any) are those specified within the [ and ] of the array type derivation. If the keyword static also appears within the [ and ] of the array type derivation, then for each call to the function, the value of the corresponding actual argument shall provide access to the first element of an array with at least as many elements as specified by the size expression.

The function declaration could be like the following:

tBleStatus hci_le_set_scan_response_data( uint8_t Scan_Response_Data_Length,
                                          const uint8_t Scan_Response_Data[static 31] );

However, if it doesn't enforce it at the compiler level it's not really worth anything.

Additional notes on this topic from the SEI CERT C Secure coding standard:

• https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
  • "Dereferencing a null pointer is undefined behavior, typically abnormal program termination. In some situations, however, dereferencing a null pointer can lead to the execution of arbitrary code [Jack 2007, van Sprundel 2006]. The indicated severity is for this more severe case; on platforms where it is not possible to exploit a null pointer dereference to execute arbitrary code, the actual severity is low."
• https://wiki.sei.cmu.edu/confluence/display/c/API02-C.+Functions+that+read+or+write+to+or+from+an+array+should+take+an+argument+to+specify+the+source+or+target+size
  • "That parameter is required to ensure that the function does not access memory outside the bounds of the array and adversely influence program execution."
• https://wiki.sei.cmu.edu/confluence/display/c/API05-C.+Use+conformant+array+parameters
  • "Failing to specify conformant array dimensions increases the likelihood that another developer will invoke the function with out-of-range integers, which could cause an out-of-bounds memory read or write."
  • https://wiki.sei.cmu.edu/confluence/display/c/API07-C.+Enforce+type+safety
  • "Upon return, functions should guarantee that any object returned by the function, or any modified value referenced by a pointer argument, is a valid object of function return type or argument type. Otherwise, type errors can occur in the program."

Thanks, Tim