Argthtjtr

I wrote this simple program similar to "tiny password manager". Now I am looking for feedback how I can improve it.

As always, a big topic in C code is error handling. So what programming techniques could I use to write nicer code instead of all this boilerplate. Also, which errors should I catch and what error could I safely ignore?

Also, code style in general. Are there any lines I could rewrite in a nicer way?

#include <assert.h>

#include <errno.h>

#include <locale.h>

#include <stdio.h>

#include <stdlib.h>

#include <string.h>

#include <sys/stat.h>

#include <sys/types.h>

#include <unistd.h>



#include <gpgme.h>



static void usage(void)

{

    printf("USAGE: pw CMD KEYn");

}



#define ENV_PW_STORE "PW_STORE"

#define DEFAULT_PW_STORE ".pw_store"



enum {

    ERR_OK = 0,

    ERR_SYS_ERROR = 1,

    ERR_NO_KEY = 2,

    ERR_NO_CMD = 3,

    ERR_CRYPTO_ERROR = 4,

};



static char input_buffer[256] = {''};

static char file_buffer[256] = {''};



static char *get_store_dir(void)

{

    char *env = getenv(ENV_PW_STORE);

    if (env != NULL)

        return strdup(env);



    // build the default from HOME/DEFAULT_PW_STORE

    const char *home = getenv("HOME");

    if (home == NULL)

        return NULL;



    size_t required = strlen(home) + strlen(DEFAULT_PW_STORE) + 2;

    assert(required > 0);

    char *def = malloc(required);

    if (def == NULL)

        return NULL;



    snprintf(def, required, "%s/%s", home, DEFAULT_PW_STORE);

    return def;

}



static char *open_password_store(void)

{

    char *pstore = get_store_dir();

    if (pstore == NULL)

        return NULL;



    struct stat sb;

    if (!((stat(pstore, &sb) == 0) && S_ISDIR(sb.st_mode))) {

        if (mkdir(pstore, S_IRWXU)) {

            fprintf(stderr, "Failed to create keystore directoryn");

        }

    }



    return pstore;

}



static char *get_passfile(const char *dir, const char *key)

{

    assert(dir != NULL);

    assert(key != NULL);



    // build the filename from DIR/KEY.gpg

    size_t required = strlen(dir) + strlen(key) + strlen(".gpg") + 2;

    assert(required > 0);

    char *path = malloc(required);



    if (path == NULL)

        return NULL;



    snprintf(path, required, "%s/%s.gpg", dir, key);

    return path;

}



static struct crypto_ctx {

    gpgme_ctx_t ctx;

    gpgme_key_t keylist[2];

    gpgme_data_t data[2];

} cc = {};



static char *decrypt_from_file(const char *path, size_t *len)

{

    assert(path != NULL);



    if (gpgme_data_new_from_file(&cc.data[0], path, 1))

        return NULL;



    gpgme_data_new(&cc.data[1]);



    if (gpgme_op_decrypt(cc.ctx, cc.data[0], cc.data[1])) {

        gpgme_data_release(cc.data[0]);

        gpgme_data_release(cc.data[1]);

        return NULL;

    }



    gpgme_data_release(cc.data[0]);

    return gpgme_data_release_and_get_mem(cc.data[1], len);

}



static int encrypt_to_file(const char *path, char *buf, size_t len)

{

    gpgme_data_new_from_mem(&cc.data[0], buf, len, 1);

    gpgme_data_new(&cc.data[1]);



    memset(buf, '', len);



    if (gpgme_op_encrypt(cc.ctx, cc.keylist, GPGME_ENCRYPT_ALWAYS_TRUST,

                cc.data[0], cc.data[1])) {

        gpgme_data_release(cc.data[0]);

        gpgme_data_release(cc.data[1]);

        return 1;

    }



    FILE *fd = fopen(path, "wb");

    if (fd == NULL) {

        gpgme_data_release(cc.data[0]);

        gpgme_data_release(cc.data[1]);

        return 1;

    }



    size_t enc_len = 0;

    char *enc = gpgme_data_release_and_get_mem(cc.data[1], &enc_len);

    fwrite(enc, sizeof(char), enc_len, fd);

    gpgme_data_release(cc.data[0]);

    gpgme_free(enc);

    fclose(fd);

    return 0;

}



static int get_console_input(char *buf, size_t bufsize)

{

    fflush(stdin);

    fgets(buf, bufsize, stdin);



    size_t last = strlen(buf) - 1;

    // get rid of the ending newline, if present

    if (buf[last] == 'n')

        buf[last] = '';



    return last;

}



static int init_crypto(void)

{

    gpgme_check_version(NULL);

    setlocale(LC_ALL, "");

    gpgme_set_locale(NULL, LC_CTYPE, setlocale(LC_CTYPE, NULL));

    if (gpgme_engine_check_version(GPGME_PROTOCOL_OpenPGP)) {

        return 1;

    }



    if (gpgme_new(&cc.ctx)) {

        return 1;

    }



    char *key = getenv("PW_ENC_KEY");

    if (key == NULL) {

        gpgme_op_keylist_start(cc.ctx, NULL, 0);

        if (gpgme_op_keylist_next(cc.ctx, &cc.keylist[0])) {

            return 1;

        }

        gpgme_op_keylist_end(cc.ctx);

    } else {

        if (gpgme_get_key(cc.ctx, key, &cc.keylist[0], 0)) {

            return 1;

        }

    }



    if (cc.keylist[0] == NULL) {

        return 1;

    }



    return 0;

}



static void cleanup_crypto(void)

{

    if (cc.keylist[0])

        gpgme_key_unref(cc.keylist[0]);



    gpgme_release(cc.ctx);

}



static int insert_entry(const char *keyfile)

{

    assert(keyfile != NULL);



    if (access(keyfile, F_OK)) {

        printf("Inserting new key...n");

    } else {

        printf("Updating existing key...n");

    }



    printf("Insert password (no input to abort): ");



    size_t input_len = get_console_input(input_buffer, 255);

    if (input_len <= 0) {

        printf("No password inserted, aborting...n");

        return ERR_OK;

    }



    if (encrypt_to_file(keyfile, input_buffer, input_len)) {

        return ERR_CRYPTO_ERROR;

    }



    return ERR_OK;

}



static int get_entry(const char *keyfile)

{

    assert(keyfile != NULL);



    if (access(keyfile, F_OK)) {

        printf("Given key does not exist.n");

        return ERR_OK;

    }



    size_t plain_len = 0;

    char *plain = decrypt_from_file(keyfile, &plain_len);



    if (plain == NULL) {

        return ERR_CRYPTO_ERROR;

    }



    printf("%.*sn", plain_len, plain);

    gpgme_free(plain);



    return ERR_OK;

}



int main(int argc, const char **argv)

{

    if (argc != 3) {

        usage();

        return 1;

    }



    char *pstore = open_password_store();

    if (pstore == NULL) {

        fprintf(stderr, "Failed to open password storen");

        return ERR_SYS_ERROR;

    }



    char *filename = get_passfile(pstore, argv[2]);

    if (filename == NULL) {

        fprintf(stderr, "Failed to modifify keyn");

        return ERR_NO_KEY;

    }



    if (init_crypto()) {

        fprintf(stderr, "Failed to set up crypto backendn");

        return ERR_CRYPTO_ERROR;

    }



    int ret = 0;



    // process possible commands

    //  new - insert or override an entry

    //  get - return an entry



    if (strcmp("new", argv[1]) == 0) {

        ret = insert_entry(filename);

    } else if (strcmp("get", argv[1]) == 0) {

        ret = get_entry(filename);

    } else {

        fprintf(stderr, "Unknown command! Use new or get!n");

        ret = ERR_NO_CMD;

    }



    cleanup_crypto();



    free(pstore);

    free(filename);



    return ret;

}

a big topic in C code is error handling

Yes. The best C code can do is to strive for uniformity as there are a number of good approaches.

which errors should I catch (?)

Some of the most important errors to catch are the ones outside code control - this is usually all I/O functions.

Missing check:

// fgets(buf, bufsize, stdin);

if (fgets(buf, bufsize, stdin) == NULL) {

  Handle_EndOfFile_or_Error();

}

Naked fwrite():

// fwrite(enc, sizeof(char), enc_len, fd);

if (fwrite(enc, sizeof(char), enc_len, fd) < enc_len) {

  // Report error

}

which errors ... could I safely ignore?

Code well does extensive checking.

The following assert() only applies if the addition rolls over to 0 - a pedantic concern.

size_t required = strlen(home) + strlen(DEFAULT_PW_STORE) + 2;

// Questionable assert.

assert(required > 0);

If code is pedantic, could detect overflow with wider math.

#include <stdint.h>

....

//              v----------v form a `uintmax_t` and add using that math

uintmax_t sum = UINTMAX_C(2) + strlen(home) + strlen(DEFAULT_PW_STORE);

assert(sum <= SIZE_MAX); 

size_t required = (size_t) sum;

how I can improve it.

Advanced: password scrubbing

Although not key to OP review request, secure code that uses passwords will 1) scrub buffers when done 2) insure atomic access 3) use functions that do 1 & 2.

Example scrubbing:

void scrub(void *p, size_t sz) {

  volatile unsigned char *m = p;

  while (sz-- > 0) m[sz] = 0;

}



char *filename = get_passfile(pstore, argv[2]);

// Code is done with `pstore, argv[2]`, so zero it.

scrub(pstore, strlen(pstore));

scrub(argv[2], strlen(argv[2]));

Scrubbing is especially important when an error occurs someplace as that is often a hack approach to cause a core dump, etc.

Interestingly, code can write to argv[2].

This is a deep issue only cursorily covered here.

Avoid UB

fflush(stdin); is undefined behavior (UB). Avoid this compiler specific feature.

I'm fairly new in C, so feel free to ignore me, however, this section concerned me:

    size_t input_len = get_console_input(input_buffer, 255);

         if (input_len <= 0) {

          printf("No password inserted, aborting...n");

          return ERR_OK;

        }

You've set an input length of 255, great. I noticed no error trapping for someone entering a length greater than 255. This could lead to buffer overflow attacks, since this program is expected to run as root level.

I'd suggest adding a routine for that, similar to the one that you've added to detect 0 characters entered.

Love the fact that you've ended with a cleanup command and the following two free commands!

static int insert_entry(const char *keyfile)

Don't write int. typedef your enum so that you can write your own type instead. Similarly, here:

if (argc != 3) {

    usage();

    return 1;

}

You're returning 1, but elsewhere in the same function you're returning enum constants. You should choose one or the other for consistency - probably the enum constants.

static struct crypto_ctx {

    gpgme_ctx_t ctx;

    gpgme_key_t keylist[2];

    gpgme_data_t data[2];

} cc = {};

This is good, but not quite good enough. Since you've made cc a global, your code is non-reentrant. You should convert that struct to a typedef struct, remove the instance cc, and in all functions that use cc, accept a pointer to it as an argument.