5
\$\begingroup\$

Background

Lately, I found OpenSSL to be difficult to learn as a beginner, while it can be implemented inside beginner-friendly projects like socket programming. After two weeks of research, I've written the following program. I thought to share it with the community to detect issues that can be improved, and add a resource for OpenSSL learning.

Goal

The goal of this post is to raise a conversation around a working secure socket program and find ways to improve it. There are some subjects which can be discussed:

  1. Security. Can the program handle real-world threats? Or else the connection is too basic and can be exploited easily?
  2. Duplicated Settings. The client and the server both use the same settings, like error messages. In a real program, a client and a server would also use the same communication rules. My first thought was to use MongoDB to store settings, but it was very slow and messy to import settings from a database. How can we handle this?
  3. Any other tips and fixes.

Description

The program opens a simple secure connection between a client and the server. The server can handle multiple clients at a time while sending fixed echo messages "OK!". Each client is allowed to send custom messages or quit the program.

The server program requires a root certificate and a server certificate to work with. It can generate them and execute the openssl commands for you: sudo ./server install root. OpenSSL must be installed.

kali@DESKTOP-DFHKO83:/mnt/c/Users/Ori/desktop/crev/server$ sudo ./server 8080
Waiting for clients to connect...
{127.0.0.1, 54872}: this is client1! || {sent}: OK!
{127.0.0.1, 54873}: this is client2 || {sent}: OK!

Project Structure

The client and the server programs were written the same way. Therefore, only the server source code is provided. The client source code is available on github.

.
├── client
│   ├── build
│   │   └── CMakeLists.txt
│   ├── connection.c
│   ├── exit.c
│   ├── include
│   │   ├── connection.h
│   │   ├── exit.h
│   │   └── settings.h
│   └── main.c
└── server
    ├── build
    │   └── CMakeLists.txt
    ├── connection.c
    ├── exit.c
    ├── include
    │   ├── connection.h
    │   ├── exit.h
    │   └── settings.h
    └── main.c

Server Source Code

CMakeLists.txt

cmake_minimum_required(VERSION 3.13)
project(server C)
set(CMAKE_C_STANDARD 11)
add_executable(server main.c exit.c connection.c)
target_link_libraries(server ssl crypto)
sudo apt-get install openssl
cmake build
make
sudo ./server install root
sudo ./server 8080

connection.h

int bindAddr(int port);
void useCertificate(SSL_CTX* ctx, char* CertFile, char* KeyFile);
SSL_CTX* InitServerCTX(void);
void startServer(SSL_CTX *ctx, int *server, int port);
void closeSSL(SSL* ssl);
void closeServerSSL(int server, SSL_CTX* ctx);

exit.h

// --- exit integers ---
enum exit {
    EXIT_OK,
    NOT_ROOT,
    ARGS_ERR,
    SSL_MISSING,
    CRT_VERIFICATION,
    BIND_ERR,
    LISTEN_ERR,
    ABORT,
    KEYS_NOT_MATCH,
    CRT_USE_ERR,
    PKEY_USE_ERR,
    SYS_CMD_ERR,
    ROOT_CRT_ERR,
    CRT_UPDATE_ERR,
    SERVER_CRT_ERR,
    LAST
};


int report(int err);
int reportExit(int err);
const char* getExitName(enum exit ex);
void printReportsInfo();

settings.h

// --- global settings ---
#define FAIL -1
#define SSL_PKG "openssl"
#define SH_RP_ARG "reports"
#define REPORT_PATH "report/err.rprt"
#define CRT_PATH "certificates"
#define ROOT_PATH "/usr/share/ca-certificates/extra"
#define ROOT_FNAME "rootsslzp"
#define SERVER_CRT_FNAME "server"
#define ROOT_REQ_CNF "\"/C=IL/ST=Israel/L=Israel/O=Ori David/OU=root-req-unit/CN=root-crt/[email protected]/\""
#define SERVER_REQ_CNF "\"/C=IL/ST=Israel/L=Israel/O=Ori David/OU=server-req-unit/CN=server-crt/[email protected]/\""
#define SCRIPTS_PATH "scripts"
#define ROOT_CRT_WARNING "No root certificate. Suggestions:\n    1. RUN: sudo ./server install root\n    2. Install a root ceritificate as '/usr/share/extra/rootsslzpcert.crt'\n"
char *servercert, *serverkey;

connection.c

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <malloc.h>
#include <string.h>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <netinet/in.h>
#include <resolv.h>
#include "openssl/ssl.h"
#include "openssl/err.h"
#include "include/settings.h"
#include "include/connection.h"
#include "include/exit.h"

int bindAddr(int port) {
    // --- bind address to socket ---
    struct sockaddr_in addr;
    int sd = socket(PF_INET, SOCK_STREAM, 0); // --- server descriptor ---
    bzero(&addr, sizeof(addr)); // --- memset addr to zero --
    addr.sin_family = AF_INET; // --- IPv4 address family
    addr.sin_port = htons(port); // --- convert to network short byte order ---
    addr.sin_addr.s_addr = INADDR_ANY; // --- set the IP of the socket / sin_addr is an union ---
    if (bind(sd, (struct sockaddr*)&addr, sizeof(addr)) != 0) reportExit(BIND_ERR); // --- bind address ---
    if (listen(sd, 10) != 0) reportExit(LISTEN_ERR); // --- set listen queue ---
    return sd; // -- return descriptor ---
}

void useCertificate(SSL_CTX* ctx, char* CertFile, char* KeyFile) {
    // --- use server certificate ---
    if (SSL_CTX_use_certificate_file(ctx, CertFile, SSL_FILETYPE_PEM) <= 0) reportExit(CRT_USE_ERR);
    else if (SSL_CTX_use_PrivateKey_file(ctx, KeyFile, SSL_FILETYPE_PEM) <= 0) reportExit(PKEY_USE_ERR);
    else if (!SSL_CTX_check_private_key(ctx)) reportExit(KEYS_NOT_MATCH); // --- check private key ---
}

SSL_CTX* InitServerCTX(void) {
    // --- create server ssl context ---
    OpenSSL_add_all_algorithms();  // --- set cryptos ---
    SSL_load_error_strings();   // --- set error messages ---
    const SSL_METHOD *method = TLS_server_method();  // --- create server method ---
    SSL_CTX *ctx = SSL_CTX_new(method);   // --- create server context ---
    if (ctx == NULL) ERR_print_errors_fp(stderr);
    else return ctx;
    reportExit(ABORT);
}


void closeSSL(SSL* ssl) {
    // --- close client ssl ---
    SSL_free(ssl);
    close(SSL_get_fd(ssl));
}

void closeServerSSL(int server, SSL_CTX* ctx) {
    // --- close server ssl ---
    close(server);
    SSL_CTX_free(ctx); // --- release context ---
}

exit.c

#include <stdio.h>
#include <stdlib.h>
#include "include/exit.h"
#include "include/settings.h"
#include <sys/socket.h>

int report(int err) {
    // --- create report file ---
    char cmd[128];
    sprintf(cmd, "touch %s && echo %i > %s", REPORT_PATH, err, REPORT_PATH);
    system(cmd);
    printf("\nEXIT: %i\nINFO: ./server reports\n", err);
}

int reportExit(int err) {
    // --- report error and exit --
    report(err);
    exit(-1);
}

const char* getExitName(enum exit ex) {
    // --- convert exit integer to string ---
   switch (ex)
   {
      case EXIT_OK: return "EXIT OK";
      case NOT_ROOT: return "NOT ROOT";
      case ARGS_ERR: return "ARGS ERR";
      case SSL_MISSING: return "SSL MISSING";
      case CRT_VERIFICATION: return "CRT VERIFICATION";
      case BIND_ERR: return "BIND ERR";
      case LISTEN_ERR: return "LISTEN ERR";
      case ABORT: return "ABORT";
      case KEYS_NOT_MATCH: return "KEYS NOT MATCH";
      case CRT_USE_ERR: return "CRT USE ERR";
      case PKEY_USE_ERR: return "PKEY USE ERR";
      case SYS_CMD_ERR: return "SYS CMD ERR";
      case ROOT_CRT_ERR: return "ROOT CRT ERR";
      case CRT_UPDATE_ERR: return "CRT UPDATE ERR";
      case SERVER_CRT_ERR: return "SERVER CRT ERR";
   }
}

void printReportsInfo() {
    // --- print exit enum strings ---
    for (int i = EXIT_OK; i < LAST; i++) printf("[%i] %s\n", i, getExitName(i));
    reportExit(EXIT_OK);
}

main.c

// ---libraries---
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <malloc.h>
#include <string.h>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <netinet/in.h>
#include <resolv.h>
#include "openssl/ssl.h"
#include "openssl/err.h"
#include "include/settings.h"
#include "include/exit.h"
#include "include/connection.h"

// ---mkdirs strings ---
char mkdirs[512];

// --- SSL client ---
typedef struct Client {
    SSL *ssl;
    int d;
    struct sockaddr_in addr;
    socklen_t len;
} Client;


// ---functions---
void createDirs();
int listPackage(char pkg[]);
int isRootCrt();
void updateCA();
void writeRootCrt();
void writeCertificate();
void acceptConnections(int server, SSL_CTX *ctx);
void handleClient(int server, Client *client, SSL_CTX *ctx);

int main(int argc, char *argv[]){
    // --- main --
    sprintf(mkdirs, "report,/usr/share/ca-certificates/extra,%s", CRT_PATH); // --- directories str ---
    createDirs(); // --- create directories ---
    if (argc == 1) reportExit(ARGS_ERR);
    if (argc == 2 && strcmp(argv[1], "reports") == 0) printReportsInfo();
    if (argc == 3 && strcmp(argv[1], "install") == 0 && strcmp(argv[2], "root") == 0) writeRootCrt(); // --- install root certificate ---
    if (getuid() != 0) reportExit(NOT_ROOT); // --- sudo required ---
    if (!listPackage(SSL_PKG)) reportExit(SSL_MISSING); // --- openssl should be installed ---
    if (argc != 2) {
        printf("Usage: %s <server> <port>", argv[0]); // --- wrong usage ---
        reportExit(EXIT_OK);
    }
    if (!isRootCrt()) {
        printf("%s\n", ROOT_CRT_WARNING); // --- no root certificate ---
        reportExit(EXIT_OK);
    }
    writeCertificate(); // --- create server certificates ---
    // --- init SSl and server context ---
    SSL_library_init(); // --- init ssl lib ---
    SSL_CTX *ctx = InitServerCTX(); // --- create ssl context ---
    useCertificate(ctx, servercert, serverkey); // --- load sever certificates from settings ---
    int server = bindAddr(atoi(argv[1])); // --- bind address ---
    acceptConnections(server, ctx); // --- accept connection from clients ---
}

void createDirs() {
    // --- create directories --
    char cmd[64], delim[] = ",";
    char *ptr = strtok(mkdirs, delim); // --- split directories str ---
    while(ptr != NULL) {
        sprintf(cmd, "/bin/mkdir -p %s", ptr); // --- system command ---
        if (system(cmd) == FAIL) reportExit(SYS_CMD_ERR); // --- make directory ---
        ptr = strtok(NULL, delim); // --- split next ---
    }
}

int listPackage(char pkg[]) {
    // --- check if a package is installed on the system ---
    char cmd[128], out[256], pkg_str[32];
    sprintf(cmd, "/bin/dpkg -l | grep %s", pkg); // --- system command ---
    FILE *f = popen(cmd, "r"); // --- execute command ---
    if (f == NULL) reportExit(SYS_CMD_ERR); // --- faild to execute dpkg ---
    while (fgets(cmd, sizeof(cmd), f) != NULL) strcat(out, cmd); // --- copy output to buffer ---
    sprintf(pkg_str, "ii  %s", pkg); // --- package info string ---
    return strstr(out, pkg_str) != NULL;
}

int isRootCrt() {
    // --- check if there's a root certificate under ROOT_PATH
    char rootcert[128];
    sprintf(rootcert, "%s/%scert.crt", ROOT_PATH, ROOT_FNAME); // --- system command ---
    FILE *f = fopen(rootcert, "r"); // --- open root certificate ---
    return f != NULL;
}

void updateCA() {
    // --- update trustred certificates ---
    char cmd[512];
    strcat(cmd, "sudo update-ca-certificates && sudo dpkg-reconfigure ca-certificates"); // --- system command ---
    if (system(cmd) == FAIL) reportExit(CRT_UPDATE_ERR); // --- update certificates ---
}

void writeRootCrt() {
    // --- create a root CA certificate ---
    char rootkey[128], rootreq[128], rootcert[128], cmd[512], *cmdp = cmd;
    // --- keys path ---
    sprintf(rootkey, "%s/%skey.pem", ROOT_PATH, ROOT_FNAME);
    sprintf(rootreq, "%s/%sreq.pem", ROOT_PATH, ROOT_FNAME);
    sprintf(rootcert, "%s/%scert.crt", ROOT_PATH, ROOT_FNAME);
    // --- create system command ---
    cmdp += sprintf(cmdp, "openssl req -newkey rsa:2048 -sha1 -nodes -keyout %s -out %s -subj %s", rootkey, rootreq, ROOT_REQ_CNF);
    sprintf(cmdp, " && openssl x509 -req -in %s -sha1 -signkey %s -out %s", rootreq, rootkey, rootcert);
    if (system(cmd) == FAIL) reportExit(ROOT_CRT_ERR); // --- generate keys ---
    updateCA(); // --- Install CA certificate as trusted certificate ---
    writeCertificate();
    reportExit(EXIT_OK);
}


void writeCertificate() {
    // --- create server certificate under certificates folder
    char scpath[128], cmd[1024], *cmdp = cmd, basepath[64], rootkey[128];
    // --- keys path ---
    sprintf(rootkey, "%s/%skey.pem", ROOT_PATH, ROOT_FNAME);
    sprintf(basepath, "%s/%s", CRT_PATH, SERVER_CRT_FNAME);
    sprintf(scpath, "%s/%scert.pem", CRT_PATH, SERVER_CRT_FNAME);
    FILE *f = fopen(scpath, "r"); // --- search for a server certificate ---
    if (f == NULL) {
        // --- system command: generate server certificate ---
        cmdp += sprintf(cmdp, "/bin/openssl req -newkey rsa:2048 -sha256 -nodes -keyout %skey.pem -out %sreq.pem -subj %s", basepath, basepath, SERVER_REQ_CNF);
        sprintf(cmdp, " && sudo openssl x509 -req -days 3650 -sha256 -extensions v3_ca -in %sreq.pem -CA /etc/ssl/certs/%scert.pem -CAkey %s -CAcreateserial -out %s", basepath, ROOT_FNAME, rootkey, scpath);
        if (system(cmd) == FAIL) reportExit(SERVER_CRT_ERR);
    }
    // --- allocate global keys ---
    servercert = malloc(128);
    serverkey = malloc(128);
    // --- save keys ---
    strcpy(servercert, scpath);
    sprintf(serverkey, "%skey.pem", basepath);
}


void acceptConnections(int server, SSL_CTX *ctx) {
    // --- wait and create new clients ---
    Client *client;
    printf("Waiting for clients to connect...\n");
    while (1) {
        client = malloc(sizeof(Client));
        client->len = sizeof(client->addr);
        client->d = accept(server, (struct sockaddr*)&(client->addr), &(client->len)); // --- client descriptor ---
        client->ssl = SSL_new(ctx); // --- hold data for the SSL cocnnection ---
        SSL_set_fd(client->ssl, client->d); // --- assigns a socket to a SSL structure ---
        // --- wait for a SSL client to initiate the handshake ---
        if (SSL_accept(client->ssl) != FAIL)
            if (!fork()) { handleClient(server, client, ctx); }
    }
}

void handleClient(int server, Client *client, SSL_CTX *ctx) {
    // --- read messages and send echo ---
    int bytes;
    char buff[1024], echo[] = "OK!";
    while (1) {
        buff[0] = '\0'; // --- clear buffer ---
        bytes = SSL_read(client->ssl, buff, 1024); // --- read message ---
        if (bytes != 0) {
            // --- show [client, message] ---
            printf("{%s, %d}: %s || {sent}: %s\n",inet_ntoa(client->addr.sin_addr), ntohs(client->addr.sin_port), buff, echo);
            SSL_write(client->ssl, echo, strlen(echo)); // --- send echo ---
        }
        else break; // --- end connection with client ---
    }
    // --- close client ---
    close(client->d);
    SSL_free(client->ssl);
}

```
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Great question - complete and well-formatted. \$\endgroup\$
    – Reinderien
    Jan 29, 2021 at 15:03
  • \$\begingroup\$ For some reason @ori-david client code is not accessible on Github I get a 404 error \$\endgroup\$ Oct 14, 2022 at 13:55
  • 1
    \$\begingroup\$ @EugenioMiró Thanks for the update, the name of the repository has changed, and I have never updated this link... :) Try again now. \$\endgroup\$
    – Ori David
    Oct 15, 2022 at 14:44
  • \$\begingroup\$ Now it works @OriDavid thank you! \$\endgroup\$ Oct 15, 2022 at 15:19

1 Answer 1

3
\$\begingroup\$

In no particular order:

Settings

You've put them in one place - great. Changing them requires recompilation - not great. Consider moving them to a settings file.

Name conflicts

enum exit, though technically not in the same namespace as exit() from stdlib.h, is needlessly confusing. Probably name it something else.

Formatting

Your code style is very... squished. You need to add some empty lines on the inside of functions; there are currently none at all. Probably after if(), too.

Your --- in comments are not helping; they're just visual noise.

You use two different pointer styles:

SSL_CTX* InitServerCTX
SSL_CTX *ctx

I prefer the second, but either way you should pick one.

Error lookup

getExitName can be replaced to a single lookup in an array of strings, since your error enum is contiguous.

Security

You have fixed-size buffers all over the place and, just like in the "bad old days", access to them is frequently vulnerable to overrun attacks as in

sprintf(mkdirs
sprintf(cmd

You need to audit these, replacing such calls with calls that respect the buffer size.

Shell interaction

You need to re-think report(). system carries a pile of risks and issues that you can easily avoid - since all you're doing is touch, and a file write to REPORT_PATH, just use direct C I/O, without touching the shell.

Also consider append mode instead of truncate mode, as you currently have it, for your report.

Somewhat similarly, your call to dpkg - where shell access is perhaps still the most convenient method - should not pipe through grep. Just do the string match yourself.

openssl definitely has C bindings that will be more reliable, fast and safe than invoking the openssl command through the shell.

C99 initialization sugar

Your struct sockaddr_in addr initialization would benefit from using C99-style struct literal initialization, which would also obviate the call to bzero.

Discussion

Wouldn't it be faster to compile the settings within each update, [than] to read them from a file with each execution?

Think about your eventual end users. They won't even want your source code on their hard drive if they're Debian-like users that install binary packages. A settings-in-header application would execute "faster" (by a few microseconds) than one that loads its settings from a file; but twenty seconds of effort to change a settings file is faster than what - for some users - would be many minutes of toil to set up a development environment just to change a string.

The security issue you mentioned called buffer overflow vulnerability, am I right?

https://en.wikipedia.org/wiki/Buffer_overflow

Also, I have had a feeling that the shell interaction regards openssl is wrong, but the "user" ends up using the shell after all, don't they?

See e.g. this recommendation. It's a matter of "what's possible". The shell is a much (much, much) more complicated application than yours, so including it vastly broadens the surface area for bugs, security vulnerabilities, and cross-system incompatibility.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Great answer! Thank you :) Few follow up discussions before I accept. Wouldn't it be faster to compile the settings within each update, whether to read them from a file with each execution? The security issue you mentioned called buffer overflow vulnerability, am I right? Also, I have had a feeling that the shell interaction regards openssl is wrong, but the "user" ends up using the shell after all, don't they? I guess running ./server install root is a lazy way to generate the certificates, without any openssl knowledge. \$\endgroup\$
    – Ori David
    Jan 29, 2021 at 23:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.