Assessment of client-server program source code

Can you assess source code of my client-server program?

I would like to get an advice about design, code security etc.
Thank you very much.

The program does the following:

• Show files in directories;

• Download and upload files (for now, only one file at a time).

Syntax:

./client   dir  
./client   upload localfile.txt remotefile.txt
./client   download remotefile.txt localfile.txt

Client source code:

client.c

```
#include
#include
#include
#include
#include
#include

#define BUFFSIZE 4096 // Size of receive buffer

int parseARGS(char **args, char *line);
void DieWithError(char *errorMessage); // Error handling function
void UploadFile(int sock, char lfile, char rfile);
void DownloadFile(int sock, char lfile, char rfile);
void SysCmd(int sock, char myCommand, char myArgs);
void ShowExamples(void);

int main(int argc, char *argv[])
{
int i, percent_sent, sock; // Socket descriptor
struct sockaddr_in servAddr; // Echo server address
unsigned short servPort; // Echo server port
char *servIP; // Server IP addr
char myCommand, myArgs, commandOpt, lfile, *rfile;

// Check cmd args
if(argc == 6 || argc == 5){
servIP = argv[1]; // Server IP addr
servPort = atoi(argv[2]);
myCommand = argv[3];

if(!strcmp(myCommand, "dir"))
myArgs = argv[4];

//if(argc == 6){
if(!strcmp(myCommand, "upload") || !strcmp(myCommand, "download")){
lfile = argv[4];
rfile = argv[5];
}
} else {
ShowExamples();
exit(1);
}

// Create a reliable, stream socket using TCP
if((sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) dir \n");
printf("./client upload localfile.txt remotefile.txt\n");
printf("./client download remotefile.txt localfile.txt\n-----\n");
}

void DieWithError(char *errorMsg)
{
perror(errorMsg);
exit(1);
}

void UploadFile(int sock, char lfile, char

Buffer overflow

The first thing I checked was for buffer overflow and I found it immediately. On the client side in SysCmd():

char buffer[BUFFSIZE], *header[BUFFSIZE];

// Create status message for EXEC command
memset(buffer, 0, sizeof(buffer));
sprintf(buffer, "EXEC:%s:%s:1\r\n", myCommand, myArgs); // 1 - to delimit args from \r\n

Here, myArgs came from argv[4] and could be anything. So if myArgs is longer than BUFFSIZE (4 KB), you will overflow your buffer. You need to either use snprintf or check the length of your arguments before constructing the buffer.

I spotted several calls to sprintf in your code and all of them have the same problem.
Server side processing

On the server side, you have problems when you parse the incoming buffer sent from the client. Remember, you have no idea what the contents of the buffer are because it could have been sent by a malicious program masquerading as a client. So for example, you have this code in SysCmd() on the server side:

// Parse buffer to get EXEC commands
    parseARGS(header, buffer);
    cmd_name = header[1];
    cmd_args = header[2];

and this is the code for parseARGS():

int parseARGS(char **args, char *line)
{
   int tmp=0;
    // Parse line to get args[] elements by ":" delimeter
   args[tmp] = strtok( line, ":" );
   while ( (args[++tmp] = strtok(NULL, ":" ) ) != NULL );
   return tmp - 1;
}

• If the client sent a buffer with too few arguments (something with no colons, for example), the calling function will end up dereferencing an argument string that doesn't exist. In other words header[1] and header[2] in the example above may be NULL, and your server will crash.

• If the client sent a buffer with no null terminating character (for example, 4KB of all 'a'), then parseARGS will keep using strtok past the end of the buffer. This can possibly cause it to write a 0 to memory if it finds a ':' character somewhere. It might also cause your program to crash if it manages to read past the end of the stack.

Handling uploads on the server side

On the server side, when you handle an upload, I saw that you parse the file size passed by the client, but you don't actually use that size later on. Your code for downloading the file is this:

file_size = atoi(header[2]);  //  0){
    all_bytes_recvd += bytes_recvd;
    printf("Received %ld B, remaining data = %ld B\n", bytes_recvd, (file_size - all_bytes_recvd));

    if((bytes_written = fwrite(buffer, sizeof(char), bytes_recvd, aFile)) < 0)
        DieWithError("fwrite() failed!\n");
}

The problem here is that the client could send the file, and then immediately send another command. But because you just keep reading BUFFSIZE chunks of data from the client, you might read a chunk that includes the end of the file plus the next command. For example, the server might read this:
This is the end of the file.EXEC:dir:/tmp


and think that the whole thing is part of the file being uploaded instead of the end of the file plus another command. You could use the file_size variable to make sure you don't read past the end of the file.
4 KB Messages

Right now you use 4 KB fixed length messages. On the one hand, having fixed length messages makes your code simpler in many respects. On the other hand, you have to send 4 KB even if your command is "EXEC:dir:/tmp" or something small like that.

It wouldn't be too hard switch to variable length messages by having all your messages begin with a 4 byte length followed by the rest of the message. You could still enforce a maximum of 4 KB per message so that you could keep using fixed length buffers in your program.

StackExchange Code Review Q#97705, answer score: 4