Merge pull request #36 from hamishcoleman/main

Bugs and leaks
n42n · May 20, 2024 · 67078b1 · 67078b1
2 parents 47e128b + e60fc70
commit 67078b1
Show file tree

Hide file tree

Showing 9 changed files with 69 additions and 43 deletions.
diff --git a/apps/n3n-edge.c b/apps/n3n-edge.c
@@ -794,8 +794,8 @@ int main (int argc, char* argv[]) {
                 generate_public_key(*conf.public_key, *(conf.shared_secret));
             generate_shared_secret(*(conf.shared_secret), *(conf.shared_secret), *(conf.federation_public_key));
             // prepare (first 128 bit) for use as key
-            conf.shared_secret_ctx = (he_context_t*)calloc(1, sizeof(speck_context_t));
-            speck_init((speck_context_t**)&(conf.shared_secret_ctx), *(conf.shared_secret), 128);
+            conf.shared_secret_ctx = calloc(1, sizeof(*conf.shared_secret_ctx));
+            speck_init(&conf.shared_secret_ctx, *(conf.shared_secret), 128);
         }
         // force header encryption
         if(conf.header_encryption != HEADER_ENCRYPTION_ENABLED) {

diff --git a/include/header_encryption.h b/include/header_encryption.h
@@ -17,20 +17,25 @@
  */
 
 #include "n2n_typedefs.h"
+#include "speck.h"          // for struct speck_context_t
 
 int packet_header_decrypt (uint8_t packet[], uint16_t packet_len,
                            char *community_name,
-                           he_context_t *ctx, he_context_t *ctx_iv,
+                           struct speck_context_t *ctx,
+                           struct speck_context_t *ctx_iv,
                            uint64_t *stamp);
 
 int packet_header_encrypt (uint8_t packet[], uint16_t header_len, uint16_t packet_len,
-                           he_context_t *ctx, he_context_t *ctx_iv,
+                           struct speck_context_t *ctx,
+                           struct speck_context_t *ctx_iv,
                            uint64_t stamp);
 
 void packet_header_setup_key (const char *community_name,
-                              he_context_t **ctx_static, he_context_t **ctx_dynamic,
-                              he_context_t **ctx_iv_static, he_context_t **ctx_iv_dynamic);
+                              struct speck_context_t **ctx_static,
+                              struct speck_context_t **ctx_dynamic,
+                              struct speck_context_t **ctx_iv_static,
+                              struct speck_context_t **ctx_iv_dynamic);
 
 void packet_header_change_dynamic_key (uint8_t *key_dynamic,
-                                       he_context_t **ctx_dynamic,
-                                       he_context_t **ctx_iv_dynamic);
+                                       struct speck_context_t **ctx_dynamic,
+                                       struct speck_context_t **ctx_iv_dynamic);
diff --git a/include/n2n_typedefs.h b/include/n2n_typedefs.h
@@ -36,6 +36,8 @@
 
 #include <config.h>     // for HAVE_LIBZSTD
 
+#include "speck.h"      // for struct speck_context_t
+
 typedef char n2n_community_t[N2N_COMMUNITY_SIZE];
 typedef uint8_t n2n_private_public_key_t[N2N_PRIVATE_PUBLIC_KEY_SIZE];
 typedef uint32_t n2n_cookie_t;
@@ -135,7 +137,6 @@ typedef u_short sa_family_t;
 #endif
 
 
-typedef struct speck_context_t he_context_t;
 typedef char n2n_sn_name_t[N2N_EDGE_SN_HOST_SIZE];
 
 typedef enum n2n_pc {
@@ -450,12 +451,12 @@ typedef struct n2n_edge_conf {
     bool allow_p2p;                                  /**< Allow P2P connection */
     n2n_private_public_key_t *public_key;            /**< edge's public key (for user/password based authentication) */
     n2n_private_public_key_t *shared_secret;         /**< shared secret derived from federation public key, username and password */
-    he_context_t             *shared_secret_ctx;     /**< context holding the roundkeys derived from shared secret */
+    speck_context_t *shared_secret_ctx;              /**< context holding the roundkeys derived from shared secret */
     n2n_private_public_key_t *federation_public_key; /**< federation public key provided by command line */
-    he_context_t     *header_encryption_ctx_static;  /**< Header encryption cipher context. */
-    he_context_t     *header_encryption_ctx_dynamic; /**< Header encryption cipher context. */
-    he_context_t             *header_iv_ctx_static;  /**< Header IV ecnryption cipher context, REMOVE as soon as separate fileds for checksum and replay protection available */
-    he_context_t             *header_iv_ctx_dynamic; /**< Header IV ecnryption cipher context, REMOVE as soon as separate fileds for checksum and replay protection available */
+    struct speck_context_t *header_encryption_ctx_static;  /**< Header encryption cipher context. */
+    struct speck_context_t *header_encryption_ctx_dynamic; /**< Header encryption cipher context. */
+    struct speck_context_t *header_iv_ctx_static;    /**< Header IV ecnryption cipher context, REMOVE as soon as separate fileds for checksum and replay protection available */
+    struct speck_context_t *header_iv_ctx_dynamic;   /**< Header IV ecnryption cipher context, REMOVE as soon as separate fileds for checksum and replay protection available */
     uint8_t header_encryption;                       /**< Header encryption indicator. */
     uint8_t transop_id;                              /**< The transop to use. */
     uint8_t compression;                             /**< Compress outgoing data packets before encryption */
@@ -616,7 +617,7 @@ typedef struct node_supernode_association {
 typedef struct sn_user {
     n2n_private_public_key_t public_key;
     n2n_private_public_key_t shared_secret;
-    he_context_t               *shared_secret_ctx;
+    struct speck_context_t *shared_secret_ctx;
     n2n_desc_t name;
 
     UT_hash_handle hh;
@@ -627,10 +628,10 @@ struct sn_community {
     bool is_federation;                                /* if true, then the current community is the federation of supernodes */
     bool purgeable;                                       /* indicates purgeable community (fixed-name, predetermined (-c parameter) communties usually are unpurgeable) */
     uint8_t header_encryption;                            /* Header encryption indicator. */
-    he_context_t          *header_encryption_ctx_static;  /* Header encryption cipher context. */
-    he_context_t          *header_encryption_ctx_dynamic; /* Header encryption cipher context. */
-    he_context_t                  *header_iv_ctx_static;  /* Header IV encryption cipher context, REMOVE as soon as separate fields for checksum and replay protection available */
-    he_context_t                  *header_iv_ctx_dynamic; /* Header IV encryption cipher context, REMOVE as soon as separate fields for checksum and replay protection available */
+    struct speck_context_t *header_encryption_ctx_static;  /* Header encryption cipher context. */
+    struct speck_context_t *header_encryption_ctx_dynamic; /* Header encryption cipher context. */
+    struct speck_context_t *header_iv_ctx_static;          /* Header IV encryption cipher context, REMOVE as soon as separate fields for checksum and replay protection available */
+    struct speck_context_t *header_iv_ctx_dynamic;         /* Header IV encryption cipher context, REMOVE as soon as separate fields for checksum and replay protection available */
     uint8_t dynamic_key[N2N_AUTH_CHALLENGE_SIZE];                       /* dynamic key */
     struct                        peer_info *edges;       /* Link list of registered edges. */
     node_supernode_association_t  *assoc;                 /* list of other edges from this community and their supernodes */

diff --git a/src/auth.c b/src/auth.c
@@ -172,7 +172,6 @@ int calculate_dynamic_key (uint8_t out_key[N2N_AUTH_CHALLENGE_SIZE],
 
     memxor(key, tmp, N2N_AUTH_CHALLENGE_SIZE);
 
-    ctx = (speck_context_t*)calloc(1, sizeof(speck_context_t));
     speck_init((speck_context_t**)&ctx, key, 128);
 
     pearson_hash_128(tmp, (uint8_t*)&key_time, sizeof(key_time));
@@ -181,7 +180,7 @@ int calculate_dynamic_key (uint8_t out_key[N2N_AUTH_CHALLENGE_SIZE],
 
     speck_128_encrypt(out_key, ctx);
 
-    free(ctx);
+    speck_deinit(ctx);
 
     return 0;
 }
diff --git a/src/edge_utils.c b/src/edge_utils.c
@@ -252,6 +252,7 @@ static int detect_local_ip_address (n2n_sock_t* out_sock, const struct n3n_runti
     SOCKET probe_sock;
     int ret = 0;
 
+    memset(out_sock, 0, sizeof(*out_sock));
     out_sock->family = AF_INVALID;
 
     // always detect local port even/especially if chosen by OS...
@@ -406,7 +407,6 @@ int supernode_connect (struct n3n_runtime_data *eee) {
         traceEvent(TRACE_INFO, "No platform support for setting pmtu_discovery");
 #endif
 
-        memset(&local_sock, 0, sizeof(n2n_sock_t));
         if(detect_local_ip_address(&local_sock, eee) == 0) {
             // always overwrite local port even/especially if chosen by OS...
             eee->conf.preferred_sock.port = local_sock.port;
@@ -1305,6 +1305,7 @@ void send_register_super (struct n3n_runtime_data *eee) {
     n2n_REGISTER_SUPER_t reg;
     n2n_sock_str_t sockbuf;
 
+    // FIXME: fix encode_* functions to not need memsets
     memset(&cmn, 0, sizeof(cmn));
     memset(&reg, 0, sizeof(reg));
 
@@ -1359,6 +1360,7 @@ static void send_unregister_super (struct n3n_runtime_data *eee) {
     n2n_UNREGISTER_SUPER_t unreg;
     n2n_sock_str_t sockbuf;
 
+    // FIXME: fix encode_* functions to not need memsets
     memset(&cmn, 0, sizeof(cmn));
     memset(&unreg, 0, sizeof(unreg));
 
@@ -1456,6 +1458,7 @@ static void send_register (struct n3n_runtime_data * eee,
         return;
     }
 
+    // FIXME: fix encode_* functions to not need memsets
     memset(&cmn, 0, sizeof(cmn));
     memset(&reg, 0, sizeof(reg));
     cmn.ttl = N2N_DEFAULT_TTL;
@@ -1507,13 +1510,14 @@ static void send_register_ack (struct n3n_runtime_data * eee,
         return;
     }
 
+    // FIXME: fix encode_* functions to not need memsets
     memset(&cmn, 0, sizeof(cmn));
-    memset(&ack, 0, sizeof(reg));
     cmn.ttl = N2N_DEFAULT_TTL;
     cmn.pc = n2n_register_ack;
     cmn.flags = 0;
     memcpy(cmn.community, eee->conf.community_name, N2N_COMMUNITY_SIZE);
 
+    // FIXME: fix encode_* functions to not need memsets
     memset(&ack, 0, sizeof(ack));
     ack.cookie = reg->cookie;
     memcpy(ack.srcMac, eee->device.mac_addr, N2N_MAC_SIZE);
@@ -2093,12 +2097,14 @@ void edge_send_packet2net (struct n3n_runtime_data * eee,
     }
 #endif
 
+    // FIXME: fix encode_* functions to not need memsets
     memset(&cmn, 0, sizeof(cmn));
     cmn.ttl = N2N_DEFAULT_TTL;
     cmn.pc = n2n_packet;
     cmn.flags = 0; /* no options, not from supernode, no socket */
     memcpy(cmn.community, eee->conf.community_name, N2N_COMMUNITY_SIZE);
 
+    // FIXME: fix encode_* functions to not need memsets
     memset(&pkt, 0, sizeof(pkt));
     memcpy(pkt.srcMac, eee->device.mac_addr, N2N_MAC_SIZE);
     memcpy(pkt.dstMac, destMac, N2N_MAC_SIZE);
@@ -2286,12 +2292,12 @@ void process_udp (struct n3n_runtime_data *eee, const struct sockaddr *sender_so
     /* REVISIT: when UDP/IPv6 is supported we will need a flag to indicate which
      * IP transport version the packet arrived on. May need to UDP sockets. */
 
-    memset(&sender, 0, sizeof(n2n_sock_t));
-
     if(eee->conf.connect_tcp)
         // TCP expects that we know our comm partner and does not deliver the sender
-        memcpy(&sender, &(eee->curr_sn->sock), sizeof(struct sockaddr_in));
+        memcpy(&sender, &(eee->curr_sn->sock), sizeof(sender));
     else {
+        // FIXME: do not do random memset on the packet processing path
+        memset(&sender, 0, sizeof(sender));
         // REVISIT: type conversion back and forth, choose a consistent approach throughout whole code,
         //          i.e. stick with more general sockaddr as long as possible and narrow only if required
         fill_n2nsock(&sender, sender_sock);
@@ -2537,7 +2543,8 @@ void process_udp (struct n3n_runtime_data *eee, const struct sockaddr *sender_so
                     return;
                 }
 
-                memset(&ra, 0, sizeof(n2n_REGISTER_SUPER_ACK_t));
+                // FIXME: fix decode_* functions to not need memsets
+                memset(&ra, 0, sizeof(ra));
                 decode_REGISTER_SUPER_ACK(&ra, &cmn, udp_buf, &rem, &idx, tmpbuf);
 
                 if(eee->conf.header_encryption == HEADER_ENCRYPTION_ENABLED) {
@@ -2596,7 +2603,6 @@ void process_udp (struct n3n_runtime_data *eee, const struct sockaddr *sender_so
                 // from here on, 'sn' gets used differently
                 for(i = 0; i < ra.num_sn; i++) {
                     n2n_sock_t payload_sock;
-                    memset(&payload_sock, 0, sizeof(payload_sock));
                     skip_add = SN_ADD;
 
                     // bugfix for https://github.com/ntop/n2n/issues/1029
@@ -2672,7 +2678,8 @@ void process_udp (struct n3n_runtime_data *eee, const struct sockaddr *sender_so
                     return;
                 }
 
-                memset(&nak, 0, sizeof(n2n_REGISTER_SUPER_NAK_t));
+                // FIXME: fix decode_* functions to not need memsets
+                memset(&nak, 0, sizeof(nak));
                 decode_REGISTER_SUPER_NAK(&nak, &cmn, udp_buf, &rem, &idx);
 
                 if(eee->conf.header_encryption == HEADER_ENCRYPTION_ENABLED) {

diff --git a/src/header_encryption.c b/src/header_encryption.c
@@ -25,9 +25,9 @@
 #include <stdlib.h>             // for calloc
 #include <string.h>             // for memcpy
 #include "header_encryption.h"  // for packet_header_change_dynamic_key, pac...
-#include "n2n.h"                // for he_context_t, N2N_COMMUNITY_SIZE...
+#include "n2n.h"                // for N2N_COMMUNITY_SIZE...
 #include "n2n_define.h"         // for N2N_COMMUNITY_SIZE
-#include "n2n_typedefs.h"       // for he_context_t, N2N_AUTH_CHALLENGE_SIZE
+#include "n2n_typedefs.h"       // for N2N_AUTH_CHALLENGE_SIZE
 #include "pearson.h"            // for pearson_hash_128, pearson_hash_64
 #include "portable_endian.h"    // for htobe32, be32toh, be64toh, htobe64
 #include "speck.h"              // for speck_init, speck_context_t, speck_ctr
@@ -39,7 +39,8 @@
 
 int packet_header_decrypt (uint8_t packet[], uint16_t packet_len,
                            char *community_name,
-                           he_context_t *ctx, he_context_t *ctx_iv,
+                           struct speck_context_t *ctx,
+                           struct speck_context_t *ctx_iv,
                            uint64_t *stamp) {
 
     // try community name as possible key and check for magic bytes "n2__"
@@ -93,7 +94,8 @@ int packet_header_decrypt (uint8_t packet[], uint16_t packet_len,
 
 
 int packet_header_encrypt (uint8_t packet[], uint16_t header_len, uint16_t packet_len,
-                           he_context_t *ctx, he_context_t *ctx_iv,
+                           struct speck_context_t *ctx,
+                           struct speck_context_t *ctx_iv,
                            uint64_t stamp) {
 
     uint32_t *p32 = (uint32_t*)packet;
@@ -135,8 +137,10 @@ int packet_header_encrypt (uint8_t packet[], uint16_t header_len, uint16_t packe
 
 
 void packet_header_setup_key (const char *community_name,
-                              he_context_t **ctx_static, he_context_t **ctx_dynamic,
-                              he_context_t **ctx_iv_static, he_context_t **ctx_iv_dynamic) {
+                              struct speck_context_t **ctx_static,
+                              struct speck_context_t **ctx_dynamic,
+                              struct speck_context_t **ctx_iv_static,
+                              struct speck_context_t **ctx_iv_dynamic) {
 
     uint8_t key[16];
 
@@ -160,7 +164,8 @@ void packet_header_setup_key (const char *community_name,
 
 
 void packet_header_change_dynamic_key (uint8_t *key_dynamic,
-                                       he_context_t **ctx_dynamic, he_context_t **ctx_iv_dynamic) {
+                                       struct speck_context_t **ctx_dynamic,
+                                       struct speck_context_t **ctx_iv_dynamic) {
 
     uint8_t key[16];
     pearson_hash_128(key, key_dynamic, N2N_AUTH_CHALLENGE_SIZE);

diff --git a/src/sn_utils.c b/src/sn_utils.c
@@ -155,7 +155,6 @@ void calculate_shared_secrets (struct n3n_runtime_data *sss) {
             // calculate common shared secret (ECDH)
             generate_shared_secret(user->shared_secret, sss->private_key, user->public_key);
             // prepare for use as key
-            user->shared_secret_ctx = (he_context_t*)calloc(1, sizeof(speck_context_t));
             speck_init((speck_context_t**)&user->shared_secret_ctx, user->shared_secret, 128);
         }
     }
@@ -302,7 +301,7 @@ int load_allowed_sn_community (struct n3n_runtime_data *sss) {
 
         // remove allowed users from community
         HASH_ITER(hh, comm->allowed_users, user, tmp_user) {
-            free(user->shared_secret_ctx);
+            speck_deinit((speck_context_t*)user->shared_secret_ctx);
             HASH_DEL(comm->allowed_users, user);
             free(user);
         }
@@ -950,6 +949,15 @@ void sn_term (struct n3n_runtime_data *sss) {
             HASH_DEL(community->assoc, assoc);
             free(assoc);
         }
+
+        // remove allowed users from community
+        sn_user_t *user, *tmp_user;
+        HASH_ITER(hh, community->allowed_users, user, tmp_user) {
+            speck_deinit((speck_context_t*)user->shared_secret_ctx);
+            HASH_DEL(community->allowed_users, user);
+            free(user);
+        }
+
         HASH_DEL(sss->communities, community);
         free(community);
     }

diff --git a/src/wire.c b/src/wire.c
@@ -363,6 +363,7 @@ int decode_sock_payload (n2n_sock_t * sock,
     uint8_t port_low = 0;
     uint8_t port_high = 0;
 
+    memset(sock, 0, sizeof(*sock));
     retval += decode_uint8(&(sock->family), base, rem, idx);
     ++(*idx); // skip blank
     --(*rem);

diff --git a/tools/crypto_helper.c b/tools/crypto_helper.c
@@ -8,7 +8,7 @@
 
 #include <getopt.h>             // for required_argument, getopt_long, no_arg...
 #include <header_encryption.h>  // for packet_header_setup_key, packet_header...
-#include <n2n_typedefs.h>       // for he_context_t
+#include <n2n_typedefs.h>       //
 #include <n3n/conffile.h>
 #include <n3n/initfuncs.h>
 #include <pearson.h>
@@ -37,10 +37,10 @@ static void cmd_header_decrypt (int argc, char **argv, void *conf) {
     strncpy((char *)&community, argv[1], sizeof(community));
     community[sizeof(community)-1] = 0;
 
-    he_context_t *ctx_static;
-    he_context_t *ctx_dynamic;
-    he_context_t *ctx_iv_static;
-    he_context_t *ctx_iv_dynamic;
+    struct speck_context_t *ctx_static;
+    struct speck_context_t *ctx_dynamic;
+    struct speck_context_t *ctx_iv_static;
+    struct speck_context_t *ctx_iv_dynamic;
 
     packet_header_setup_key(
         community,