Rajaram-Regupathy / libtypec

“libtypec” is aimed to provide a generic interface abstracting all platform complexity for user space to develop tools for efficient USB-C port management. The library can also enable development of diagnostic and debug tools to debug system issues around USB-C/USB PD topology.
34 stars 4 forks source link

remove redundant lstat check #13

Closed ColinIanKing closed 1 year ago

ColinIanKing commented 1 year ago

Static analysis shows that there is a race condition between lstat'ing a directory to see if it exists and opening the directory. It is theoretically possible for the file to be removed and a new one added between the lstat and the opening of the directory. Since the opendir will fail if the directory does not exist the lstat is actually not really required, so I think one should safely be able to remove the lstat check.

880static int libtypec_sysfs_get_pdos_ops(int conn_num, int partner, int offset, int num_pdo, int src_snk, int type, unsigned int *pdo_data)
 881{
 882        struct stat sb;
 883        int num_pdos_read = 0;
 884        char path_str[512], port_content[512 + 256];
 885        DIR *typec_path , *port_path;
 886        struct dirent *typec_entry, *port_entry;
 887
 888        snprintf(path_str, sizeof(path_str), SYSFS_TYPEC_PATH "/port%d", conn_num);
 889
    1. Condition lstat(path_str, &sb) == -1, taking false branch.
 890        if (lstat(path_str, &sb) == -1)
 891        {
 892                printf("Incorrect connector number : failed to open, %s", path_str);
 893                return -1;
 894        }
 895
    2. Condition partner == 0, taking true branch.
 896        if (partner == 0)
 897        {
    3. Condition src_snk, taking true branch.
 898                if(src_snk)
    4. Falling through to end of if statement.
 899                        snprintf(path_str, sizeof(path_str), SYSFS_TYPEC_PATH "/port%d/usb_power_delivery/source-capabilities", conn_num);
 900                else
 901                        snprintf(path_str, sizeof(path_str), SYSFS_TYPEC_PATH "/port%d/usb_power_delivery/sink-capabilities", conn_num);
    5. Falling through to end of if statement.
 902        }
 903        else
 904        {
 905                if(src_snk)
 906                        snprintf(path_str, sizeof(path_str), SYSFS_TYPEC_PATH "/port%d-partner/usb_power_delivery/source-capabilities", conn_num);
 907                else
 908                        snprintf(path_str, sizeof(path_str), SYSFS_TYPEC_PATH "/port%d-partner/usb_power_delivery/sink-capabilities", conn_num);                
 909        }
    CID 436122 (#1 of 1): Time of check time of use (TOCTOU)
    6. fs_check_call: Calling function lstat to perform check on path_str.
    7. Condition lstat(path_str, &sb) == -1, taking false branch.
 910        if (lstat(path_str, &sb) == -1)
 911                return -1;
 912
    8. toctou: Calling function opendir that uses path_str after a check function. This can cause a time-of-check, time-of-use race condition.
 913        typec_path = opendir(path_str);
 914        if (typec_path == NULL)
 915                return -1;
Rajaram-Regupathy commented 1 year ago

thanks fixed