TencentCloud / tencentcloud-iot-sdk-embedded-c

SDK for connecting to Tencent Cloud IoT from a device using embedded C.
https://cloud.tencent.com/product/iothub
Other
148 stars 71 forks source link

samples/mqtt/multi_thread_mqtt_sample.c, multi_client_shadow_sample.c, ota_mqtt_sample.c存在内存DF和UAF问题。 #37

Open kydahe opened 2 years ago

kydahe commented 2 years ago

samples/mqtt/multi_thread_mqtt_sample.c,ota_mqtt_sample.c存在内存UAF (memory Use-After-Free) 安全问题(即使用了一个已经释放的内存)。 multi_client_shadow_sample.c, ota_mqtt_sample.c存在DF(memory Double Free)安全问题(即对内存进行双重释放)。

  1. samples/mqtt/multi_thread_mqtt_sample.c https://github.com/tencentyun/qcloud-iot-sdk-embedded-c/blob/master/samples/mqtt/multi_thread_mqtt_sample.c UAF问题:在第201行已经释放了action_value内存,然而在202行又调用了action_value内存,此时释放后的action_value内存值是不确定的,会出现非预期行为。
static void _mqtt_message_handler(void *pClient, MQTTMessage *message, void *userData)
{
    if (message == NULL) {
        return;
    }

    if (MAX_SIZE_OF_TOPIC_CONTENT >= message->payload_len) {
        ...
        if (action_value != NULL) {
            temp = strstr(action_value, "-");
            if (NULL != temp) {
                tempRow = atoi(temp + 1);
                HAL_Free(action_value);
            } else {
                HAL_Free(action_value);      //!!! action_value memory is released
                Log_e("invalid action value: %s", action_value);    //!!! Use of memory action_value  after it is freed
                sg_rx_unexpected_count++;
                return;
            }
        } else {
            ...
        }

        ...
    } else {
        sg_rx_msg_buf_too_big_count++;
    }
}
  1. samples/multi_client/multi_client_shadow_sample.c https://github.com/tencentyun/qcloud-iot-sdk-embedded-c/blob/master/samples/multi_client/multi_client_shadow_sample.c DF问题:在180行已经通过IOT_Shadow_Destroy释放了shadow_client内存,但在226行再次通过IOT_Shadow_Destroy释放shadow_client内存,造成Memory Double Free安全问题。
static void _shadow_client_thread_runner(void *ptr)
{
    int   rc            = QCLOUD_ERR_FAILURE;
    void *shadow_client = NULL;
...
    shadow_client = IOT_Shadow_Construct(&init_params);
    if (shadow_client == NULL) {
        Log_e("shadow client constructed failed.");
        goto thread_exit;
    }

    // register delta property
    shadow_property.key  = thread_data->property_key;
    shadow_property.data = &current_update_count;
    shadow_property.type = JINT32;
    rc                   = IOT_Shadow_Register_Property(shadow_client, &shadow_property, OnDeltaCallback);
    if (rc != QCLOUD_RET_SUCCESS) {
        rc = IOT_Shadow_Destroy(shadow_client);    //!!! shadow_client memory was freed via IOT_Shadow_Destroy
        Log_e("register device shadow property failed, err: %d", rc);
        goto thread_exit;
    }
...
thread_exit:

    if (shadow_client != NULL) {
        IOT_Shadow_Destroy(shadow_client);    //!!! shadow_client memory was freed via IOT_Shadow_Destroy again since it was freed before.
        shadow_client = NULL;
    }

    sg_thread_status[thread_id] = 1;
}
  1. samples/ota/ota_mqtt_sample.c https://github.com/tencentyun/qcloud-iot-sdk-embedded-c/blob/master/samples/ota/ota_mqtt_sample.c UAF问题:在第285行已经释放了version内存,但在第289行又再次调用version,此时释放后的version内存值是不确定的,会出现非预期行为,造成UAF问题。 DF问题:在第285行已经释放了version内存,但在第290行又再次释放version,造成double free安全问题。
static int _get_local_fw_info(char *file_name, char *local_version)
{
    ...

    char *version = LITE_json_value_of(KEY_VER, json_doc);
    char *size    = LITE_json_value_of(KEY_SIZE, json_doc);

    if ((NULL == version) || (NULL == size)) {
        if (version)
            HAL_Free(version);
        if (size)
            HAL_Free(size);
        fclose(fp);
        return 0;
    }

    int local_size = atoi(size);
    HAL_Free(size);

    if (local_size <= 0) {
        Log_w("local info offset invalid: %d", local_size);
        HAL_Free(version);    //!!! version memory is released
        local_size = 0;
    }

    strncpy(local_version, version, FW_VERSION_MAX_LEN);    //!!! Use of memory version after it is freed
    HAL_Free(version);    //!!! Double Free issues.
    fclose(fp);
    return local_size;
}
xupenghu commented 2 years ago

感谢指出。

  1. 确实存在问题,下一版本修复
  2. IOT_Shadow_Destroy()会对入参进行检查,不会有重复释放的风险

    int IOT_Shadow_Destroy(void *handle)
    {
    IOT_FUNC_ENTRY;
    
    POINTER_SANITY_CHECK(handle, QCLOUD_ERR_INVAL);
  3. 下一版本修复
Yunlongs commented 2 years ago

感谢指出。

  1. 确实存在问题,下一版本修复
  2. IOT_Shadow_Destroy()会对入参进行检查,不会有重复释放的风险
int IOT_Shadow_Destroy(void *handle)
{
    IOT_FUNC_ENTRY;

    POINTER_SANITY_CHECK(handle, QCLOUD_ERR_INVAL);
  1. 下一版本修复

我认为对于Bug2 的问题是存在的,IOT_Shadow_Destroy()中的参数检查并没有生效。且此问题不仅会造成Double Free,也会造成UAF!请修复此问题。 理由如下:

  1. 根据IOT_Shadow_Destroy()POINTER_SANITY_CHECK检查的定义,可以看出来此check仅对在指针为空时报错。
    #define POINTER_SANITY_CHECK(ptr, err)                     \
    do {                                                   \
        if (NULL == (ptr)) {                               \
            Log_e("Invalid argument, %s = %p", #ptr, ptr); \
            return (err);                                  \
        }                                                  \
    } while (0)
  2. 在第169行对shadow_client进行了非空验证,所以第180行处shadow_client一定非空 且被释放,随后直接跳转到thread_exit分支,并在第225行处仍然非空,IOT_Shadow_Destroy()中的check并未生效!随后被释放第二次!

    static void _shadow_client_thread_runner(void *ptr)
    {
    int   rc            = QCLOUD_ERR_FAILURE;
    void *shadow_client = NULL;
    ...
    shadow_client = IOT_Shadow_Construct(&init_params);
    if (shadow_client == NULL) {     //----------------------> line 169
        Log_e("shadow client constructed failed.");
        goto thread_exit;
    }
    
    // register delta property
    shadow_property.key  = thread_data->property_key;
    shadow_property.data = &current_update_count;
    shadow_property.type = JINT32;
    rc                   = IOT_Shadow_Register_Property(shadow_client, &shadow_property, OnDeltaCallback);
    if (rc != QCLOUD_RET_SUCCESS) {
        rc = IOT_Shadow_Destroy(shadow_client);   //------------------->line 180
                                                                  //!!! shadow_client memory was freed via IOT_Shadow_Destroy
        Log_e("register device shadow property failed, err: %d", rc);
        goto thread_exit;
    }
    ...
    thread_exit:
    
    if (shadow_client != NULL) {      // -------------------------->line 225
        IOT_Shadow_Destroy(shadow_client);    //!!! shadow_client memory was freed via IOT_Shadow_Destroy again since it was freed before.
        shadow_client = NULL;
    }
    
    sg_thread_status[thread_id] = 1;
    }