From c44b08613508c993e7fd9f625e0b1b4775efffed Mon Sep 17 00:00:00 2001 From: Kirill Isakov Date: Fri, 22 Apr 2022 18:33:52 +0600 Subject: [PATCH] Wipe (some) secrets from memory after use to lessen the amount of sensitive information ending up in swap, core dumps, or in the hands of any remote attackers. While there still remaings a lot interesting data in configuration trees, connection_t structs, etc, this is considered a good practice nevertheless. Some bedtime reading: - http://www.daemonology.net/blog/2014-09-04-how-to-zero-a-buffer.html - http://www.daemonology.net/blog/2014-09-06-zeroing-buffers-is-insufficient.html - https://github.com/jedisct1/libsodium/blob/be58b2e6664389d9c7993b55291402934b43b3ca/src/libsodium/sodium/utils.c#L78:L101 --- src/chacha-poly1305/chacha-poly1305.c | 8 +- src/chacha-poly1305/chacha-poly1305.h | 2 +- src/conf.c | 2 +- src/ed25519/ecdh.c | 5 +- src/ed25519/ecdsa.c | 31 ++--- src/ed25519/ecdsagen.c | 3 + src/edge.c | 8 +- src/gcrypt/cipher.c | 12 +- src/gcrypt/digest.c | 4 + src/gcrypt/rsa.c | 63 +++++----- src/gcrypt/rsagen.c | 5 +- src/have.h | 2 + src/invitation.c | 43 ++++--- src/keys.c | 4 +- src/meson.build | 3 + src/net_setup.c | 12 +- src/node.c | 4 + src/openssl/rsa.c | 2 +- src/protocol.c | 6 +- src/protocol_auth.c | 29 +++-- src/protocol_key.c | 21 ++-- src/script.c | 2 +- src/sptps.c | 165 +++++++++++++------------- src/sptps.h | 26 +++- src/sptps_keypair.c | 8 +- src/sptps_test.c | 36 +++--- src/utils.c | 2 + src/utils.h | 1 + src/xalloc.h | 49 ++++++++ test/unit/meson.build | 7 ++ test/unit/test_memzero_null.c | 12 ++ test/unit/test_random_noinit.c | 8 +- test/unit/test_xalloc.c | 61 ++++++++++ 33 files changed, 419 insertions(+), 227 deletions(-) create mode 100644 test/unit/test_memzero_null.c create mode 100644 test/unit/test_xalloc.c diff --git a/src/chacha-poly1305/chacha-poly1305.c b/src/chacha-poly1305/chacha-poly1305.c index d13fec41..77d531ad 100644 --- a/src/chacha-poly1305/chacha-poly1305.c +++ b/src/chacha-poly1305/chacha-poly1305.c @@ -10,16 +10,14 @@ struct chacha_poly1305_ctx { }; chacha_poly1305_ctx_t *chacha_poly1305_init(void) { - chacha_poly1305_ctx_t *ctx = xzalloc(sizeof(*ctx)); - return ctx; + return xzalloc(sizeof(chacha_poly1305_ctx_t)); } void chacha_poly1305_exit(chacha_poly1305_ctx_t *ctx) { - free(ctx); + xzfree(ctx, sizeof(chacha_poly1305_ctx_t)); } -bool chacha_poly1305_set_key(chacha_poly1305_ctx_t *ctx, const void *vkey) { - const uint8_t *key = vkey; +bool chacha_poly1305_set_key(chacha_poly1305_ctx_t *ctx, const uint8_t *key) { chacha_keysetup(&ctx->main_ctx, key, 256); chacha_keysetup(&ctx->header_ctx, key + 32, 256); return true; diff --git a/src/chacha-poly1305/chacha-poly1305.h b/src/chacha-poly1305/chacha-poly1305.h index af7eaf5e..b558e450 100644 --- a/src/chacha-poly1305/chacha-poly1305.h +++ b/src/chacha-poly1305/chacha-poly1305.h @@ -7,7 +7,7 @@ typedef struct chacha_poly1305_ctx chacha_poly1305_ctx_t; extern chacha_poly1305_ctx_t *chacha_poly1305_init(void); extern void chacha_poly1305_exit(chacha_poly1305_ctx_t *); -extern bool chacha_poly1305_set_key(chacha_poly1305_ctx_t *ctx, const void *key); +extern bool chacha_poly1305_set_key(chacha_poly1305_ctx_t *ctx, const uint8_t *key); extern bool chacha_poly1305_encrypt(chacha_poly1305_ctx_t *ctx, uint64_t seqnr, const void *indata, size_t inlen, void *outdata, size_t *outlen); extern bool chacha_poly1305_decrypt(chacha_poly1305_ctx_t *ctx, uint64_t seqnr, const void *indata, size_t inlen, void *outdata, size_t *outlen); diff --git a/src/conf.c b/src/conf.c index a40fdfa8..c8cb1414 100644 --- a/src/conf.c +++ b/src/conf.c @@ -98,7 +98,7 @@ config_t *new_config(void) { void free_config(config_t *cfg) { free(cfg->variable); - free(cfg->value); + free_string(cfg->value); free(cfg->file); free(cfg); } diff --git a/src/ed25519/ecdh.c b/src/ed25519/ecdh.c index 469f502e..cfb2077b 100644 --- a/src/ed25519/ecdh.c +++ b/src/ed25519/ecdh.c @@ -36,16 +36,17 @@ ecdh_t *ecdh_generate_public(void *pubkey) { uint8_t seed[32]; randomize(seed, sizeof(seed)); ed25519_create_keypair(pubkey, ecdh->private, seed); + memzero(seed, sizeof(seed)); return ecdh; } bool ecdh_compute_shared(ecdh_t *ecdh, const void *pubkey, void *shared) { ed25519_key_exchange(shared, pubkey, ecdh->private); - free(ecdh); + ecdh_free(ecdh); return true; } void ecdh_free(ecdh_t *ecdh) { - free(ecdh); + xzfree(ecdh, sizeof(ecdh_t)); } diff --git a/src/ed25519/ecdsa.c b/src/ed25519/ecdsa.c index f0ce2c16..28daf4a1 100644 --- a/src/ed25519/ecdsa.c +++ b/src/ed25519/ecdsa.c @@ -47,7 +47,7 @@ ecdsa_t *ecdsa_set_base64_public_key(const char *p) { if(len != 32) { logger(DEBUG_ALWAYS, LOG_ERR, "Invalid format of public key! len = %lu", (unsigned long)len); - free(ecdsa); + ecdsa_free(ecdsa); return 0; } @@ -64,6 +64,7 @@ char *ecdsa_get_base64_public_key(ecdsa_t *ecdsa) { // Read PEM ECDSA keys static bool read_pem(FILE *fp, const char *type, void *vbuf, size_t size) { + const size_t buflen = size; char line[1024]; bool data = false; size_t typelen = strlen(type); @@ -90,16 +91,10 @@ static bool read_pem(FILE *fp, const char *type, void *vbuf, size_t size) { size_t linelen = strcspn(line, "\r\n"); size_t len = b64decode_tinc(line, line, linelen); - if(!len) { - logger(DEBUG_ALWAYS, LOG_ERR, "Invalid base64 data in PEM file\n"); + if(!len || len > size) { + logger(DEBUG_ALWAYS, LOG_ERR, "%s base64 data in PEM file\n", len ? "Too much" : "Invalid"); errno = EINVAL; - return false; - } - - if(len > size) { - logger(DEBUG_ALWAYS, LOG_ERR, "Too much base64 data in PEM file\n"); - errno = EINVAL; - return false; + goto exit; } memcpy(buf, line, len); @@ -114,7 +109,13 @@ static bool read_pem(FILE *fp, const char *type, void *vbuf, size_t size) { } else { errno = ENOENT; } + } + +exit: + memzero(line, sizeof(line)); + if(size) { + memzero(vbuf, buflen); return false; } @@ -128,8 +129,8 @@ ecdsa_t *ecdsa_read_pem_public_key(FILE *fp) { return ecdsa; } - free(ecdsa); - return 0; + ecdsa_free(ecdsa); + return NULL; } ecdsa_t *ecdsa_read_pem_private_key(FILE *fp) { @@ -139,8 +140,8 @@ ecdsa_t *ecdsa_read_pem_private_key(FILE *fp) { return ecdsa; } - free(ecdsa); - return 0; + ecdsa_free(ecdsa); + return NULL; } size_t ecdsa_size(ecdsa_t *ecdsa) { @@ -164,5 +165,5 @@ bool ecdsa_active(ecdsa_t *ecdsa) { } void ecdsa_free(ecdsa_t *ecdsa) { - free(ecdsa); + xzfree(ecdsa, sizeof(ecdsa_t)); } diff --git a/src/ed25519/ecdsagen.c b/src/ed25519/ecdsagen.c index bc14fd29..1edc945a 100644 --- a/src/ed25519/ecdsagen.c +++ b/src/ed25519/ecdsagen.c @@ -40,6 +40,7 @@ ecdsa_t *ecdsa_generate(void) { uint8_t seed[32]; randomize(seed, sizeof(seed)); ed25519_create_keypair(ecdsa->public, ecdsa->private, seed); + memzero(seed, sizeof(seed)); return ecdsa; } @@ -60,6 +61,8 @@ static bool write_pem(FILE *fp, const char *type, void *vbuf, size_t size) { size -= todo; } + memzero(base64, sizeof(base64)); + fprintf(fp, "-----END %s-----\n", type); return !ferror(fp); } diff --git a/src/edge.c b/src/edge.c index c521d0e1..baa6d26c 100644 --- a/src/edge.c +++ b/src/edge.c @@ -71,10 +71,12 @@ edge_t *new_edge(void) { } void free_edge(edge_t *e) { - sockaddrfree(&e->address); - sockaddrfree(&e->local_address); + if(e) { + sockaddrfree(&e->address); + sockaddrfree(&e->local_address); - free(e); + free(e); + } } void edge_add(edge_t *e) { diff --git a/src/gcrypt/cipher.c b/src/gcrypt/cipher.c index 37f232f6..2813da6b 100644 --- a/src/gcrypt/cipher.c +++ b/src/gcrypt/cipher.c @@ -106,7 +106,7 @@ static bool cipher_open(cipher_t *cipher, cipher_algo_t algo, cipher_mode_t mode cipher->keylen = gcry_cipher_get_algo_keylen(algo); cipher->blklen = gcry_cipher_get_algo_blklen(algo); - cipher->key = xmalloc(cipher->keylen + cipher->blklen); + cipher->key = xmalloc(cipher_keylength(cipher)); cipher->padding = mode == GCRY_CIPHER_MODE_ECB || mode == GCRY_CIPHER_MODE_CBC; return true; @@ -137,13 +137,15 @@ bool cipher_open_by_nid(cipher_t *cipher, nid_t nid) { } void cipher_close(cipher_t *cipher) { + if(!cipher) { + return; + } + if(cipher->handle) { gcry_cipher_close(cipher->handle); - cipher->handle = NULL; } - free(cipher->key); - + xzfree(cipher->key, cipher_keylength(cipher)); memset(cipher, 0, sizeof(*cipher)); } @@ -184,7 +186,7 @@ size_t cipher_blocksize(const cipher_t *cipher) { bool cipher_set_key(cipher_t *cipher, void *key, bool encrypt) { (void)encrypt; - memcpy(cipher->key, key, cipher->keylen + cipher->blklen); + memcpy(cipher->key, key, cipher_keylength(cipher)); gcry_cipher_setkey(cipher->handle, cipher->key, cipher->keylen); gcry_cipher_setiv(cipher->handle, cipher->key + cipher->keylen, cipher->blklen); diff --git a/src/gcrypt/digest.c b/src/gcrypt/digest.c index 50446f6e..cfea1f0a 100644 --- a/src/gcrypt/digest.c +++ b/src/gcrypt/digest.c @@ -111,6 +111,10 @@ bool digest_open_by_nid(digest_t *digest, nid_t nid, size_t maclength) { } void digest_close(digest_t *digest) { + if(!digest) { + return; + } + if(digest->hmac) { gcry_md_close(digest->hmac); } diff --git a/src/gcrypt/rsa.c b/src/gcrypt/rsa.c index 1162ddf6..83f177b8 100644 --- a/src/gcrypt/rsa.c +++ b/src/gcrypt/rsa.c @@ -130,7 +130,7 @@ rsa_t *rsa_set_hex_public_key(const char *n, const char *e) { if(err) { logger(DEBUG_ALWAYS, LOG_ERR, "Error while reading RSA public key: %s", gcry_strerror(errno)); - free(rsa); + rsa_free(rsa); return false; } @@ -152,8 +152,8 @@ rsa_t *rsa_set_hex_private_key(const char *n, const char *e, const char *d) { if(err) { logger(DEBUG_ALWAYS, LOG_ERR, "Error while reading RSA public key: %s", gcry_strerror(errno)); - free(rsa); - return false; + rsa_free(rsa); + return NULL; } return rsa; @@ -177,7 +177,7 @@ rsa_t *rsa_read_pem_public_key(FILE *fp) { || !ber_read_mpi(&derp, &derlen, &rsa->e) || derlen) { logger(DEBUG_ALWAYS, LOG_ERR, "Error while decoding RSA public key"); - free(rsa); + rsa_free(rsa); return NULL; } @@ -207,10 +207,11 @@ rsa_t *rsa_read_pem_private_key(FILE *fp) { || !ber_read_mpi(&derp, &derlen, NULL) // u || derlen) { logger(DEBUG_ALWAYS, LOG_ERR, "Error while decoding RSA private key"); - free(rsa); - return NULL; + rsa_free(rsa); + rsa = NULL; } + memzero(derbuf, sizeof(derbuf)); return rsa; } @@ -218,19 +219,27 @@ size_t rsa_size(const rsa_t *rsa) { return (gcry_mpi_get_nbits(rsa->n) + 7) / 8; } +static bool check(gcry_error_t err) { + if(err) { + logger(DEBUG_ALWAYS, LOG_ERR, "gcrypt error %s/%s", gcry_strsource(err), gcry_strerror(err)); + } + + return !err; +} + /* Well, libgcrypt has functions to handle RSA keys, but they suck. * So we just use libgcrypt's mpi functions, and do the math ourselves. */ -// TODO: get rid of this macro, properly clean up gcry_ structures after use -#define check(foo) { gcry_error_t err = (foo); if(err) {logger(DEBUG_ALWAYS, LOG_ERR, "gcrypt error %s/%s at %s:%d", gcry_strsource(err), gcry_strerror(err), __FILE__, __LINE__); return false; }} +static bool rsa_powm(const gcry_mpi_t ed, const gcry_mpi_t n, const void *in, size_t len, void *out) { + gcry_mpi_t inmpi = NULL; -bool rsa_public_encrypt(rsa_t *rsa, const void *in, size_t len, void *out) { - gcry_mpi_t inmpi; - check(gcry_mpi_scan(&inmpi, GCRYMPI_FMT_USG, in, len, NULL)); + if(!check(gcry_mpi_scan(&inmpi, GCRYMPI_FMT_USG, in, len, NULL))) { + return false; + } - gcry_mpi_t outmpi = gcry_mpi_new(len * 8); - gcry_mpi_powm(outmpi, inmpi, rsa->e, rsa->n); + gcry_mpi_t outmpi = gcry_mpi_snew(len * 8); + gcry_mpi_powm(outmpi, inmpi, ed, n); size_t out_bytes = (gcry_mpi_get_nbits(outmpi) + 7) / 8; size_t pad = len - MIN(out_bytes, len); @@ -240,28 +249,20 @@ bool rsa_public_encrypt(rsa_t *rsa, const void *in, size_t len, void *out) { *pout++ = 0; } - check(gcry_mpi_print(GCRYMPI_FMT_USG, pout, len, NULL, outmpi)); - - return true; -} + bool ok = check(gcry_mpi_print(GCRYMPI_FMT_USG, pout, len, NULL, outmpi)); -bool rsa_private_decrypt(rsa_t *rsa, const void *in, size_t len, void *out) { - gcry_mpi_t inmpi; - check(gcry_mpi_scan(&inmpi, GCRYMPI_FMT_USG, in, len, NULL)); - - gcry_mpi_t outmpi = gcry_mpi_new(len * 8); - gcry_mpi_powm(outmpi, inmpi, rsa->d, rsa->n); - - size_t pad = len - (gcry_mpi_get_nbits(outmpi) + 7) / 8; - unsigned char *pout = out; + gcry_mpi_release(outmpi); + gcry_mpi_release(inmpi); - for(; pad; --pad) { - *pout++ = 0; - } + return ok; +} - check(gcry_mpi_print(GCRYMPI_FMT_USG, pout, len, NULL, outmpi)); +bool rsa_public_encrypt(rsa_t *rsa, const void *in, size_t len, void *out) { + return rsa_powm(rsa->e, rsa->n, in, len, out); +} - return true; +bool rsa_private_decrypt(rsa_t *rsa, const void *in, size_t len, void *out) { + return rsa_powm(rsa->d, rsa->n, in, len, out); } void rsa_free(rsa_t *rsa) { diff --git a/src/gcrypt/rsagen.c b/src/gcrypt/rsagen.c index 01bb1378..85765557 100644 --- a/src/gcrypt/rsagen.c +++ b/src/gcrypt/rsagen.c @@ -26,6 +26,7 @@ #include "pem.h" #include "../rsagen.h" #include "../xalloc.h" +#include "../utils.h" // ASN.1 tags. typedef enum { @@ -240,7 +241,9 @@ bool rsa_write_pem_private_key(rsa_t *rsa, FILE *fp) { gcry_mpi_release(params[dq]); gcry_mpi_release(params[u]); - return pem_encode(fp, "RSA PRIVATE KEY", derbuf, derlen); + bool success = pem_encode(fp, "RSA PRIVATE KEY", derbuf, derlen); + memzero(derbuf, sizeof(derbuf)); + return success; } static gcry_mpi_t find_mpi(const gcry_sexp_t rsa, const char *token) { diff --git a/src/have.h b/src/have.h index 6c9d6754..d176098f 100644 --- a/src/have.h +++ b/src/have.h @@ -29,6 +29,8 @@ #define _CRT_NONSTDC_NO_WARNINGS #endif +#define __STDC_WANT_LIB_EXT1__ 1 + #include #include #include diff --git a/src/invitation.c b/src/invitation.c index cff9e727..5db1e987 100644 --- a/src/invitation.c +++ b/src/invitation.c @@ -314,6 +314,7 @@ static bool copy_config_replacing_port(FILE *out, const char *filename, const ch } } + memzero(line, sizeof(line)); fclose(in); return true; } @@ -521,6 +522,7 @@ int cmd_invite(int argc, char *argv[]) { int ifd = open(filename, O_RDWR | O_CREAT | O_EXCL, 0600); if(!ifd) { + memzero(cookie, sizeof(cookie)); fprintf(stderr, "Could not create invitation file %s: %s\n", filename, strerror(errno)); return 1; } @@ -536,9 +538,17 @@ int cmd_invite(int argc, char *argv[]) { char *port = NULL; if(!get_my_hostname(&address, &port)) { + memzero(cookie, sizeof(cookie)); return 1; } + // Create an URL from the local address, key hash and cookie + char *url; + xasprintf(&url, "%s/%s%s", address, hash, cookie); + + memzero(cookie, sizeof(cookie)); + free(address); + // Fill in the details. fprintf(f, "Name = %s\n", argv[1]); @@ -577,14 +587,10 @@ int cmd_invite(int argc, char *argv[]) { if(!appended) { fprintf(stderr, "Could not append my config to invitation file: %s.\n", strerror(errno)); - free(address); + free_string(url); return 1; } - // Create an URL from the local address, key hash and cookie - char *url; - xasprintf(&url, "%s/%s%s", address, hash, cookie); - // Call the inviation-created script environment_t env; environment_init(&env); @@ -595,8 +601,7 @@ int cmd_invite(int argc, char *argv[]) { environment_exit(&env); puts(url); - free(url); - free(address); + free_string(url); return 0; } @@ -972,9 +977,9 @@ make_names: return false; } - char *b64key = ecdsa_get_base64_public_key(key); + char *b64_pubkey = ecdsa_get_base64_public_key(key); - if(!b64key) { + if(!b64_pubkey) { return false; } @@ -994,10 +999,10 @@ make_names: fclose(f); - fprintf(fh, "Ed25519PublicKey = %s\n", b64key); + fprintf(fh, "Ed25519PublicKey = %s\n", b64_pubkey); - sptps_send_record(&sptps, 1, b64key, strlen(b64key)); - free(b64key); + sptps_send_record(&sptps, 1, b64_pubkey, strlen(b64_pubkey)); + free(b64_pubkey); ecdsa_free(key); #ifndef DISABLE_LEGACY @@ -1299,13 +1304,13 @@ int cmd_join(int argc, char *argv[]) { return 1; } - char *b64key = ecdsa_get_base64_public_key(key); + char *b64_pubkey = ecdsa_get_base64_public_key(key); // Connect to the tinc daemon mentioned in the URL. struct addrinfo *ai = str2addrinfo(address, port, SOCK_STREAM); if(!ai) { - free(b64key); + free(b64_pubkey); ecdsa_free(key); return 1; } @@ -1320,7 +1325,7 @@ next: if(!aip) { freeaddrinfo(ai); - free(b64key); + free(b64_pubkey); ecdsa_free(key); return 1; } @@ -1346,13 +1351,13 @@ next: fprintf(stderr, "Connected to %s port %s...\n", address, port); // Tell him we have an invitation, and give him our throw-away key. - ssize_t len = snprintf(line, sizeof(line), "0 ?%s %d.%d\n", b64key, PROT_MAJOR, PROT_MINOR); + ssize_t len = snprintf(line, sizeof(line), "0 ?%s %d.%d\n", b64_pubkey, PROT_MAJOR, PROT_MINOR); if(len <= 0 || (size_t)len >= sizeof(line)) { abort(); } - if(!sendline(sock, "0 ?%s %d.%d", b64key, PROT_MAJOR, 1)) { + if(!sendline(sock, "0 ?%s %d.%d", b64_pubkey, PROT_MAJOR, 1)) { fprintf(stderr, "Error sending request to %s port %s: %s\n", address, port, strerror(errno)); closesocket(sock); goto next; @@ -1371,8 +1376,8 @@ next: ai = NULL; aip = NULL; - free(b64key); - b64key = NULL; + free(b64_pubkey); + b64_pubkey = NULL; // Check if the hash of the key he gave us matches the hash in the URL. char *fingerprint = line + 2; diff --git a/src/keys.c b/src/keys.c index 2db2b159..ac99ac40 100644 --- a/src/keys.c +++ b/src/keys.c @@ -227,13 +227,13 @@ rsa_t *read_rsa_private_key(splay_tree_t *config_tree, char **keyfile) { if(get_config_string(rsa_priv_conf, &d)) { if(!get_config_string(lookup_config(config_tree, "PublicKey"), &n)) { logger(DEBUG_ALWAYS, LOG_ERR, "PrivateKey used but no PublicKey found!"); - free(d); + free_string(d); return NULL; } key = rsa_set_hex_private_key(n, "FFFF", d); free(n); - free(d); + free_string(d); if(key && keyfile && rsa_priv_conf->file) { *keyfile = xstrdup(rsa_priv_conf->file); diff --git a/src/meson.build b/src/meson.build index c8e783b7..f84fb4d0 100644 --- a/src/meson.build +++ b/src/meson.build @@ -73,8 +73,11 @@ endif check_functions = [ 'asprintf', 'daemon', + 'explicit_bzero', + 'explicit_memset', 'fchmod', 'gettimeofday', + 'memset_s', 'mlockall', 'putenv', 'strsignal', diff --git a/src/net_setup.c b/src/net_setup.c index b0b87608..40cdaf6c 100644 --- a/src/net_setup.c +++ b/src/net_setup.c @@ -267,7 +267,7 @@ bool setup_myself_reloadable(void) { proxytype = PROXY_EXEC; } else { logger(DEBUG_ALWAYS, LOG_ERR, "Unknown proxy type %s!", proxy); - free(proxy); + free_string(proxy); return false; } @@ -277,10 +277,10 @@ bool setup_myself_reloadable(void) { free(proxyport); proxyport = NULL; - free(proxyuser); + free_string(proxyuser); proxyuser = NULL; - free(proxypass); + free_string(proxypass); proxypass = NULL; switch(proxytype) { @@ -291,7 +291,7 @@ bool setup_myself_reloadable(void) { case PROXY_EXEC: if(!space || !*space) { logger(DEBUG_ALWAYS, LOG_ERR, "Argument expected for proxy type exec!"); - free(proxy); + free_string(proxy); return false; } @@ -312,7 +312,7 @@ bool setup_myself_reloadable(void) { logger(DEBUG_ALWAYS, LOG_ERR, "Host and port argument expected for proxy!"); proxyport = NULL; proxyhost = NULL; - free(proxy); + free_string(proxy); return false; } @@ -338,7 +338,7 @@ bool setup_myself_reloadable(void) { break; } - free(proxy); + free_string(proxy); } bool choice; diff --git a/src/node.c b/src/node.c index 53013d2d..23a06101 100644 --- a/src/node.c +++ b/src/node.c @@ -89,6 +89,10 @@ node_t *new_node(void) { } void free_node(node_t *n) { + if(!n) { + return; + } + splay_empty_tree(&n->subnet_tree); splay_empty_tree(&n->edge_tree); diff --git a/src/openssl/rsa.c b/src/openssl/rsa.c index a4e4ac69..56ebf277 100644 --- a/src/openssl/rsa.c +++ b/src/openssl/rsa.c @@ -113,7 +113,7 @@ exit: if(!rsa) #endif { - BN_free(bn_d); + BN_clear_free(bn_d); BN_free(bn_e); BN_free(bn_n); } diff --git a/src/protocol.c b/src/protocol.c index 3c006d5c..bf165a7d 100644 --- a/src/protocol.c +++ b/src/protocol.c @@ -80,8 +80,10 @@ static int past_request_compare(const past_request_t *a, const past_request_t *b } static void free_past_request(past_request_t *r) { - free((char *)r->request); - free(r); + if(r) { + free((char *)r->request); + free(r); + } } static splay_tree_t past_request_tree = { diff --git a/src/protocol_auth.c b/src/protocol_auth.c index 22254575..b13d901a 100644 --- a/src/protocol_auth.c +++ b/src/protocol_auth.c @@ -554,9 +554,10 @@ bool send_metakey(connection_t *c) { } const size_t len = rsa_size(ctx->rsa); + const size_t hexkeylen = HEX_SIZE(len); char *key = alloca(len); char *enckey = alloca(len); - char *hexkey = alloca(2 * len + 1); + char *hexkey = alloca(hexkeylen); /* Create a random key */ @@ -576,12 +577,14 @@ bool send_metakey(connection_t *c) { if(!cipher_set_key_from_rsa(&ctx->out.cipher, key, len, true)) { free_legacy_ctx(ctx); + memzero(key, len); return false; } if(debug_level >= DEBUG_SCARY_THINGS) { bin2hex(key, hexkey, len); logger(DEBUG_SCARY_THINGS, LOG_DEBUG, "Generated random meta key (unencrypted): %s", hexkey); + memzero(hexkey, hexkeylen); } /* Encrypt the random data @@ -591,7 +594,10 @@ bool send_metakey(connection_t *c) { with a length equal to that of the modulus of the RSA key. */ - if(!rsa_public_encrypt(ctx->rsa, key, len, enckey)) { + bool encrypted = rsa_public_encrypt(ctx->rsa, key, len, enckey); + memzero(key, len); + + if(!encrypted) { free_legacy_ctx(ctx); logger(DEBUG_ALWAYS, LOG_ERR, "Error during encryption of meta key for %s (%s)", c->name, c->hostname); return false; @@ -631,6 +637,11 @@ bool metakey_h(connection_t *c, const char *request) { return false; } + if(!cipher || !digest) { + logger(DEBUG_ALWAYS, LOG_ERR, "Possible intruder %s (%s): cipher %d, digest %d", c->name, c->hostname, cipher, digest); + return false; + } + /* Convert the challenge from hexadecimal back to binary */ size_t inlen = hex2bin(hexkey, enckey, len); @@ -652,26 +663,26 @@ bool metakey_h(connection_t *c, const char *request) { if(debug_level >= DEBUG_SCARY_THINGS) { bin2hex(key, hexkey, len); logger(DEBUG_SCARY_THINGS, LOG_DEBUG, "Received random meta key (unencrypted): %s", hexkey); + // Hopefully the user knew what he was doing leaking session keys into logs. We'll do the right thing here anyway. + memzero(hexkey, HEX_SIZE(len)); } /* Check and lookup cipher and digest algorithms */ - if(!cipher || !digest) { - logger(DEBUG_ALWAYS, LOG_ERR, "Possible intruder %s (%s): cipher %d, digest %d", c->name, c->hostname, cipher, digest); - return false; - } - if(!init_crypto_by_nid(&c->legacy->in, cipher, digest)) { + memzero(key, len); logger(DEBUG_ALWAYS, LOG_ERR, "Error during initialisation of cipher or digest from %s (%s)", c->name, c->hostname); return false; } - if(!cipher_set_key_from_rsa(&c->legacy->in.cipher, key, len, false)) { + bool key_set = cipher_set_key_from_rsa(&c->legacy->in.cipher, key, len, false); + memzero(key, len); + + if(!key_set) { logger(DEBUG_ALWAYS, LOG_ERR, "Error setting RSA key for %s (%s)", c->name, c->hostname); return false; } - c->status.decryptin = true; c->allow_request = CHALLENGE; diff --git a/src/protocol_key.c b/src/protocol_key.c index 740d2fb4..09acd6a9 100644 --- a/src/protocol_key.c +++ b/src/protocol_key.c @@ -33,7 +33,7 @@ #include "utils.h" #include "compression.h" #include "random.h" -#include "legacy.h" +#include "xalloc.h" void send_key_changed(void) { #ifndef DISABLE_LEGACY @@ -341,7 +341,8 @@ bool send_ans_key(node_t *to) { return false; #else size_t keylen = myself->incipher ? cipher_keylength(myself->incipher) : 1; - char *key = alloca(keylen * 2 + 1); + size_t keyhexlen = HEX_SIZE(keylen); + char *key = alloca(keyhexlen); randomize(key, keylen); @@ -388,12 +389,16 @@ bool send_ans_key(node_t *to) { to->status.validkey_in = true; - return send_request(to->nexthop->connection, "%d %s %s %s %d %d %lu %d", ANS_KEY, - myself->name, to->name, key, - cipher_get_nid(to->incipher), - digest_get_nid(to->indigest), - (unsigned long)digest_length(to->indigest), - to->incompression); + bool sent = send_request(to->nexthop->connection, "%d %s %s %s %d %d %lu %d", ANS_KEY, + myself->name, to->name, key, + cipher_get_nid(to->incipher), + digest_get_nid(to->indigest), + (unsigned long)digest_length(to->indigest), + to->incompression); + + memzero(key, keyhexlen); + + return sent; #endif } diff --git a/src/script.c b/src/script.c index 0b16c861..3f44bf9f 100644 --- a/src/script.c +++ b/src/script.c @@ -134,7 +134,7 @@ void environment_init(environment_t *env) { void environment_exit(environment_t *env) { for(int i = 0; i < env->n; i++) { - free(env->entries[i]); + free_string(env->entries[i]); } free(env->entries); diff --git a/src/sptps.c b/src/sptps.c index a0483c34..42eaba0c 100644 --- a/src/sptps.c +++ b/src/sptps.c @@ -23,10 +23,10 @@ #include "chacha-poly1305/chacha-poly1305.h" #include "ecdh.h" #include "ecdsa.h" -#include "logger.h" #include "prf.h" #include "sptps.h" #include "random.h" +#include "xalloc.h" unsigned int sptps_replaywin = 16; @@ -90,6 +90,22 @@ static void warning(sptps_t *s, const char *format, ...) { va_end(ap); } +static sptps_kex_t *new_sptps_kex(void) { + return xzalloc(sizeof(sptps_kex_t)); +} + +static void free_sptps_kex(sptps_kex_t *kex) { + xzfree(kex, sizeof(sptps_kex_t)); +} + +static sptps_key_t *new_sptps_key(void) { + return xzalloc(sizeof(sptps_key_t)); +} + +static void free_sptps_key(sptps_key_t *key) { + xzfree(key, sizeof(sptps_key_t)); +} + // Send a record (datagram version, accepts all record types, handles encryption and authentication). static bool send_record_priv_datagram(sptps_t *s, uint8_t type, const void *data, uint16_t len) { uint8_t *buffer = alloca(len + 21UL); @@ -154,49 +170,49 @@ bool sptps_send_record(sptps_t *s, uint8_t type, const void *data, uint16_t len) // Send a Key EXchange record, containing a random nonce and an ECDHE public key. static bool send_kex(sptps_t *s) { - size_t keylen = ECDH_SIZE; - // Make room for our KEX message, which we will keep around since send_sig() needs it. if(s->mykex) { return false; } - s->mykex = realloc(s->mykex, 1 + 32 + keylen); - - if(!s->mykex) { - return error(s, errno, strerror(errno)); - } + s->mykex = new_sptps_kex(); // Set version byte to zero. - s->mykex[0] = SPTPS_VERSION; + s->mykex->version = SPTPS_VERSION; // Create a random nonce. - randomize(s->mykex + 1, 32); + randomize(s->mykex->nonce, ECDH_SIZE); // Create a new ECDH public key. - if(!(s->ecdh = ecdh_generate_public(s->mykex + 1 + 32))) { + if(!(s->ecdh = ecdh_generate_public(s->mykex->pubkey))) { return error(s, EINVAL, "Failed to generate ECDH public key"); } - return send_record_priv(s, SPTPS_HANDSHAKE, s->mykex, 1 + 32 + keylen); + return send_record_priv(s, SPTPS_HANDSHAKE, s->mykex, sizeof(sptps_kex_t)); +} + +static size_t sigmsg_len(size_t labellen) { + return 1 + 2 * sizeof(sptps_kex_t) + labellen; +} + +static void fill_msg(uint8_t *msg, bool initiator, const sptps_kex_t *kex0, const sptps_kex_t *kex1, const sptps_t *s) { + *msg = initiator, msg++; + memcpy(msg, kex0, sizeof(*kex0)), msg += sizeof(*kex0); + memcpy(msg, kex1, sizeof(*kex1)), msg += sizeof(*kex1); + memcpy(msg, s->label, s->labellen); } // Send a SIGnature record, containing an Ed25519 signature over both KEX records. static bool send_sig(sptps_t *s) { - size_t keylen = ECDH_SIZE; - size_t siglen = ecdsa_size(s->mykey); - // Concatenate both KEX messages, plus tag indicating if it is from the connection originator, plus label - const size_t msglen = (1 + 32 + keylen) * 2 + 1 + s->labellen; + size_t msglen = sigmsg_len(s->labellen); uint8_t *msg = alloca(msglen); - uint8_t *sig = alloca(siglen); - - msg[0] = s->initiator; - memcpy(msg + 1, s->mykex, 1 + 32 + keylen); - memcpy(msg + 1 + 33 + keylen, s->hiskex, 1 + 32 + keylen); - memcpy(msg + 1 + 2 * (33 + keylen), s->label, s->labellen); + fill_msg(msg, s->initiator, s->mykex, s->hiskex, s); // Sign the result. + size_t siglen = ecdsa_size(s->mykey); + uint8_t *sig = alloca(siglen); + if(!ecdsa_sign(s->mykey, msg, msglen, sig)) { return error(s, EINVAL, "Failed to sign SIG record"); } @@ -218,30 +234,27 @@ static bool generate_key_material(sptps_t *s, const uint8_t *shared, size_t len) } // Allocate memory for key material - size_t keylen = 2 * CHACHA_POLY1305_KEYLEN; + s->key = new_sptps_key(); - s->key = realloc(s->key, keylen); + // Create the HMAC seed, which is "key expansion" + session label + server nonce + client nonce + const size_t msglen = sizeof("key expansion") - 1; + const size_t seedlen = msglen + s->labellen + ECDH_SIZE * 2; + uint8_t *seed = alloca(seedlen); - if(!s->key) { - return error(s, errno, strerror(errno)); - } + uint8_t *ptr = seed; + memcpy(ptr, "key expansion", msglen); + ptr += msglen; - // Create the HMAC seed, which is "key expansion" + session label + server nonce + client nonce - uint8_t *seed = alloca(s->labellen + 64 + 13); - memcpy(seed, "key expansion", 13); + memcpy(ptr, (s->initiator ? s->mykex : s->hiskex)->nonce, ECDH_SIZE); + ptr += ECDH_SIZE; - if(s->initiator) { - memcpy(seed + 13, s->mykex + 1, 32); - memcpy(seed + 45, s->hiskex + 1, 32); - } else { - memcpy(seed + 13, s->hiskex + 1, 32); - memcpy(seed + 45, s->mykex + 1, 32); - } + memcpy(ptr, (s->initiator ? s->hiskex : s->mykex)->nonce, ECDH_SIZE); + ptr += ECDH_SIZE; - memcpy(seed + 77, s->label, s->labellen); + memcpy(ptr, s->label, s->labellen); // Use PRF to generate the key material - if(!prf(shared, len, seed, s->labellen + 64 + 13, s->key, keylen)) { + if(!prf(shared, len, seed, seedlen, s->key->both, sizeof(sptps_key_t))) { return error(s, EINVAL, "Failed to generate key material"); } @@ -261,17 +274,13 @@ static bool receive_ack(sptps_t *s, const uint8_t *data, uint16_t len) { return error(s, EIO, "Invalid ACK record length"); } - if(s->initiator) { - if(!chacha_poly1305_set_key(s->incipher, s->key)) { - return error(s, EINVAL, "Failed to set counter"); - } - } else { - if(!chacha_poly1305_set_key(s->incipher, s->key + CHACHA_POLY1305_KEYLEN)) { - return error(s, EINVAL, "Failed to set counter"); - } + uint8_t *key = s->initiator ? s->key->key0 : s->key->key1; + + if(!chacha_poly1305_set_key(s->incipher, key)) { + return error(s, EINVAL, "Failed to set counter"); } - free(s->key); + free_sptps_key(s->key); s->key = NULL; s->instate = true; @@ -281,24 +290,21 @@ static bool receive_ack(sptps_t *s, const uint8_t *data, uint16_t len) { // Receive a Key EXchange record, respond by sending a SIG record. static bool receive_kex(sptps_t *s, const uint8_t *data, uint16_t len) { // Verify length of the HELLO record - if(len != 1 + 32 + ECDH_SIZE) { + if(len != sizeof(sptps_kex_t)) { return error(s, EIO, "Invalid KEX record length"); } - // Ignore version number for now. + if(*data != SPTPS_VERSION) { + return error(s, EINVAL, "Received incorrect version %d", *data); + } // Make a copy of the KEX message, send_sig() and receive_sig() need it if(s->hiskex) { return error(s, EINVAL, "Received a second KEX message before first has been processed"); } - s->hiskex = realloc(s->hiskex, len); - - if(!s->hiskex) { - return error(s, errno, strerror(errno)); - } - - memcpy(s->hiskex, data, len); + s->hiskex = new_sptps_kex(); + memcpy(s->hiskex, data, sizeof(sptps_kex_t)); if(s->initiator) { return send_sig(s); @@ -309,22 +315,15 @@ static bool receive_kex(sptps_t *s, const uint8_t *data, uint16_t len) { // Receive a SIGnature record, verify it, if it passed, compute the shared secret and calculate the session keys. static bool receive_sig(sptps_t *s, const uint8_t *data, uint16_t len) { - size_t keylen = ECDH_SIZE; - size_t siglen = ecdsa_size(s->hiskey); - // Verify length of KEX record. - if(len != siglen) { + if(len != ecdsa_size(s->hiskey)) { return error(s, EIO, "Invalid KEX record length"); } // Concatenate both KEX messages, plus tag indicating if it is from the connection originator - const size_t msglen = (1 + 32 + keylen) * 2 + 1 + s->labellen; + const size_t msglen = sigmsg_len(s->labellen); uint8_t *msg = alloca(msglen); - - msg[0] = !s->initiator; - memcpy(msg + 1, s->hiskex, 1 + 32 + keylen); - memcpy(msg + 1 + 33 + keylen, s->mykex, 1 + 32 + keylen); - memcpy(msg + 1 + 2 * (33 + keylen), s->label, s->labellen); + fill_msg(msg, !s->initiator, s->hiskex, s->mykex, s); // Verify signature. if(!ecdsa_verify(s->hiskey, msg, msglen, data)) { @@ -334,14 +333,18 @@ static bool receive_sig(sptps_t *s, const uint8_t *data, uint16_t len) { // Compute shared secret. uint8_t shared[ECDH_SHARED_SIZE]; - if(!ecdh_compute_shared(s->ecdh, s->hiskex + 1 + 32, shared)) { + if(!ecdh_compute_shared(s->ecdh, s->hiskex->pubkey, shared)) { + memzero(shared, sizeof(shared)); return error(s, EINVAL, "Failed to compute ECDH shared secret"); } s->ecdh = NULL; // Generate key material from shared secret. - if(!generate_key_material(s, shared, sizeof(shared))) { + bool generated = generate_key_material(s, shared, sizeof(shared)); + memzero(shared, sizeof(shared)); + + if(!generated) { return false; } @@ -349,10 +352,10 @@ static bool receive_sig(sptps_t *s, const uint8_t *data, uint16_t len) { return false; } - free(s->mykex); - free(s->hiskex); - + free_sptps_kex(s->mykex); s->mykex = NULL; + + free_sptps_kex(s->hiskex); s->hiskex = NULL; // Send cipher change record @@ -361,14 +364,10 @@ static bool receive_sig(sptps_t *s, const uint8_t *data, uint16_t len) { } // TODO: only set new keys after ACK has been set/received - if(s->initiator) { - if(!chacha_poly1305_set_key(s->outcipher, s->key + CHACHA_POLY1305_KEYLEN)) { - return error(s, EINVAL, "Failed to set key"); - } - } else { - if(!chacha_poly1305_set_key(s->outcipher, s->key)) { - return error(s, EINVAL, "Failed to set key"); - } + uint8_t *key = s->initiator ? s->key->key1 : s->key->key0; + + if(!chacha_poly1305_set_key(s->outcipher, key)) { + return error(s, EINVAL, "Failed to set key"); } return true; @@ -759,9 +758,9 @@ bool sptps_stop(sptps_t *s) { chacha_poly1305_exit(s->outcipher); ecdh_free(s->ecdh); free(s->inbuf); - free(s->mykex); - free(s->hiskex); - free(s->key); + free_sptps_kex(s->mykex); + free_sptps_kex(s->hiskex); + free_sptps_key(s->key); free(s->label); free(s->late); memset(s, 0, sizeof(*s)); diff --git a/src/sptps.h b/src/sptps.h index 96edc366..95209e5d 100644 --- a/src/sptps.h +++ b/src/sptps.h @@ -47,6 +47,26 @@ typedef enum sptps_state_t { SPTPS_ACK = 4, // Waiting for an ACKnowledgement record } sptps_state_t; +PACKED(struct sptps_kex_t { + uint8_t version; + uint8_t nonce[ECDH_SIZE]; + uint8_t pubkey[ECDH_SIZE]; +}); + +typedef struct sptps_kex_t sptps_kex_t; + +STATIC_ASSERT(sizeof(sptps_kex_t) == 65, "sptps_kex_t has invalid size"); + +typedef union sptps_key_t { + struct { + uint8_t key0[CHACHA_POLY1305_KEYLEN]; + uint8_t key1[CHACHA_POLY1305_KEYLEN]; + }; + uint8_t both[CHACHA_POLY1305_KEYLEN * 2]; +} sptps_key_t; + +STATIC_ASSERT(sizeof(sptps_key_t) == 128, "sptps_key_t has invalid size"); + typedef struct sptps { bool initiator; bool datagram; @@ -72,9 +92,9 @@ typedef struct sptps { ecdsa_t *hiskey; ecdh_t *ecdh; - uint8_t *mykex; - uint8_t *hiskex; - uint8_t *key; + sptps_kex_t *mykex; + sptps_kex_t *hiskex; + sptps_key_t *key; uint8_t *label; size_t labellen; diff --git a/src/sptps_keypair.c b/src/sptps_keypair.c index 17d26f93..7fcfee64 100644 --- a/src/sptps_keypair.c +++ b/src/sptps_keypair.c @@ -62,14 +62,14 @@ static int generate_keypair(char *argv[]) { if(fp) { if(!ecdsa_write_pem_private_key(key, fp)) { fprintf(stderr, "Could not write ECDSA private key\n"); - free(key); + ecdsa_free(key); return 1; } fclose(fp); } else { fprintf(stderr, "Could not open '%s' for writing: %s\n", argv[1], strerror(errno)); - free(key); + ecdsa_free(key); return 1; } @@ -80,12 +80,12 @@ static int generate_keypair(char *argv[]) { fprintf(stderr, "Could not write ECDSA public key\n"); } - free(key); + ecdsa_free(key); fclose(fp); return 0; } else { fprintf(stderr, "Could not open '%s' for writing: %s\n", argv[2], strerror(errno)); - free(key); + ecdsa_free(key); return 1; } } diff --git a/src/sptps_test.c b/src/sptps_test.c index 249f2e4f..acc692ad 100644 --- a/src/sptps_test.c +++ b/src/sptps_test.c @@ -543,14 +543,14 @@ static int run_test(int argc, char *argv[]) { if(!fp) { fprintf(stderr, "Could not open %s: %s\n", argv[2], strerror(errno)); - free(mykey); + ecdsa_free(mykey); return 1; } ecdsa_t *hiskey = NULL; if(!(hiskey = ecdsa_read_pem_public_key(fp))) { - free(mykey); + ecdsa_free(mykey); return 1; } @@ -563,8 +563,8 @@ static int run_test(int argc, char *argv[]) { sptps_t s; if(!sptps_start(&s, &sock, initiator, datagram, mykey, hiskey, "sptps_test", 10, send_data, receive_record)) { - free(mykey); - free(hiskey); + ecdsa_free(mykey); + ecdsa_free(hiskey); return 1; } @@ -575,8 +575,8 @@ static int run_test(int argc, char *argv[]) { if(in < 0) { fprintf(stderr, "Could not init stdin reader thread\n"); - free(mykey); - free(hiskey); + ecdsa_free(mykey); + ecdsa_free(hiskey); return 1; } } @@ -603,8 +603,8 @@ static int run_test(int argc, char *argv[]) { FD_SET(sock, &fds); if(select(max_fd + 1, &fds, NULL, NULL, NULL) <= 0) { - free(mykey); - free(hiskey); + ecdsa_free(mykey); + ecdsa_free(hiskey); return 1; } @@ -617,8 +617,8 @@ static int run_test(int argc, char *argv[]) { if(len < 0) { fprintf(stderr, "Could not read from stdin: %s\n", strerror(errno)); - free(mykey); - free(hiskey); + ecdsa_free(mykey); + ecdsa_free(hiskey); return 1; } @@ -649,8 +649,8 @@ static int run_test(int argc, char *argv[]) { sptps_send_record(&s, 0, buf, len); } } else if(!sptps_send_record(&s, buf[0] == '!' ? 1 : 0, buf, (len == 1 && buf[0] == '\n') ? 0 : buf[0] == '*' ? sizeof(buf) : (size_t)len)) { - free(mykey); - free(hiskey); + ecdsa_free(mykey); + ecdsa_free(hiskey); return 1; } } @@ -660,8 +660,8 @@ static int run_test(int argc, char *argv[]) { if(len < 0) { fprintf(stderr, "Could not read from socket: %s\n", sockstrerror(sockerrno)); - free(mykey); - free(hiskey); + ecdsa_free(mykey); + ecdsa_free(hiskey); return 1; } @@ -691,8 +691,8 @@ static int run_test(int argc, char *argv[]) { if(!done) { if(!datagram) { - free(mykey); - free(hiskey); + ecdsa_free(mykey); + ecdsa_free(hiskey); return 1; } } @@ -705,8 +705,8 @@ static int run_test(int argc, char *argv[]) { bool stopped = sptps_stop(&s); - free(mykey); - free(hiskey); + ecdsa_free(mykey); + ecdsa_free(hiskey); closesocket(sock); return !stopped; diff --git a/src/utils.c b/src/utils.c index c2f4333f..c395169d 100644 --- a/src/utils.c +++ b/src/utils.c @@ -18,6 +18,8 @@ 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include + #include "logger.h" #include "system.h" #include "utils.h" diff --git a/src/utils.h b/src/utils.h index be629ee0..08180477 100644 --- a/src/utils.h +++ b/src/utils.h @@ -26,6 +26,7 @@ #include "crypto.h" #define B64_SIZE(len) ((len) * 4 / 3 + 5) +#define HEX_SIZE(len) ((len) * 2 + 1) extern size_t hex2bin(const char *src, void *dst, size_t length); extern size_t bin2hex(const void *src, char *dst, size_t length); diff --git a/src/xalloc.h b/src/xalloc.h index 96b9592b..c8cfbebf 100644 --- a/src/xalloc.h +++ b/src/xalloc.h @@ -23,6 +23,8 @@ #include "system.h" +#include + static inline void *xmalloc(size_t n) ATTR_MALLOC; static inline void *xmalloc(size_t n) { void *p = malloc(n); @@ -96,4 +98,51 @@ static inline int xasprintf(char **strp, const char *fmt, ...) { return result; } +// Zero out a block of memory containing sensitive information using whatever secure +// erase function is available on the platform (or an unreliable fallback if none are). +// The pointer must not be NULL. Length can be zero, in which case the call is a noop. +static inline void memzero(void *buf, size_t buflen) ATTR_NONNULL; +static inline void memzero(void *buf, size_t buflen) { + assert(buf); + + if(!buflen) { + return; + } + +#if defined(HAVE_EXPLICIT_BZERO) + explicit_bzero(buf, buflen); +#elif defined(HAVE_EXPLICIT_MEMSET) + explicit_memset(buf, 0, buflen); +#elif defined(HAVE_MEMSET_S) + errno_t err = memset_s(buf, buflen, 0, buflen); + assert(err == 0); +#elif defined(HAVE_WINDOWS) + SecureZeroMemory(buf, buflen); +#else + volatile uint8_t *p = buf; + + while(buflen--) { + *p++ = 0; + } + #endif +} + +// Zero out a buffer of size `len` located at `ptr` and free() it. +// Does nothing if called on NULL. +static inline void xzfree(void *ptr, size_t len) { + if(ptr) { + memzero(ptr, len); + free(ptr); + } +} + +// Zero out a NULL-terminated string using memzero() and then free it. +// Does nothing if called on NULL. +static inline void free_string(char *str) { + if(str) { + xzfree(str, strlen(str)); + } +} + +#endif // TINC_XALLOC_H diff --git a/test/unit/meson.build b/test/unit/meson.build index bebd0937..eaa9d181 100644 --- a/test/unit/meson.build +++ b/test/unit/meson.build @@ -47,6 +47,13 @@ tests = { 'utils': { 'code': 'test_utils.c', }, + 'xalloc': { + 'code': 'test_xalloc.c', + }, + 'memzero_null': { + 'code': 'test_memzero_null.c', + 'fail': true, + }, 'splay_tree': { 'code': 'test_splay_tree.c', 'link': link_tinc, diff --git a/test/unit/test_memzero_null.c b/test/unit/test_memzero_null.c new file mode 100644 index 00000000..b7ae136f --- /dev/null +++ b/test/unit/test_memzero_null.c @@ -0,0 +1,12 @@ +// Test that memzero() with NULL pointer crashes the program + +#include "config.h" +#undef HAVE_ATTR_NONNULL + +#include "unittest.h" +#include "../../src/xalloc.h" + +int main(void) { + memzero(NULL, 1); + return 0; +} diff --git a/test/unit/test_random_noinit.c b/test/unit/test_random_noinit.c index d02f4532..aeedda95 100644 --- a/test/unit/test_random_noinit.c +++ b/test/unit/test_random_noinit.c @@ -9,14 +9,8 @@ int main(void) { #else #include "../../src/random.h" -static void on_abort(int sig) { - (void)sig; - exit(1); -} - int main(void) { - signal(SIGABRT, on_abort); - u_int8_t buf[16]; + uint8_t buf[16]; randomize(buf, sizeof(buf)); return 0; } diff --git a/test/unit/test_xalloc.c b/test/unit/test_xalloc.c new file mode 100644 index 00000000..08f7a726 --- /dev/null +++ b/test/unit/test_xalloc.c @@ -0,0 +1,61 @@ +#include "unittest.h" +#include "../../src/xalloc.h" + +static const uint8_t ref[] = {0, 1, 2, 3, 4, 5, 6, 7}; + +static void test_memzero_wipes_partial(void **state) { + (void)state; + + uint8_t buf[sizeof(ref)]; + memcpy(buf, ref, sizeof(buf)); + + const size_t len = 2; + memzero(buf, len); + assert_int_equal(0, buf[0]); + assert_int_equal(0, buf[1]); + + assert_memory_equal(&buf[len], &ref[len], sizeof(ref) - len); +} + +static void test_memzero_wipes_buffer(void **state) { + (void)state; + + uint8_t buf[sizeof(ref)]; + uint8_t zero[sizeof(ref)] = {0}; + + memcpy(buf, ref, sizeof(buf)); + assert_memory_equal(ref, buf, sizeof(buf)); + + memzero(buf, sizeof(buf)); + assert_memory_not_equal(buf, ref, sizeof(buf)); + assert_memory_equal(zero, buf, sizeof(buf)); +} + +static void test_memzero_zerolen_does_not_change_memory(void **state) { + (void)state; + + uint8_t buf[sizeof(ref)]; + + memcpy(buf, ref, sizeof(buf)); + assert_memory_equal(ref, buf, sizeof(buf)); + + memzero(buf, 0); + assert_memory_equal(ref, buf, sizeof(buf)); +} + +// This test will fail under ASAN if xzfree forgets to call free() or overflows the buffer +static void test_xzfree_frees_memory(void **state) { + (void)state; + + xzfree(xmalloc(64), 64); +} + +int main(void) { + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_memzero_wipes_partial), + cmocka_unit_test(test_memzero_wipes_buffer), + cmocka_unit_test(test_memzero_zerolen_does_not_change_memory), + cmocka_unit_test(test_xzfree_frees_memory), + }; + return cmocka_run_group_tests(tests, NULL, NULL); +} -- 2.20.1