axmail

In axmail

char username[20];
char fullname[31];
...
        /* Strip SSID */
        if (local) {
                pw = getpwuid(getuid());
        } else {
                strcpy(callsign, call);
                strcpy(username, callsign);
                strlwr(username);
                p = strchr(username, '-');
                if (p) *p = '\0';
                pw = getpwnam(username);
        }
...
                if (local) {
                        strcpy(username, pw->pw_name);
                        strcpy(callsign, username);
                }
                /* Strip full name from the gecos field... */
                if (strchr(pw->pw_gecos, ',') == NULL)
                        strcpy(fullname, pw->pw_gecos);
                else
                        strcpy(fullname, strtok(pw->pw_gecos, ","));

This seems to be a common legacy code bug - assumptions about username/gecos lengths etc.

Forensics Bugs (#2 rifitui)

rifitui is a tool to recover Windows recycle bins.

  int currrecoff;
  int recordsize;

...

  pread( info2_file, fourbytes, 4, 0x0C );
  recordsize = bah_to_i( fourbytes, 4 );

  record = malloc( recordsize );

...

  while (eof == 0) {
    res = pread( info2_file, record, recordsize, currrecoff );
    if (res < recordsize) {
      eof = 1;
    } else {
      filename = record + 0x04;

      index = bah_to_i( record+0x108, 4 );

      drive = bah_to_i( record+0x10C, 4 );

      deltime = win_time_to_unix( record+0x110 );

      deltm = localtime( &deltime );

      year = deltm->tm_year + 1900;

      mon = deltm->tm_mon + 1;

      sprintf( ascdeltime, "%02d/%02d/%02d %02d:%02d:%02d", mon, deltm->tm_mday, year, deltm->tm_hour, deltm->tm_min, deltm->tm_sec );

      filesize = bah_to_i( record+0x118, 4 );

      printf( "%d%s%s%s%d%s%s%s%d\n", index, delim, ascdeltime, delim, drive, delim, filename, delim, filesize );

    }

    currrecoff = currrecoff + recordsize;

It's not a big bug, but file offsets should probably be 64bit off_t and not int types. There is potential for integer overflows and other issues.

Forensics Bugs (#1 recoverjpeg)

recoverjpeg

    const char *buffer = file_name(dir_format, file_format, begin_index + i);
      i++;
      if (verbose) {
        printf("%s %ld bytes\n", buffer, (long) size);
      }
      fdout = open(buffer, O_WRONLY | O_CREAT, 0666);
      if (fdout < 0) {
        fprintf(stderr, "Unable to open %s for writing\n", buffer);
        exit(1);
      }

...

Writes to an output file (e.g., image00000.jpg) and doesn't check for it being a symlink - hence an attacker could create a symlink pointing to a privileged file that the person running recoverjpeg has write access to. This is mitigated in /tmp /var/tmp by the Linux kernel, but it's still a bug.

unhide (part #3)

Stack overflow in unhide. There is a mismatch between the maxpathlen from a readlink (it gets both sizes wrong in any case).

An attack scenario might be that a rootkit is installed by an attacker then gets code execution (again presumably) when the sysadmin tries to "unhide" the rootkit.

char cmdcont[1000] ;
...
     char proc_exe[512] ;
...
      sprintf(mypath,"/proc/%d",my_pid);
      statuscmd = stat(mypath, &buffer) ;
      if ((statuscmd == 0) && S_ISDIR(buffer.st_mode))
      {
         pid_exists[N_PROC] = TRUE ;
         strcat(mypath,"/exe") ;
         length = readlink(mypath, cmdcont, 1000) ;
         if (-1 != length)
         {
            cmdcont[length] = 0;   // terminate the string
//            printf("cmdcont(proc_exe) = %s\n", cmdcont) ;   //DEBUG
            strcpy(proc_exe,cmdcont) ;
         }
         else
         {
            strcpy(proc_exe,"unknown exe") ;
         }
      }

unhide (part #2)

In the unhide source we have the following lines of code

// sysctl kernel.pid_max
int maxpid = 32768;

Lets look at what my Linux system that has unhide says.

# sysctl kernel.pid_max

kernel.pid_max = 131072

More code in unhide:

unsigned int proc_parent_pids[65536] ;

char *proc_tasks[65536];

char *ps_pids[65536];

char *messages_pids[65536];

char message[1000] ;

char description[1000] ;

int ps_count = 0 ;

This looks like it's assuming 16bit PIDs.. I won't investigate further, but it's likely to be a problem that will allow for a rootkit to bypass unhide.

unhide (part #1)

In the Linux package unhide, which is rootkit detection software.

// Temporary string for output
char scratch[1000] ;
char cmdcont[1000] ;

...

               size_t length ;

               char myexe[512] ;

               sprintf(myexe,"%s%s/exe",mypath,directory);

//               printf("%s\n",myexe);

               length = readlink(myexe, cmdcont, 1000) ;

This use of readlink() is in a few places in the code. The trouble is that PATH_MAX isn't 1000.

# getconf -a|grep PATH_MAX

PATH_MAX                           4096

_POSIX_PATH_MAX                    4096

This probably leads to a rootkit bypass for unhide.

chkrootkit (part #4)

In chkrootkit

#define MAX_ID 99999

int main(int argc, char*argv[]) {
        int             fh_wtmp;
        int             fh_lastlog;
        struct lastlog  lastlog_ent;
        struct utmp     utmp_ent;
        long            userid[MAX_ID];

...

        for (i=0; i<MAX_ID; i++)

                userid[i]=FALSE;

...

                if (*uid > MAX_ID)

                {

                   fprintf(stderr, "MAX_ID is %ld and current uid is %ld, please check\n\r", (long int)MAX_ID, (long int)*uid );

                   exit (1);

                }

uid gets set by getpwnam(). On modern systems, it can be 32bits. Much higher than MAX_ID of 99999. If your backdoored account has a high uid, it won't be detected in lastlog/wtmp rootkit detection.

This is not unusually bad of chkrootkit. It's just old code that hasn't been maintained.

chkrootkit (part #3)

In chkrootkit
    

    if (!quiet)
    {
      signal(SIGALRM, read_status);
      alarm(5);
    }

...

void read_status() {
   double remaining_time;
   static long last_total_bytes_read=0;
   int diff;

   diff = total_wtmp_bytes_read-last_total_bytes_read;
   if (diff == 0) diff = 1;
   remaining_time=(wtmp_file_size-total_wtmp_bytes_read)*5/(diff);
   last_total_bytes_read=total_wtmp_bytes_read;

   printf("Remaining time: %6.2f seconds\n", remaining_time);
/*
   signal(SIGALRM,read_status);

   alarm(5);
*/
}

I'll just quote the man page for signal()

DESCRIPTION
       The behavior of signal() varies across UNIX versions, and has also var‐
       ied historically across different versions of Linux.   Avoid  its  use:
       use sigaction(2) instead.  See Portability below.

Is it a security bug? Unlikely. Is it a bug? Maybe. Should it be fixed? Yes, if you want to maintain it..

chkrootkit (part #2)

Subsequently to the previous bug http://blog.infosectcbr.com.au/2018/03/chkrootkit.html

int main(int argc, char*argv[]) {
        int             fh_wtmp;
        int             fh_lastlog;
        struct lastlog  lastlog_ent;
        struct utmp     utmp_ent;
        long            userid[MAX_ID];
        long            i, slot;
        int             status = 0;
        long            wtmp_bytes_read;
        struct stat     wtmp_stat;
        struct s_localpwd       *localpwd;
        uid_t           *uid;
    int         quiet = 0;

        char wtmpfile[128], lastlogfile[128];

        memcpy(wtmpfile, WTMP_FILENAME, 127);
        memcpy(lastlogfile, LASTLOG_FILENAME, 127);

        while (--argc && ++argv) /* poor man getopt */

        {

           if (!memcmp("-f", *argv, 2))

           {

              if (!--argc)

                 break;

              ++argv;

              memcpy(wtmpfile, *argv, 127);

           }

           else if (!memcmp("-l", *argv, 2))

           {

              if (!--argc)

                 break;

              ++argv;

              memcpy(lastlogfile, *argv, 127);

           }

           else if (!memcmp("-q", *argv, 2))

           {

             quiet = 1;

           }

        }

lastlogfile and wtmpfile stack buffers are not guaranteed to be nul terminated. Also, a buffer overread. Why, oh why are they using memcpy instead of a string copy?

chkrootkit (part #1)

#ifdef __FreeBSD__ 
#define WTMP_FILENAME "/var/log/wtmp"
#define LASTLOG_FILENAME "/var/log/lastlog"
#endif
#ifdef __OpenBSD__
#include <stdlib.h> 
#define WTMP_FILENAME "/var/log/wtmp"
#define LASTLOG_FILENAME "/var/log/lastlog"
#endif
#ifndef WTMP_FILENAME
#define WTMP_FILENAME "/var/log/wtmp"
#endif
#ifndef LASTLOG_FILENAME
#define LASTLOG_FILENAME "/var/log/lastlog"
#endif

So WTMP_FILENAME and LASTLOG_FILENAME are pretty bog standard C strings.

int main(int argc, char*argv[]) {

        int             fh_wtmp;

        int             fh_lastlog;

        struct lastlog  lastlog_ent;

        struct utmp     utmp_ent;

        long            userid[MAX_ID];

        long            i, slot;

        int             status = 0;

        long            wtmp_bytes_read;

        struct stat     wtmp_stat;

        struct s_localpwd       *localpwd;

        uid_t           *uid;

    int         quiet = 0;

        char wtmpfile[128], lastlogfile[128];

        memcpy(wtmpfile, WTMP_FILENAME, 127);

        memcpy(lastlogfile, LASTLOG_FILENAME, 127);

What? This is clearly a buffer overread. memcpy shouldn't be used to copy strings. It's theoretically possible that if WTMP_FILENAME et al were at the end of the text segment, and going past the end of those strings went into non readable memory, the program would SEGV.

Security guys will call this pointless, but as a developer, this is bug that should be fixed.

Wireshark (#2)

In the packet-usbip.c dissector

            num_of_devs = tvb_get_ntohl(tvb, offset);
            offset += 4;

            if (num_of_devs == 0)
                return expected_size;

            if (tvb_captured_length_remaining(tvb, offset) < (gint) (0x138 * num_of_devs))
                return 0;

            for (i = 0; i < num_of_devs; i++) {
                guint8 num_of_intf = tvb_get_guint8(tvb, offset + 0x137);
                int skip = num_of_intf * 4;

                expected_size += 0x138 + skip;
                offset += 0x138 + skip;
            }
            return expected_size;

Integer overflow with 0x138 * num_of_devs.

Does it lead to memory corruption? I'm not sure. Perhaps, perhaps not. I'm really more looking for the presence of input validation bugs as opposed to what they can affect.

2 bugs do make a right in Wireshark

Lets look at the current Wireshark source.

table_length =  tvb_get_ntohl(tvb, offset);

Looks like it's grabbing a 32bit integer.

  tf = proto_tree_add_item(info_tree, hf_address_table_length, tvb, offset, 4, ENC_BIG_ENDIAN);
  element_tree = proto_item_add_subtree(tf, ett_table_element);
  EAT(4);

And it uses up 4 bytes with that EAT(4).

  if (wccp_wccp_address_table->in_use == FALSE) {
    wccp_wccp_address_table->family = family;
    wccp_wccp_address_table->table_length =  table_length;

    /* check if the length is valid and allocate the tables if needed */
    switch (wccp_wccp_address_table->family) {
    case 1:
      if (wccp_wccp_address_table->table_ipv4 == NULL)
        wccp_wccp_address_table->table_ipv4 = (guint32 *)
          wmem_alloc0(pinfo->pool, wccp_wccp_address_table->table_length * 4);

We have an integer overflow in the alloc on 32 bit systems (where size_t is 32bits). These are potentially useful since you can get a heap overflow if you have an alloc size mismatch with the size of the buffer being used. Bug 1.

No wait..

Lets double check some things.

static gint
dissect_wccp2r1_address_table_info(tvbuff_t *tvb, int offset, int length,
                                   packet_info *pinfo, proto_tree *info_tree, wc
cp_address_table* wccp_wccp_address_table)
{
  guint16 address_length;
  guint32 i;
  gint16 family;
  guint16 table_length;
  proto_tree *element_tree;
  proto_item *tf;

Uh oh. Looks like table_length is declared as 16bit.. even though the code I showed earlier believes its 32 bits. Bug 2.

/* with version 2.01 we now have a address table which is possibly present */

typedef struct wccp_address_table {
  gboolean in_use;
  gint16 family;
  gint16 version;
  guint16 table_length;
  guint32 *table_ipv4;
  struct e_in6_addr *table_ipv6;
} wccp_address_table;

Given that table_length is 16bits, the alloc() function earlier won't integer overflow.

2 bugs don't make a right. Except, in this case, they do.