Open server asking for a password

Question

It first creates a server and waits to connect. Once the client connects to the server he gets the line. If he puts the correct code he gets output -> good job. If not -> the software disconnects.

My question is whether there is some sort of security hole in the software that allows you to know (as a client) the password. Is there any weakness in the code? Is it possible to exploit my code? If so, how?

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <signal.h>
#include <netinet/in.h>
#include <sys/wait.h>

#define ALARM_TIMEOUT_SEC (1200)
#define PASSWORD_LENGTH   (100)
#define BRUTE_FORCE_TIMEOUT (1)

int is_correct(char * given_password_hex)
{
char b2h[256] = {
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    0,  1,  2,  3,  4,  5,  6,  7,  8,  9, -1, -1, -1, -1, -1, -1, /* 0-9 */
    -1, 10, 11, 12, 13, 14, 15, -1, -1, -1, -1, -1, -1, -1, -1, -1, /* A-F */
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, 10, 11, 12, 13, 14, 15, -1, -1, -1, -1, -1, -1, -1, -1, -1, /* a-f */
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
};

char password[50] = "hajdgufh{0000000000123456780000000000000000000000}";
char given_password[50];
char value1;
    char value2;
    int i;
    char diff = 0;

    size_t given_password_hex_length = strlen(given_password_hex);
    if (PASSWORD_LENGTH != given_password_hex_length) {
        printf("bad input1: %zu\n", given_password_hex_length);
        return 0;
    }

    bzero(given_password, sizeof(given_password));
    for (i = 0; i < sizeof(given_password); i++) {
        value1 = b2h[given_password_hex[i * 2]];
        value2 = b2h[given_password_hex[i * 2 + 1]];

        if (value1 == -1 || value2 == -1) {
            printf("bad input2\n");
            return 0;
        }

    given_password[i] = (value1 << 4) | value2;
}

for (i = 0; i < 50; i++) {
    diff |= (password[i] ^ given_password[i]);
}

return (diff == 0);
}

void right_trim(char * str)
{
char * t = str + strlen(str) - 1;
char * p;
for (p = t; p >= str; p--) {
    if (!strchr(" \r\n", *p)) {
        break;
    }

    *p = '\0';
}
}

void handle(int s)
{
char inbuf[4096];//we defined inbuf as 4096 size

dup2(s, 0);
dup2(s, 1);

setbuf(stdout, NULL);

alarm(ALARM_TIMEOUT_SEC);

printf("lets see if you able to solve me : ");

if (NULL == fgets(inbuf, sizeof(inbuf), stdin)) {//fgets -> don't have any vulnarble to buffer overflow 
    return;//becoas its restrict the size of the input 
}

right_trim(inbuf);

if (is_correct(inbuf)) {
    printf("Good job!\n");
    }
}

void handle_sigchld(int sig) {
    waitpid((pid_t)(-1), 0, WNOHANG);
}

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

    printf("we in");
    if (1 == argc) {
        printf("Usage: %s <port>\n", argv[0]);
        printf("section 1");
        exit(-0);
    }

    int port = strtol(argv[1], NULL, 10);
    if (0 == port) {
        printf("section 2");
        perror("Invalid port");
        exit(-1);
    }

struct sigaction sa;
sa.sa_handler = &handle_sigchld;
sigemptyset(&sa.sa_mask);
sa.sa_flags = SA_RESTART | SA_NOCLDSTOP;
if (sigaction(SIGCHLD, &sa, 0) == -1) {
    perror("Unable to register sigaction");
    exit(-2);
}

int s = socket(AF_INET, SOCK_STREAM, 0);
if (-1 == s) {
    perror("Unable to create server socket");
    exit(-3);
}

int optval = 1;
if (0 != setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &optval, sizeof(optval))) {
    perror("Unable to setsockopt");
    exit(-4);
}

struct sockaddr_in bind_addr = {
    .sin_family = AF_INET,
    .sin_port = htons(port)
};

if (0 != bind(s, (struct sockaddr *) &bind_addr, sizeof(bind_addr))) {
    perror("Unable to bind socket");
    printf("section 3");
    exit(-5);
}

if (0 != listen(s, 10)) {
    perror("Unable to listen");
    exit(-6);
}

while (1) {
    int s_ = accept(s, NULL, NULL);

    sleep(BRUTE_FORCE_TIMEOUT);

    if (-1 == s_) {
        perror("Unable to accept");
        continue;
    }

    pid_t child_pid = fork();
    if (-1 == child_pid) {
        perror("Unable to fork");
        goto accept_cleanup;
    }

    if (0 == child_pid) {
        close(s);
        handle(s_);
        exit(0);
    }

accept_cleanup:
    close(s_);
}

exit(0);
}

Answer 1

Using char as array index

Be careful when you use a char as an array index. A char is signed, so it can take on negative values. When you do something like this:

    value1 = b2h[given_password_hex[i * 2]];

you could be reading from b2h[-128]. It would be better to use unsigned char instead, or to use casts to unsigned char in the appropriate places.

Answer 2

The easier it is to read the code, the easier it is to spot bugs and flaws in it. Use common formatting. It's hard to read when your function blocks are not indented, it looks like everything is in global scope. The readability is also increase with sensible variable names. Don't have two variables named s and s_. How am I going to keep track of which is what? You could call them server and client. handle has a parameter int s, which to the reader could be anything without checking for how the function is called. Call it socket or client.

Don't use bzero, it was deprecated in 2001 and removed from POSIX in 2008. Use the ANSI standard memset instead. It's more versatile and portable.

Your code is vulnerable to a timing attack. Since you are comparing the input string byte for byte and breaking out of the loop whenever a non matching byte is found, the response time can be measured to estimate how many leading bytes match. You should be executing the whole loop, even though you know they don't match.