989308 - Potential buffer overrun in update_ctrl_interface (hardware/libhardware_legacy/wifi/wifi.c)

Status: RESOLVED DUPLICATE of bug 1123636

(Firefox OS Graveyard :: Wifi, defect)

Description

[Julian Seward [:jseward]]

This is startup of b2g on nexus 5.  Looks like it reads some config
file into a malloc'd buffer and then does strstr to check if bits of
text are present.  Doesn't look like it zero terminates the buffer
before it does that.

Conditional jump or move depends on uninitialised value(s)
   at 0x48973E0: strstr (in /system/lib/valgrind/vgpreload_memcheck-arm-linux.so)
   by 0x49F9FB5: update_ctrl_interface (hardware/libhardware_legacy/wifi/wifi.c:370)
   by 0x49FA17B: ensure_config_file_exists (hardware/libhardware_legacy/wifi/wifi.c:415)
   by 0x49FA417: wifi_start_supplicant (hardware/libhardware_legacy/wifi/wifi.c:502)
   by 0x5BAF861: JBWpaSupplicantImpl::do_wifi_start_supplicant(int) (objdir-gecko/dom/wifi/../../../gecko/dom/wifi/WifiUtils.cpp:212)
   by 0x5BAFC53: WpaSupplicant::ExecuteCommand(CommandOptions, mozilla::dom::WifiResultOptions&, nsCString const&) (objdir-gecko/dom/wifi/../../../gecko/dom/wifi/WifiUtils.cpp:334)
   by 0x5BAF035: mozilla::ControlRunnable::Run() (objdir-gecko/dom/wifi/../../../gecko/dom/wifi/WifiProxyService.cpp:140)
   by 0x563ED25: nsThread::ProcessNextEvent(bool, bool*) (objdir-gecko/xpcom/threads/../../../gecko/xpcom/threads/nsThread.cpp:694)
   by 0x5612621: NS_ProcessNextEvent(nsIThread*, bool) (objdir-gecko/xpcom/build/../../../gecko/xpcom/glue/nsThreadUtils.cpp:263)
   by 0x5774B89: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (objdir-gecko/ipc/glue/../../../gecko/ipc/glue/MessagePump.cpp:336)
   by 0x5769675: MessageLoop::RunInternal() (objdir-gecko/ipc/chromium/../../../gecko/ipc/chromium/src/base/message_loop.cc:226)
   by 0x5769727: MessageLoop::Run() (objdir-gecko/ipc/chromium/../../../gecko/ipc/chromium/src/base/message_loop.cc:219)
 Uninitialised value was created by a heap allocation
   at 0x489B1B4: malloc (in /system/lib/valgrind/vgpreload_memcheck-arm-linux.so)
   by 0x49F9EED: update_ctrl_interface (hardware/libhardware_legacy/wifi/wifi.c:334)
   by 0x49FA17B: ensure_config_file_exists (hardware/libhardware_legacy/wifi/wifi.c:415)
   by 0x49FA417: wifi_start_supplicant (hardware/libhardware_legacy/wifi/wifi.c:502)
   by 0x5BAF861: JBWpaSupplicantImpl::do_wifi_start_supplicant(int) (objdir-gecko/dom/wifi/../../../gecko/dom/wifi/WifiUtils.cpp:212)
   by 0x5BAFC53: WpaSupplicant::ExecuteCommand(CommandOptions, mozilla::dom::WifiResultOptions&, nsCString const&) (objdir-gecko/dom/wifi/../../../gecko/dom/wifi/WifiUtils.cpp:334)
   by 0x5BAF035: mozilla::ControlRunnable::Run() (objdir-gecko/dom/wifi/../../../gecko/dom/wifi/WifiProxyService.cpp:140)
   by 0x563ED25: nsThread::ProcessNextEvent(bool, bool*) (objdir-gecko/xpcom/threads/../../../gecko/xpcom/threads/nsThread.cpp:694)
   by 0x5612621: NS_ProcessNextEvent(nsIThread*, bool) (objdir-gecko/xpcom/build/../../../gecko/xpcom/glue/nsThreadUtils.cpp:263)
   by 0x5774B89: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (objdir-gecko/ipc/glue/../../../gecko/ipc/glue/MessagePump.cpp:336)
   by 0x5769675: MessageLoop::RunInternal() (objdir-gecko/ipc/chromium/../../../gecko/ipc/chromium/src/base/message_loop.cc:226)
   by 0x5769727: MessageLoop::Run() (objdir-gecko/ipc/chromium/../../../gecko/ipc/chromium/src/base/message_loop.cc:219)

int update_ctrl_interface(const char *config_file) {

    int srcfd, destfd;
    int nread;
    char ifc[PROPERTY_VALUE_MAX];
    char *pbuf;
    char *sptr;
    struct stat sb;
    int ret;

    if (stat(config_file, &sb) != 0)
        return -1;

    pbuf = malloc(sb.st_size + PROPERTY_VALUE_MAX); <<<----------------- line 334
    if (!pbuf)
        return 0;
    srcfd = TEMP_FAILURE_RETRY(open(config_file, O_RDONLY));
    if (srcfd < 0) {
        ALOGE("Cannot open \"%s\": %s", config_file, strerror(errno));
        free(pbuf);
        return 0;
    }
    nread = TEMP_FAILURE_RETRY(read(srcfd, pbuf, sb.st_size));
    close(srcfd);
    if (nread < 0) {
        ALOGE("Cannot read \"%s\": %s", config_file, strerror(errno));
        free(pbuf);
        return 0;
    }

    if (!strcmp(config_file, SUPP_CONFIG_FILE)) {
        property_get("wifi.interface", ifc, WIFI_TEST_INTERFACE);
    } else {
        strcpy(ifc, CONTROL_IFACE_PATH);
    }
    /* Assume file is invalid to begin with */
    ret = -1;
    /*
     * if there is a "ctrl_interface=<value>" entry, re-write it ONLY if it is
     * NOT a directory.  The non-directory value option is an Android add-on
     * that allows the control interface to be exchanged through an environment
     * variable (initialized by the "init" program when it starts a service
     * with a "socket" option).
     *
     * The <value> is deemed to be a directory if the "DIR=" form is used or
     * the value begins with "/".
     */
    if ((sptr = strstr(pbuf, "ctrl_interface="))) {
        ret = 0;
        if ((!strstr(pbuf, "ctrl_interface=DIR=")) &&  <<<----------------- line 370
                (!strstr(pbuf, "ctrl_interface=/"))) {
            char *iptr = sptr + strlen("ctrl_interface=");
            int ilen = 0;
            int mlen = strlen(ifc);
            int nwrite;
            if (strncmp(ifc, iptr, mlen) != 0) {
                ALOGE("ctrl_interface != %s", ifc);
                while (((ilen + (iptr - pbuf)) < nread) && (iptr[ilen] != '\n'))
                    ilen++;
                mlen = ((ilen >= mlen) ? ilen : mlen) + 1;
                memmove(iptr + mlen, iptr + ilen + 1, nread - (iptr + ilen + 1 - pbuf));
                memset(iptr, '\n', mlen);
                memcpy(iptr, ifc, strlen(ifc));
                destfd = TEMP_FAILURE_RETRY(open(config_file, O_RDWR, 0660));
                if (destfd < 0) {
                    ALOGE("Cannot update \"%s\": %s", config_file, strerror(errno));
                    free(pbuf);
                    return -1;
                }
                TEMP_FAILURE_RETRY(write(destfd, pbuf, nread + mlen - ilen -1));
                close(destfd);
            }
        }
    }
    free(pbuf);
    return ret;
}

Comment 1

[Michael Wu [:mwu]]

I don't think this is a very severe issue - this is a config file that the system generates which should not be accessible by users. However, still worth fixing.

Comment 2

[Julian Seward [:jseward]]

It seems I reported this twice.  Closing as dup in favour of bug 1123636
since that bug has some what-to-do discussion about it.