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

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

Open
kydahe opened this issue Aug 16, 2021 · 2 comments

Comments

@kydahe
Copy link

kydahe commented Aug 16, 2021

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
Copy link
Collaborator

xupenghu commented Sep 2, 2021

感谢指出。

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

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

@Yunlongs
Copy link

感谢指出。

  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)
  1. 在第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;
}

willssong2018 pushed a commit that referenced this issue Sep 26, 2022
fix : issue #37
doc : 更新动态注册文档,增加了平台证书动态注册和私有证书动态注册说明
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants