9
\$\begingroup\$

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 KEY\n");
}

#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] = {'\0'};
static char file_buffer[256] = {'\0'};

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 directory\n");
        }
    }

    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, '\0', 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] = '\0';

    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("%.*s\n", 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 store\n");
        return ERR_SYS_ERROR;
    }

    char *filename = get_passfile(pstore, argv[2]);
    if (filename == NULL) {
        fprintf(stderr, "Failed to modifify key\n");
        return ERR_NO_KEY;
    }

    if (init_crypto()) {
        fprintf(stderr, "Failed to set up crypto backend\n");
        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;
}
\$\endgroup\$
1
  • \$\begingroup\$ Did you run your code through a static code analyser? [CppCheck]|(cppcheck.sourceforge.net) works for C, as well as C++. And don't forget to do unit testing. \$\endgroup\$
    – Mawg
    Dec 21, 2018 at 9:16

3 Answers 3

5
\$\begingroup\$

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.

\$\endgroup\$
4
  • 4
    \$\begingroup\$ memset should not be used for memory scrubbing. See note here: en.cppreference.com/w/c/string/byte/memset \$\endgroup\$ Dec 21, 2018 at 0:57
  • \$\begingroup\$ @DavidBrown Fair enough - memset() is potentially optimized out. Post amened. I do not see scrub() will get optimized out. Your thoughts? \$\endgroup\$ Dec 21, 2018 at 4:22
  • \$\begingroup\$ So what could I do about fflush(stdin);? I need at least some way to ensure there are no characters read that the user did not enter after the promt. \$\endgroup\$
    – flowit
    Dec 21, 2018 at 8:43
  • \$\begingroup\$ @flowit 1) The usual solution is to insure the prior read of user input read all the data up to the end of line. 2) After that fixing that, please provide more detail why "ensure there are no characters read that the user did not enter after the prompt". \$\endgroup\$ Dec 21, 2018 at 14:37
4
\$\begingroup\$

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!

\$\endgroup\$
3
  • \$\begingroup\$ The two bits of code you posted seem identical. What am I missing? \$\endgroup\$ Dec 20, 2018 at 21:58
  • \$\begingroup\$ Oops... my bad....they are! Sorry! \$\endgroup\$
    – KoshVorlon
    Dec 20, 2018 at 22:00
  • \$\begingroup\$ Thank you Cris Luengo. I hadn't seen that before. I fixed the problem! :) \$\endgroup\$
    – KoshVorlon
    Dec 20, 2018 at 22:10
4
\$\begingroup\$
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.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Could you cite exact section of C99 that declares (void) in argument list redundant? You seem to be confusing C99 with C++. At least GCC compiles the following without problem with -std=c99, which contradicts your claim: void f(){} void test(){f(2,3);}. \$\endgroup\$
    – Ruslan
    Dec 21, 2018 at 6:49
  • \$\begingroup\$ @Ruslan ISO9899, chapter 6.9.1. Given typedef int F(void), the following are compatible function signatures: int f(void), and int g(). \$\endgroup\$
    – Reinderien
    Dec 21, 2018 at 15:44
  • \$\begingroup\$ @Ruslan Also, your example produces quite obvious warnings from gcc: warning: too many arguments in call to 'f' . \$\endgroup\$
    – Reinderien
    Dec 21, 2018 at 15:47
  • \$\begingroup\$ If you mean the footnote, then it doesn't say that int g() is the same signature as int f(void): it's simply compatible, i.e. won't cause an error when you declare as int(void) and define as int(), and vice-versa. Moreover, I've still not found any normative description of (void) as redundant (footnotes aren't normative). As for your warning, I can't reproduce it with gcc 7 with options -std=c99 -pedantic-errors -Wall -Wextra. I do get this as an error in C++ mode though, which once more confirms that it's only C++ feature, not C one. \$\endgroup\$
    – Ruslan
    Dec 21, 2018 at 16:03
  • \$\begingroup\$ @Ruslan See stackoverflow.com/questions/41803937/func-vs-funcvoid-in-c99 \$\endgroup\$
    – Reinderien
    Dec 21, 2018 at 16:52

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.