ASUS DSL-AC3100 Router Firmware BCM Kernel Driver Bug

Time for some kernel bugs.

//********************************************************************************************
// misc. ioctl calls come to here. (flash, led, reset, kernel memory access, etc.)
//********************************************************************************************
static int board_ioctl( struct inode *inode, struct file *flip,
                       unsigned int command, unsigned long arg )
{
    int ret = 0;
    BOARD_IOCTL_PARMS ctrlParms;
    unsigned char ucaMacAddr[NVRAM_MAC_ADDRESS_LEN];
// ARCADYAN
    unsigned char ucaDectRfpi[NVRAM_ARCADYAN_DECT_RFPI_LEN];
    unsigned char ucaDectRxtun[NVRAM_ARCADYAN_DECT_RXTUN_LEN];
/// ARCADYAN

    switch (command) {
#if defined(BRCM_XDSL_DISTPOINT)
    case BOARD_IOCTL_FTTDP_DSP_BOOTER:  
        download_dsp_booter();
        break;
#endif

    case BOARD_IOCTL_FLASH_WRITE:

        if (copy_from_user((void*)&ctrlParms, (void*)arg, sizeof(ctrlParms)) == 0) {

            switch (ctrlParms.action) {

            case SCRATCH_PAD:

                if (ctrlParms.offset == -1)

                    ret =  kerSysScratchPadClearAll();

                else

                    ret = kerSysScratchPadSet(ctrlParms.string, ctrlParms.buf, ctrlParms.offset);

                break;

...

            case NVRAM:

            {

                NVRAM_DATA * pNvramData;

                /*

                 * Note: even though NVRAM access is protected by

                 * flashImageMutex at the kernel level, this protection will

                 * not work if two userspaces processes use ioctls to get

                 * NVRAM data, modify it, and then use this ioctl to write

                 * NVRAM data.  This seems like an unlikely scenario.

                 */

                mutex_lock(&flashImageMutex);

                if (NULL == (pNvramData = readNvramData()))

                {

                    mutex_unlock(&flashImageMutex);

                    return -ENOMEM;

                }

                if ( !strncmp(ctrlParms.string, "WLANFEATURE", 11 ) ) { //Wlan Data data

                    pNvramData->wlanParams[NVRAM_WLAN_PARAMS_LEN-1]= *(unsigned char *)(ctrlParms.string+11);

                    writeNvramDataCrcLocked(pNvramData);

                }

                else if ( !strncmp(ctrlParms.string, "WLANDATA", 8 ) ) { //Wlan Data data

                        int t_strlen=ctrlParms.strLen-8;

                        int nm=_get_wl_nandmanufacture();

                        if(nm<WLAN_MFG_PARTITION_HASSIZE) {

                                if(t_strlen>NVRAM_WLAN_PARAMS_LEN-1)

                                        t_strlen=NVRAM_WLAN_PARAMS_LEN-1;

                                memset((char *)pNvramData + ((size_t) &((NVRAM_DATA *)0)->wlanParams),

                                                0, sizeof(pNvramData->wlanParams)-1 );

                                memcpy( (char *)pNvramData + ((size_t) &((NVRAM_DATA *)0)->wlanParams),

                                                ctrlParms.string+8,

                                                t_strlen);

ok... where to begin?

lets keep the following in mind

typedef struct boardIoctParms

{

    char *string;

    char *buf;

    int strLen;

    int offset;

    BOARD_IOCTL_ACTION action;

    int result;

} BOARD_IOCTL_PARMS;

Lets go back over the code now:

    case BOARD_IOCTL_FLASH_WRITE:

        if (copy_from_user((void*)&ctrlParms, (void*)arg, sizeof(ctrlParms)) == 0) {

This is pretty straight forward. Copy the ioctl user args into kernelspace.

                else if ( !strncmp(ctrlParms.string, "WLANDATA", 8 ) ) { //Wlan Data data

                        int t_strlen=ctrlParms.strLen-8;

Huh? strncmp without copying the string from user to kernelspace? That doesn't seem good. Lets assume we can make this work.

                       int t_strlen=ctrlParms.strLen-8;

                        int nm=_get_wl_nandmanufacture();

                        if(nm<WLAN_MFG_PARTITION_HASSIZE) {

                                if(t_strlen>NVRAM_WLAN_PARAMS_LEN-1)

                                        t_strlen=NVRAM_WLAN_PARAMS_LEN-1;

t_strlen is a signed int. And ctrlParms.strLen is untrusted. Can we make t_strlen negative? We just need to pass the t_strlen > NVRAM_WLAN_PARAMS_LEN-1 check.

#define NVRAM_WLAN_PARAMS_LEN      256

Fortunately it's a signed integer, which means we can simply make ctrlParms.strLen less than 8. And we'll get a negative value into t_strlen.

                                 memset((char *)pNvramData + ((size_t) &((NVRAM_DATA *)0)->wlanParams),

                                                0, sizeof(pNvramData->wlanParams)-1 );

                                memcpy( (char *)pNvramData + ((size_t) &((NVRAM_DATA *)0)->wlanParams),

                                                ctrlParms.string+8,

                                                t_strlen);

And there we go. a memcpy with a negative size. This will cause memory corruption.

ASUS DSL-AC3100 Router Firmware radvd Bugs

Lets look at packet processing in the router code

                case ND_OPT_RDNSS_INFORMATION:
                        rdnssinfo = (struct nd_opt_rdnss_info_local *) opt_str;
                        count = rdnssinfo->nd_opt_rdnssi_len;

                        /* Check the RNDSS addresses received */
                        switch (count) {
                                case 7:

Now opt_str is the options part of the packet. Does the code check that the packet is big enough to account for these options? No. There are a bunch of cases like this. All lead to out of bounds memory access.

Lets look at the current radvd source from a recent Linux distro

                case ND_OPT_RDNSS_INFORMATION: {

                        char rdnss_str[INET6_ADDRSTRLEN];

                        struct AdvRDNSS *rdnss = 0;

                        struct nd_opt_rdnss_info_local *rdnssinfo = (struct nd_opt_rdnss_info_local *)opt_str;

                        if (len < sizeof(*rdnssinfo))

                                return;

                        int count = rdnssinfo->nd_opt_rdnssi_len;

                        /* Check the RNDSS addresses received */

                        switch (count) {

                        case 7:

It's clearly been fixed. But the patches haven't been backported to the router firmware.

ASUS DSL-AC3100 Router Firmware sendpackets Bug

This is a tiny bug, but it's still a bug nevertheless. strncpy is not guaranteed to NUL terminate if the max buf size is reached. The code below doesn't explicity NUL terminate the strncpy to iface. It's probably not been triggered because the stack is likely to be clean when the program reaches the strncpy. However, it's not guaranteed.

void
main (int argc, char **argv)
{
  pcap_t *fp;
  char errbuf[PCAP_ERRBUF_SIZE];
  int i;
  int j;
  int nstreams;

  int cnt;
  int tdelay;
  char iface[32];
  int patternlen;
  int opt;
  struct timeval tstart;
  struct timeval t;
  struct timeval tint;
  int pdone;
  int pbusy;

  iface[0] = 0;
  patternlen = 0;
  nstreams = 0;
  tdelay = 0;


  while ((opt = getopt (argc, argv, "i:t:c:p:")) != -1)
    {
      switch (opt)
        {
        case 'i':
          strncpy (iface, optarg, 32);
          break;

ASUS DSL-AC3100 Router Firmware PPP bug

Lets look at the following code in the current version of ppp on Linux.

static void
cbcp_recvreq(us, pckt, pcktlen)
    cbcp_state *us;
    u_char *pckt;
    int pcktlen;
{
    u_char type, opt_len, delay, addr_type;
    char address[256];
    int len = pcktlen;

    address[0] = 0;

    while (len >= 2) {
        dbglog("length: %d", len);

        GETCHAR(type, pckt);
        GETCHAR(opt_len, pckt);
        if (opt_len < 2 || opt_len > len)
            break;

        if (opt_len > 2)
            GETCHAR(delay, pckt);

        us->us_allowed |= (1 << type);

        switch(type) {
        case CB_CONF_NO:
            dbglog("no callback allowed");
            break;

        case CB_CONF_USER:

            dbglog("user callback allowed");

            if (opt_len > 4) {

                GETCHAR(addr_type, pckt);

                memcpy(address, pckt, opt_len - 4);

                address[opt_len - 4] = 0;

                if (address[0])

                    dbglog("address: %s", address);

            }

            break;

        case CB_CONF_ADMIN:

            dbglog("user admin defined allowed");

            break;

        case CB_CONF_LIST:

            break;

        }

        len -= opt_len;

    }

    if (len != 0) {

        if (debug)

            dbglog("cbcp_recvreq: malformed packet (%d bytes left)", len);

        return;

    }

    cbcp_resp(us);

}

Now I draw your attention to the loop condition. while (len >= 2). Lets look at the router firmware code:

    while (len) {

        dbglog("length: %d", len);

        GETCHAR(type, pckt);

        GETCHAR(opt_len, pckt);

        if (opt_len > 2)

            GETCHAR(delay, pckt);

        us->us_allowed |= (1 << type);

        switch(type) {

        case CB_CONF_NO:

            dbglog("no callback allowed");

            break;

        case CB_CONF_USER:

            dbglog("user callback allowed");

            if (opt_len > 4) {

                GETCHAR(addr_type, pckt);

                memcpy(address, pckt, opt_len - 4);

                address[opt_len - 4] = 0;

                if (address[0])

                    dbglog("address: %s", address);

            }

            break;

        case CB_CONF_ADMIN:

            dbglog("user admin defined allowed");

            break;

        case CB_CONF_LIST:

            break;

        }

        len -= opt_len;

    }

What's going to happen if the opt_len's don't add up to the packet size? Lets think of when len is 1 and opt_len is 2. It'll wrap around to a non zero negative. And the loop will keep iterating.

Clearly buggy.

ASUS DSL-AC3100 Router Firmware DHCP Bug

It's great that ASUS makes the GPL firmware source for their routers easy to download. I wish more vendors would do this.

Unfortunately, it didn't take more than a few minutes of auditing to come across the DHCPd code. Lets look at the original non ASUS code in wide-dhcp-server.

int
dhcp6_get_options(p, ep, optinfo)
        struct dhcp6opt *p, *ep;
        struct dhcp6_optinfo *optinfo;
{
        struct dhcp6opt *np, opth;
        int i, opt, optlen, reqopts, num;
        u_int16_t num16;
        char *bp, *cp, *val;
        u_int16_t val16;
        u_int32_t val32;
        struct dhcp6opt_ia optia;
        struct dhcp6_ia ia;
        struct dhcp6_list sublist;
        int authinfolen;

        bp = (char *)p;
        for (; p + 1 <= ep; p = np) {
                struct duid duid0;

                /*
                 * get the option header.  XXX: since there is no guarantee
                 * about the header alignment, we need to make a local copy.
                 */
                memcpy(&opth, p, sizeof(opth));
                optlen = ntohs(opth.dh6opt_len);

...
                case DH6OPT_STATUS_CODE:
                        if (optlen < sizeof(u_int16_t))
                                goto malformed;
                        memcpy(&val16, cp, sizeof(val16));
                        num16 = ntohs(val16);
                        debug_printf(LOG_DEBUG, "", "  status code: %s",
                            dhcp6_stcodestr(num16));

It's pretty clear that the options parsing code has to verify optlen. We also note that optlen is a signed 32-bit integer and that optlen casts ntohs() which returns a 16-bit unsigned int by default.

Lets look at the router firmware code which has added its own extensions:

      // brcm: get ACS URL from dhcp server option 17

      case DH6OPT_VENDOR_OPTS:

      {

          char *option_string;

          int option_len;

          u_int32_t enterprise_id;

          u_int16_t sub_option_num=1;

          int sub_option_offset=0;

          int sub_option_len=0;

          /* No guarentee on alignment, so copy to word variable */

          memcpy(&enterprise_id, cp, sizeof(enterprise_id));

          enterprise_id = ntohl(enterprise_id);

          // the first word in the data is the enterprise number.

          // I cannot find an Enterprise number for Broadband Forum in the

          // IANA database, so don't check for now.  See page 85 of RFC 3315.

          dprintf(LOG_DEBUG, FNAME, "    enterprise-number: %d (0x%x)\n",

                  enterprise_id, enterprise_id);

          // advance to point to the real data

          option_string = cp + 4;

          option_len = optlen - 4;

          // look for sub option 1: ManagementServer.URL

          if (findEncapVendorSpecificOption(option_string, option_len,

                 sub_option_num, &sub_option_offset, &sub_option_len))

          {

             int copyLen = sizeof(optinfo->acsURL) - 1;

             if (copyLen > sub_option_len) copyLen=sub_option_len;

             memcpy(optinfo->acsURL, &option_string[sub_option_offset], copyLen);

             optinfo->acsURL[copyLen] = '\0';

             // fprintf(stderr, "Found acsURL %s!!\n", optinfo->acsURL);

          }

Now we note that option_len is optlen - 4. There is no input validation on optlen. Because option_len is a signed int, if we make optlen < 4, we can get a negative value into option_len. Now _if_ we were able to enter the code that does:

             int copyLen = sizeof(optinfo->acsURL) - 1;

             if (copyLen > sub_option_len) copyLen=sub_option_len;

And sub_option_len was also negative, then we could get a buffer overflow, since copyLen and sub_option_len are both signed. Lets look at the function that triggers all of this: 

int findEncapVendorSpecificOption(const char *option, int len,

                                  u_int16_t sub_option_num,

                                  int *sub_option_offset, int *sub_option_len)

{

   struct dhcp6opt hdr;

   int i=0;

   u_int16_t curr_sub_option_num;

   int curr_sub_option_len;

   while (i < len)

   {

      /* no guarantee on alignment, so copy header */

      memcpy(&hdr, &option[i], sizeof(hdr));

      curr_sub_option_num = ntohs(hdr.dh6opt_type);

      curr_sub_option_len = ntohs(hdr.dh6opt_len);

      /* sanity check */

      if (i + 4 + curr_sub_option_len > len)

      {

         printf("sub-option exceeds len, %d %d %d",

                 i, curr_sub_option_len, len);

         return 0;

      }

      if (sub_option_num == curr_sub_option_num)

      {

         *sub_option_offset = i+4;

         *sub_option_len = curr_sub_option_len;

         return 1;

      }

      i += 4 + curr_sub_option_len;  /* advance i to the next sub-option */

   }

   return 0;

}

Hmm.. we get blocked. When len is negative, we can't enter the loop. Sure, we can still get out of bounds reads for option_len is positive since len is not validated. But it's unlikely that we can use this bug for anything interesting in terms of memory corruption.

So... tl;dr No input validation on length in vendor specific dhcp code. Out of bounds memory access. No memory corruption.

What other goodies exist in vendor supplied GPL source code?

Month of Kali Off-By-Ones #1 #2 # 3 #4

The following 4 code snippets have classic off-by-one bugs. They don't explicitly nul terminate strings after a strncpy. If strncpy reaches its buffer max, it won't nul terminate. In fact, strncpy's behaviour is quite problematic and prone to this type of bug so OpenBSD introduced strlcpy many years ago and other OSs have done the same.

source package: libaria #1

char myWaitingForDir[2048];

…

AREXPORT void ArClientFileLister::changeToAbsDir(const char *dir)
{
  myDataMutex.lock();
  strncpy(myWaitingForDir, dir, sizeof(myWaitingForDir));
  myLastRequested.setToNow();
  //printf("Getting %s\n", myWaitingForDir);
  std::string waitingFor = myWaitingForDir;
  myDataMutex.unlock();
  //myClient->requestOnceWithString("getDirListing", waitingFor.c_str());
  getDirListing(waitingFor.c_str());
}

source package: xenomai #2

static inline void xntimer_set_name(xntimer_t *timer, const char *name)
{
#ifdef CONFIG_XENO_OPT_STATS
        strncpy(timer->name, name, sizeof(timer->name));
#endif /* CONFIG_XENO_OPT_STATS */
}

source package: service-wrapper #3

    char localAddr[128];
...
        addr.S_un.S_addr = (u_long)tcpTable->table[i].dwLocalAddr;
        strncpy(localAddr, inet_ntoa(addr), sizeof(localAddr));
        localPort = ntohs((u_short)tcpTable->table[i].dwLocalPort);
        
#ifdef GETPORTSTATUS_DEBUG
        addr.S_un.S_addr = (u_long)tcpTable->table[i].dwRemoteAddr;
        strncpy(remoteAddr, inet_ntoa(addr), sizeof(remoteAddr));

is this a bug? inet_ntoa will be quite small and nul terminate.. I class it as a bad development practice.

source package: nfs-utils #4

        static char buf[PATH_MAX];
        struct stat st;
        char *path;

        /* First: test length of name and whether it exists */
        if (lstat(parentdir, &st) == -1) {
                (void)fprintf(stderr, "%s: Failed to stat %s: %s",
                                progname, parentdir, strerror(errno));
                return false;
        }

        /* Ensure we have a clean directory pathname */
        strncpy(buf, parentdir, sizeof(buf));
        path = dirname(buf);
        if (*path == '.') {
                (void)fprintf(stderr, "%s: Unusable directory %s",
                                progname, parentdir);
                return false;
        }