From bf6c791f9702b43fb7325fffddfacc902607d68d Mon Sep 17 00:00:00 2001 From: Antony Vennard Date: Mon, 6 Jan 2020 17:27:36 +0100 Subject: [PATCH] Tidy up a number of uglies in the code, remove some magic consts, make++ This code: * Adds NULL guard checks. * Detects mlen overflow in encrypt. * Allows you to specify CSTD=c89, CSTD=c99, CSTD=c11 for the library. * Supports E4_OUTPUT_DIR for building code anywhere. --- include/e4/e4.h | 18 +++++++-- include/e4/stdint.h | 1 + mk/conf.mk | 21 ++++++++++ mk/pubkey/conf.mk | 35 +++++------------ mk/pubkey/rules.mk | 4 +- mk/symkey/conf.mk | 35 ++++++----------- mk/symkey/rules.mk | 4 +- mk/unix.mk | 2 + src/e4c_pk_store_file.c | 2 +- src/e4c_pk_store_mem.c | 2 +- src/e4pkcclient.c | 59 +++++++++++++++++++++-------- src/e4symclient.c | 35 ++++++++++++++--- test/pubkey/pubkey_e4cmd_test.c | 7 ++-- test/pubkey/pubkey_filestore_test.c | 6 +-- 14 files changed, 142 insertions(+), 89 deletions(-) diff --git a/include/e4/e4.h b/include/e4/e4.h index 0d9c4bf..9a7c345 100644 --- a/include/e4/e4.h +++ b/include/e4/e4.h @@ -32,6 +32,9 @@ extern "C" { #define E4_RESULT_OK 0 /* A control message was handled. Applications may discard the resulting buffer */ #define E4_RESULT_OK_CONTROL -1 + +/* Internal error: for exception conditions that indicate the code has a bug */ +#define E4_ERROR_INTERNAL -100 /* Invalid authentication tag indicates corrupted ciphertext */ #define E4_ERROR_INVALID_TAG -101 /* Message received outside of error window for clock. */ @@ -48,15 +51,18 @@ extern "C" { #define E4_ERROR_INVALID_COMMAND -107 /* E4 Persistence layer reported an error. */ #define E4_ERROR_PERSISTENCE_ERROR -108 - /* Unable to find public key for device;e4 */ #define E4_ERROR_DEVICEPK_MISSING -109 /* Signature verification failed */ #define E4_ERROR_PK_SIGVERIF_FAILED -110 - +/* Overflow detected */ +#define E4_ERROR_PARAMETER_OVERFLOW -111 +/* Invalid parameters, e.g. NULL pointers */ +#define E4_ERROR_PARAMETER_INVALID -112 /* Size of the timestamp field */ #define E4_TS_LEN 8 +#define E4_TIMESTAMP_LEN 8 /* Size of the ID, truncated sha3(alias) */ #define E4_ID_LEN 16 @@ -70,7 +76,7 @@ extern "C" { #define E4_CTRLTOPIC_LEN (2 * E4_ID_LEN) + 3 #define E4_TAG_LEN 16 -#define E4_TIMESTAMP_LEN 8 + #define E4_MSGHDR_LEN (E4_TAG_LEN + E4_TIMESTAMP_LEN) /* Public key support */ @@ -152,7 +158,8 @@ int e4c_unprotect_message(uint8_t *message, const uint8_t *ciphertext, size_t ciphertext_len, const char *topic_name, - e4storage *storage); + e4storage *storage + ); /* the e4storage type pre-defined above implements these API calls */ @@ -168,10 +175,13 @@ int e4c_set_topic_key(e4storage *store, const uint8_t *topic_hash, const uint8_t int e4c_remove_topic(e4storage *store, const uint8_t *topic_hash); int e4c_reset_topics(e4storage *store); +#ifdef E4_MODE_SYMKEY int e4c_set_idkey(e4storage *store, const uint8_t *key); +#endif #ifdef E4_MODE_PUBKEY /* pubkey storage apis */ int e4c_set_idpubkey(e4storage *store, const uint8_t *pubkey); +int e4c_set_idseckey(e4storage *store, const uint8_t *key); int e4c_getdeviceindex(e4storage *store, const uint8_t* id); int e4c_getdevicekey(uint8_t* key, e4storage *store, const int index); int e4c_set_device_key(e4storage *store, const uint8_t *id, const uint8_t *key); diff --git a/include/e4/stdint.h b/include/e4/stdint.h index bf6adb2..4222c56 100644 --- a/include/e4/stdint.h +++ b/include/e4/stdint.h @@ -23,6 +23,7 @@ extern "C" { #endif #if __STDC_VERSION__ >= 199901L + #include #include #else #include "e4/pstdint.h" diff --git a/mk/conf.mk b/mk/conf.mk index e799b7c..063ea7e 100644 --- a/mk/conf.mk +++ b/mk/conf.mk @@ -1,4 +1,25 @@ VERSION=1.0.0 +CC ?= clang +AR ?= ar +LD ?= clang +ARFLAGS = rcs +CFLAGS = -Wall -Werror -fPIC -std=$(CSTD) $(E4_CFLAGS) +LDFLAGS = -L. $(E4_LDFLAGS) + +O = o + +TESTCFLAGS = -Wall -Werror -g -std=c11 $(E4_CFLAGS) +TESTLDFLAGS= $(E4_LDFLAGS) + +GITCOMMIT=$(shell git rev-list -1 HEAD) +NOW=$(shell date "+%Y%m%d%H%M") + +INCDIR = include +SRCDIR = src +DOCDIR = doc + +LIBNAME = libe4 + diff --git a/mk/pubkey/conf.mk b/mk/pubkey/conf.mk index dac7854..4315dd0 100644 --- a/mk/pubkey/conf.mk +++ b/mk/pubkey/conf.mk @@ -1,34 +1,19 @@ -CC ?= clang -AR ?= ar -LD ?= clang -ARFLAGS = rcs -CFLAGS = -Wall -Werror -fPIC -std=c89 $(E4_CFLAGS) -LDFLAGS = -L. $(E4_CFLAGS) -LDSOFLAGS = -shared -fPIC -Wl,-soname,libe4p.so.1 -INCLUDES = -Iinclude/ -Ibuild/pubkey/include/ +E4_OUTPUT_DIR ?= build/pubkey -# BUILD environment -GITCOMMIT=$(shell git rev-list -1 HEAD) -NOW=$(shell date "+%Y%m%d%H%M") +LDSOFLAGS = -shared -fPIC -Wl,-soname,libe4p.so.1 +INCLUDES = -Iinclude/ -I$(E4_OUTPUT_DIR)/include/ # OBJ paths match their src folder equivalents -INCDIR = include OBJDIR = tmp/pubkey/build TESTOBJDIR = tmp/pubkey/test -SRCDIR = src -DOCDIR = doc -BUILDDIR = build/pubkey -LIBDIR = build/pubkey/lib -OUTINCDIR = build/pubkey/include -LIBNAME = libe4 -LIB = $(LIBDIR)/$(LIBNAME).a -LIBSO = $(LIBDIR)/$(LIBNAME)p.so.$(VERSION) + +BUILDDIR = $(E4_OUTPUT_DIR) +LIBDIR = $(BUILDDIR)/lib +OUTINCDIR = $(BUILDDIR)/include DISTDIR = dist/pubkey/ -TESTDIR = build/pubkey/test +TESTDIR = $(BUILDDIR)/test -O = o +LIB = $(LIBDIR)/$(LIBNAME).a +LIBSO = $(LIBDIR)/$(LIBNAME)p.so.$(VERSION) -# test specific parts: -TESTCFLAGS = -Wall -Werror -g -std=c11 -Wno-unused-variable $(E4_CFLAGS) -TESTLDFLAGS = $(LDFLAGS) diff --git a/mk/pubkey/rules.mk b/mk/pubkey/rules.mk index f4c4eaa..4bcc955 100644 --- a/mk/pubkey/rules.mk +++ b/mk/pubkey/rules.mk @@ -9,9 +9,9 @@ pub_so: setup pub_header $(OBJS) cp -rfv $(INCDIR)/* $(OUTINCDIR)/; \ $(CC) $(LDSOFLAGS) $(OBJS) -o $(LIBSO) -.PHONY pub_header: build/pubkey/include/e4config/e4_config.h +.PHONY pub_header: $(BUILDDIR)/include/e4config/e4_config.h -build/pubkey/include/e4config/e4_config.h: +$(BUILDDIR)/include/e4config/e4_config.h: echo '#define E4_MODE_PUBKEY 1' > $@ ifeq ("$(STORE)", "mem") echo "#define E4_STORE_MEM 1" >> $@ diff --git a/mk/symkey/conf.mk b/mk/symkey/conf.mk index 3eaf075..0d9efb0 100644 --- a/mk/symkey/conf.mk +++ b/mk/symkey/conf.mk @@ -1,33 +1,20 @@ -CC ?= clang -AR ?= ar -ARFLAGS = rcs -CFLAGS = -Wall -fPIC -Werror -std=c89 $(E4_CFLAGS) -LDFLAGS = -L. $(E4_CFLAGS) -LDSOFLAGS = -shared -fPIC -Wl,-soname,libe4s.so.1 -INCLUDES = -Iinclude/ -Ibuild/symkey/include/ +E4_OUTPUT_DIR ?= build/symkey -# BUILD environment -GITCOMMIT=$(shell git rev-list -1 HEAD) -NOW=$(shell date "+%Y%m%d%H%M") +LDSOFLAGS = -shared -fPIC -Wl,-soname,libe4s.so.1 +INCLUDES = -Iinclude/ -I$(E4_OUTPUT_DIR)/include/ # OBJ paths match their src folder equivalents -INCDIR = include OBJDIR = tmp/symkey/build TESTOBJDIR = tmp/symkey/test -SRCDIR = src -DOCDIR = doc -BUILDDIR = build/symkey -LIBDIR = build/symkey/lib -OUTINCDIR = build/symkey/include -LIBNAME = libe4 -LIB = $(LIBDIR)/$(LIBNAME).a -LIBSO = $(LIBDIR)/$(LIBNAME).so.$(VERSION) + +BUILDDIR = $(E4_OUTPUT_DIR) +LIBDIR = $(BUILDDIR)/lib +OUTINCDIR = $(BUILDDIR)/include DISTDIR = dist/symkey/ -TESTDIR = build/symkey/test +TESTDIR = $(BUILDDIR)/test + +LIB = $(LIBDIR)/$(LIBNAME).a +LIBSO = $(LIBDIR)/$(LIBNAME)s.so.$(VERSION) -O = o -# test specific parts: -TESTCFLAGS = -Wall -Werror -g -std=c11 $(E4_CFLAGS) -TESTLDFLAGS = diff --git a/mk/symkey/rules.mk b/mk/symkey/rules.mk index bb63018..fffed0d 100644 --- a/mk/symkey/rules.mk +++ b/mk/symkey/rules.mk @@ -9,9 +9,9 @@ sym_so: setup sym_header $(OBJS) cp -rfv $(INCDIR)/* $(OUTINCDIR)/; \ $(CC) $(LDSOFLAGS) $(OBJS) -o $(LIBSO) -.PHONY sym_header: build/symkey/include/e4config/e4_config.h +.PHONY sym_header: $(BUILDDIR)/include/e4config/e4_config.h -build/symkey/include/e4config/e4_config.h: +$(BUILDDIR)/include/e4config/e4_config.h: echo '#define E4_MODE_SYMKEY 1' > $@ ifeq ("$(STORE)", "mem") echo "#define E4_STORE_MEM 1" >> $@ diff --git a/mk/unix.mk b/mk/unix.mk index 7f02836..4c758f7 100644 --- a/mk/unix.mk +++ b/mk/unix.mk @@ -4,6 +4,8 @@ include mk/conf.mk CONF ?= symkey STORE ?= file +CSTD ?= -std=c89 + include mk/$(CONF)/conf.mk include mk/$(CONF)/objects.mk include mk/$(CONF)/tests.mk diff --git a/src/e4c_pk_store_file.c b/src/e4c_pk_store_file.c index b3d2c6e..9d2c9a5 100644 --- a/src/e4c_pk_store_file.c +++ b/src/e4c_pk_store_file.c @@ -266,7 +266,7 @@ int e4c_set_id(e4storage *store, const uint8_t *id) exit: return r; } -int e4c_set_idkey(e4storage *store, const uint8_t *key) +int e4c_set_idseckey(e4storage *store, const uint8_t *key) { memmove(store->privkey, key, sizeof store->privkey); e4c_sync(store); diff --git a/src/e4c_pk_store_mem.c b/src/e4c_pk_store_mem.c index 5101528..35dbca4 100644 --- a/src/e4c_pk_store_mem.c +++ b/src/e4c_pk_store_mem.c @@ -73,7 +73,7 @@ int e4c_set_id(e4storage *store, const uint8_t *id) return r; } -int e4c_set_idkey(e4storage *store, const uint8_t *key) +int e4c_set_idseckey(e4storage *store, const uint8_t *key) { memmove(store->privkey, key, sizeof store->privkey); e4c_sync(store); diff --git a/src/e4pkcclient.c b/src/e4pkcclient.c index b366844..0e1271c 100644 --- a/src/e4pkcclient.c +++ b/src/e4pkcclient.c @@ -50,17 +50,30 @@ int e4c_protect_message(uint8_t *cptr, e4storage *storage) { int i = 0; - size_t clen2 = 0; + size_t clen_siv = 0; uint8_t key[E4_KEY_LEN]; uint64_t time_now = 0; uint8_t* signaturep = NULL; size_t signeddatalen=0; /* pubkey messages are: Timestamp (8) | id (16) | IV (16) | Ciphertext (n) | sig (64) */ - if (mlen + E4_PK_TOPICMSGHDR_LEN + E4_PK_EDDSA_SIG_LEN > cmax) /* actually: not enough space */ return E4_ERROR_CIPHERTEXT_TOO_SHORT; - *clen = mlen + E4_MSGHDR_LEN; + + if (mlen + E4_PK_TOPICMSGHDR_LEN + E4_PK_EDDSA_SIG_LEN < mlen) /* overflow */ + return E4_ERROR_PARAMETER_OVERFLOW; + + if (cptr == NULL || + mptr == NULL || + topic == NULL || + clen == NULL || + storage == NULL) + { + return E4_ERROR_PARAMETER_INVALID; + } + + *clen = mlen + E4_PK_TOPICMSGHDR_LEN; + /* get the key */ i = e4c_getindex(storage, topic); @@ -86,17 +99,15 @@ int e4c_protect_message(uint8_t *cptr, time_now >>= 8; } - /* set our ID in the output buffer - * if Avi is reading this code, hope you are enjoying this memcpy. - */ - memcpy(cptr + E4_TS_LEN, storage->id, E4_ID_LEN); + /* set our ID in the output buffer */ + memcpy(cptr + E4_TIMESTAMP_LEN, storage->id, E4_ID_LEN); /* encrypt */ - clen2 = 0; - aes256_encrypt_siv(cptr + E4_TS_LEN + E4_ID_LEN, &clen2, cptr, 8, mptr, mlen, key); + clen_siv = 0; + aes256_encrypt_siv(cptr + E4_TIMESTAMP_LEN + E4_ID_LEN, &clen_siv, cptr, 8, mptr, mlen, key); /* sign the result */ - signeddatalen = E4_TS_LEN + E4_ID_LEN + E4_TAG_LEN + mlen; + signeddatalen = E4_TIMESTAMP_LEN + E4_ID_LEN + E4_TAG_LEN + mlen; signaturep = &cptr[0] + signeddatalen; /* @@ -112,6 +123,13 @@ int e4c_protect_message(uint8_t *cptr, storage->pubkey, storage->privkey); + /* pubkey messages are: Timestamp (8) | id (16) | IV (16) | Ciphertext (n) | sig (64) */ + /* safety check. clen_siv = msglen+E4_TAG_LEN. Therefore adding E4_PK_EDDSA_SIG_LEN + E4_TIMESTAMP_LEN + E4_ID_LEN should + * equal clen */ + if ( *clen != clen_siv + E4_PK_EDDSA_SIG_LEN + E4_TIMESTAMP_LEN + E4_ID_LEN ) { + return E4_ERROR_INTERNAL; + } + return E4_RESULT_OK; } @@ -138,6 +156,15 @@ int e4c_unprotect_message(uint8_t *mptr, secs1970 = (uint64_t)time(NULL); / this system has a RTC */ #endif + + if (cptr == NULL || + mptr == NULL || + topic == NULL || + storage == NULL) + { + return E4_ERROR_PARAMETER_INVALID; + } + /* There are two possible message formats: @@ -175,8 +202,8 @@ int e4c_unprotect_message(uint8_t *mptr, /* set things up for symmetric decryption: */ /* From the C2: Timestamp (8) | IV (16) | Ciphertext (n) */ - assocdatalen = E4_TS_LEN; - sivpayloadlen = clen - E4_TS_LEN; + assocdatalen = E4_TIMESTAMP_LEN; + sivpayloadlen = clen - E4_TIMESTAMP_LEN; } else { @@ -234,8 +261,8 @@ int e4c_unprotect_message(uint8_t *mptr, /* set things up for symmetric decryption: */ /* From other clients: Timestamp (8) | id (16) | IV (16) | Ciphertext (n) | sig (64) */ - assocdatalen = E4_ID_LEN + E4_TS_LEN; - sivpayloadlen = clen - (E4_TS_LEN + E4_ID_LEN + E4_PK_EDDSA_SIG_LEN); + assocdatalen = E4_ID_LEN + E4_TIMESTAMP_LEN; + sivpayloadlen = clen - (E4_TIMESTAMP_LEN + E4_ID_LEN + E4_PK_EDDSA_SIG_LEN); } /* Retrieve timestamp encoded as little endian */ @@ -320,8 +347,8 @@ int e4c_unprotect_message(uint8_t *mptr, return r == 0 ? E4_RESULT_OK_CONTROL : r; case 0x02: /* SetIdKey(key) */ - if (*mlen != (1 + E4_KEY_LEN)) return E4_ERROR_INVALID_COMMAND; - r = e4c_set_idkey(storage, mptr + 1); + if (*mlen != (1 + E4_PK_EDDSA_PRIVKEY_LEN)) return E4_ERROR_INVALID_COMMAND; + r = e4c_set_idseckey(storage, mptr + 1); return r == 0 ? E4_RESULT_OK_CONTROL : r; case 0x03: /* SetTopicKey(topic, key) */ diff --git a/src/e4symclient.c b/src/e4symclient.c index 5aa2fc5..5ba06bd 100644 --- a/src/e4symclient.c +++ b/src/e4symclient.c @@ -46,13 +46,21 @@ int e4c_protect_message(uint8_t *cptr, e4storage *storage) { int i = 0; - size_t clen2 = 0; uint8_t key[E4_KEY_LEN]; uint64_t time_now = 0; if (mlen + E4_MSGHDR_LEN > cmax) /* actually: not enough space */ return E4_ERROR_CIPHERTEXT_TOO_SHORT; - *clen = mlen + E4_MSGHDR_LEN; + if (mlen + E4_MSGHDR_LEN < mlen) /* overflow */ + return E4_ERROR_PARAMETER_OVERFLOW; + + if (cptr == NULL || + mptr == NULL || + topic == NULL || + storage == NULL) + { + return E4_ERROR_PARAMETER_INVALID; + } /* get the key */ i = e4c_getindex(storage, topic); @@ -76,15 +84,21 @@ int e4c_protect_message(uint8_t *cptr, time_now = time(NULL); /* timestamp */ #endif - for (i = 0; i < 8; i++) + for (i = 0; i < E4_TIMESTAMP_LEN; i++) { cptr[i] = time_now & 0xFF; time_now >>= 8; } /* encrypt */ - clen2 = 0; - aes256_encrypt_siv(cptr + 8, &clen2, cptr, 8, mptr, mlen, key); + aes256_encrypt_siv(cptr + E4_TIMESTAMP_LEN, clen, cptr, E4_TIMESTAMP_LEN, mptr, mlen, key); + /* add associated data */ + *clen += E4_TIMESTAMP_LEN; + + /* safety check */ + if ( *clen != mlen + E4_MSGHDR_LEN ) { + return E4_ERROR_INTERNAL; + } return E4_RESULT_OK; } @@ -116,6 +130,15 @@ int e4c_unprotect_message(uint8_t *mptr, { return E4_ERROR_CIPHERTEXT_TOO_SHORT; } + + if (cptr == NULL || + mptr == NULL || + topic == NULL || + storage == NULL) + { + return E4_ERROR_PARAMETER_INVALID; + } + /* get the key */ i = e4c_getindex(storage, topic); @@ -143,7 +166,7 @@ int e4c_unprotect_message(uint8_t *mptr, } /* decrypt */ - if (aes256_decrypt_siv(mptr, mlen, cptr, 8, cptr + 8, clen - 8, key) != 0) + if (aes256_decrypt_siv(mptr, mlen, cptr, E4_TIMESTAMP_LEN, cptr + E4_TIMESTAMP_LEN, clen - E4_TIMESTAMP_LEN, key) != 0) { return E4_ERROR_INVALID_TAG; } diff --git a/test/pubkey/pubkey_e4cmd_test.c b/test/pubkey/pubkey_e4cmd_test.c index 6911439..fa668f3 100644 --- a/test/pubkey/pubkey_e4cmd_test.c +++ b/test/pubkey/pubkey_e4cmd_test.c @@ -19,8 +19,7 @@ const size_t SIZE_TOPICNAME = 8; int main(int argc, char** argv, char** envp) { int returncode = 0; int e4retcode = 0; - int iteration = 0; - size_t bytes_read = 0; + //size_t bytes_read = 0; FILE* urand_fd = NULL; e4storage store; @@ -56,9 +55,9 @@ int main(int argc, char** argv, char** envp) { goto exit_close; } - e4retcode = e4c_set_idkey(&store, pkkat[i].dev_edwards_seckey); + e4retcode = e4c_set_idseckey(&store, pkkat[i].dev_edwards_seckey); if ( e4retcode != 0 ) { - printf("Failed: unable to set idkey\n"); + printf("Failed: unable to set idseckey\n"); returncode = 1; goto exit_close; } diff --git a/test/pubkey/pubkey_filestore_test.c b/test/pubkey/pubkey_filestore_test.c index 70e9d82..7f12893 100644 --- a/test/pubkey/pubkey_filestore_test.c +++ b/test/pubkey/pubkey_filestore_test.c @@ -24,8 +24,6 @@ int main(int argc, char** argv, char** envp) { FILE* urand_fd = NULL; e4storage store; - unsigned char clientid[E4_ID_LEN]; - unsigned char clientkey[E4_KEY_LEN]; unsigned char topickey_current[E4_KEY_LEN]; unsigned char topicname_tmp[SIZE_TOPICNAME/2]; char topicname_current[SIZE_TOPICNAME+1]; @@ -58,9 +56,9 @@ int main(int argc, char** argv, char** envp) { goto exit_close; } - e4retcode = e4c_set_idkey(&store, pkkat[0].dev_edwards_seckey); + e4retcode = e4c_set_idseckey(&store, pkkat[0].dev_edwards_seckey); if ( e4retcode != 0 ) { - printf("Failed: unable to set idkey\n"); + printf("Failed: unable to set idseckey\n"); returncode = 1; goto exit_close; }