Browse code

crypto: always reload tls-auth/crypt key contexts

In preparation to having tls-auth/crypt keys per connection
block, it is important to ensure that such material is always
reloaded upon SIGUSR1, no matter if `persist-key` was specified
or not.

This is required because when moving from one remote to the
other the key may change and thus the key context needs to
be refreshed.

To ensure that the `persist-key` logic will still work
as expected, the tls-auth/crypt key is pre-loaded so that
the keyfile is not required at runtime.

Trac: #720
Cc: Steffan Karger <steffan@karger.me>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <20180708024517.27108-1-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17237.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

Antonio Quartulli authored on 2018/07/08 11:45:17
Showing 7 changed files
... ...
@@ -189,6 +189,34 @@ free_buf(struct buffer *buf)
189 189
     CLEAR(*buf);
190 190
 }
191 191
 
192
+static void
193
+free_buf_gc(struct buffer *buf, struct gc_arena *gc)
194
+{
195
+    if (gc)
196
+    {
197
+        struct gc_entry **e = &gc->list;
198
+
199
+        while (*e)
200
+        {
201
+            /* check if this object is the one we want to delete */
202
+            if ((uint8_t *)(*e + 1) == buf->data)
203
+            {
204
+                struct gc_entry *to_delete = *e;
205
+
206
+                /* remove element from linked list and free it */
207
+                *e = (*e)->next;
208
+                free(to_delete);
209
+
210
+                break;
211
+            }
212
+
213
+            e = &(*e)->next;
214
+        }
215
+    }
216
+
217
+    CLEAR(*buf);
218
+}
219
+
192 220
 /*
193 221
  * Return a buffer for write that is a subset of another buffer
194 222
  */
... ...
@@ -1332,3 +1360,36 @@ buffer_list_file(const char *fn, int max_line_len)
1332 1332
     }
1333 1333
     return bl;
1334 1334
 }
1335
+
1336
+struct buffer
1337
+buffer_read_from_file(const char *filename, struct gc_arena *gc)
1338
+{
1339
+    struct buffer ret = { 0 };
1340
+
1341
+    platform_stat_t file_stat = {0};
1342
+    if (platform_stat(filename, &file_stat) < 0)
1343
+    {
1344
+        return ret;
1345
+    }
1346
+
1347
+    FILE *fp = platform_fopen(filename, "r");
1348
+    if (!fp)
1349
+    {
1350
+        return ret;
1351
+    }
1352
+
1353
+    const size_t size = file_stat.st_size;
1354
+    ret = alloc_buf_gc(size + 1, gc); /* space for trailing \0 */
1355
+    ssize_t read_size = fread(BPTR(&ret), 1, size, fp);
1356
+    if (read_size < 0)
1357
+    {
1358
+        free_buf_gc(&ret, gc);
1359
+        goto cleanup;
1360
+    }
1361
+    ASSERT(buf_inc_len(&ret, read_size));
1362
+    buf_null_terminate(&ret);
1363
+
1364
+cleanup:
1365
+    fclose(fp);
1366
+    return ret;
1367
+}
... ...
@@ -1112,4 +1112,16 @@ void buffer_list_aggregate_separator(struct buffer_list *bl,
1112 1112
 
1113 1113
 struct buffer_list *buffer_list_file(const char *fn, int max_line_len);
1114 1114
 
1115
+/**
1116
+ * buffer_read_from_file - copy the content of a file into a buffer
1117
+ *
1118
+ * @param file      path to the file to read
1119
+ * @param gc        the garbage collector to use when allocating the buffer. It
1120
+ *                  is passed to alloc_buf_gc() and therefore can be NULL.
1121
+ *
1122
+ * @return the buffer storing the file content or an invalid buffer in case of
1123
+ * error
1124
+ */
1125
+struct buffer buffer_read_from_file(const char *filename, struct gc_arena *gc);
1126
+
1115 1127
 #endif /* BUFFER_H */
... ...
@@ -1224,7 +1224,7 @@ read_key_file(struct key2 *key2, const char *file, const unsigned int flags)
1224 1224
 {
1225 1225
     struct gc_arena gc = gc_new();
1226 1226
     struct buffer in;
1227
-    int fd, size;
1227
+    int size;
1228 1228
     uint8_t hex_byte[3] = {0, 0, 0};
1229 1229
     const char *error_filename = file;
1230 1230
 
... ...
@@ -1268,22 +1268,11 @@ read_key_file(struct key2 *key2, const char *file, const unsigned int flags)
1268 1268
     }
1269 1269
     else /* 'file' is a filename which refers to a file containing the ascii key */
1270 1270
     {
1271
-        in = alloc_buf_gc(2048, &gc);
1272
-        fd = platform_open(file, O_RDONLY, 0);
1273
-        if (fd == -1)
1274
-        {
1275
-            msg(M_ERR, "Cannot open key file '%s'", file);
1276
-        }
1277
-        size = read(fd, in.data, in.capacity);
1278
-        if (size < 0)
1279
-        {
1271
+        in = buffer_read_from_file(file, &gc);
1272
+        if (!buf_valid(&in))
1280 1273
             msg(M_FATAL, "Read error on key file ('%s')", file);
1281
-        }
1282
-        if (size == in.capacity)
1283
-        {
1284
-            msg(M_FATAL, "Key file ('%s') can be a maximum of %d bytes", file, (int)in.capacity);
1285
-        }
1286
-        close(fd);
1274
+
1275
+        size = in.len;
1287 1276
     }
1288 1277
 
1289 1278
     cp = (unsigned char *)in.data;
... ...
@@ -2410,7 +2410,6 @@ key_schedule_free(struct key_schedule *ks, bool free_ssl_ctx)
2410 2410
     if (tls_ctx_initialised(&ks->ssl_ctx) && free_ssl_ctx)
2411 2411
     {
2412 2412
         tls_ctx_free(&ks->ssl_ctx);
2413
-        free_key_ctx_bi(&ks->tls_wrap_key);
2414 2413
     }
2415 2414
     CLEAR(*ks);
2416 2415
 }
... ...
@@ -2501,6 +2500,48 @@ do_init_crypto_static(struct context *c, const unsigned int flags)
2501 2501
 }
2502 2502
 
2503 2503
 /*
2504
+ * Initialize the tls-auth/crypt key context
2505
+ */
2506
+static void
2507
+do_init_tls_wrap_key(struct context *c)
2508
+{
2509
+    const struct options *options = &c->options;
2510
+
2511
+    /* TLS handshake authentication (--tls-auth) */
2512
+    if (options->tls_auth_file)
2513
+    {
2514
+        /* Initialize key_type for tls-auth with auth only */
2515
+        CLEAR(c->c1.ks.tls_auth_key_type);
2516
+        if (!streq(options->authname, "none"))
2517
+        {
2518
+            c->c1.ks.tls_auth_key_type.digest = md_kt_get(options->authname);
2519
+                c->c1.ks.tls_auth_key_type.hmac_length =
2520
+                    md_kt_size(c->c1.ks.tls_auth_key_type.digest);
2521
+        }
2522
+        else
2523
+        {
2524
+            msg(M_FATAL, "ERROR: tls-auth enabled, but no valid --auth "
2525
+                "algorithm specified ('%s')", options->authname);
2526
+        }
2527
+
2528
+        crypto_read_openvpn_key(&c->c1.ks.tls_auth_key_type,
2529
+                                &c->c1.ks.tls_wrap_key,
2530
+                                options->tls_auth_file,
2531
+                                options->tls_auth_file_inline,
2532
+                                options->key_direction,
2533
+                                "Control Channel Authentication", "tls-auth");
2534
+    }
2535
+
2536
+    /* TLS handshake encryption+authentication (--tls-crypt) */
2537
+    if (options->tls_crypt_file)
2538
+    {
2539
+        tls_crypt_init_key(&c->c1.ks.tls_wrap_key,
2540
+                           options->tls_crypt_file,
2541
+                           options->tls_crypt_inline, options->tls_server);
2542
+    }
2543
+}
2544
+
2545
+/*
2504 2546
  * Initialize the persistent component of OpenVPN's TLS mode,
2505 2547
  * which is preserved across SIGUSR1 resets.
2506 2548
  */
... ...
@@ -2549,35 +2590,8 @@ do_init_crypto_tls_c1(struct context *c)
2549 2549
         /* Initialize PRNG with config-specified digest */
2550 2550
         prng_init(options->prng_hash, options->prng_nonce_secret_len);
2551 2551
 
2552
-        /* TLS handshake authentication (--tls-auth) */
2553
-        if (options->tls_auth_file)
2554
-        {
2555
-            /* Initialize key_type for tls-auth with auth only */
2556
-            CLEAR(c->c1.ks.tls_auth_key_type);
2557
-            if (!streq(options->authname, "none"))
2558
-            {
2559
-                c->c1.ks.tls_auth_key_type.digest = md_kt_get(options->authname);
2560
-                c->c1.ks.tls_auth_key_type.hmac_length =
2561
-                    md_kt_size(c->c1.ks.tls_auth_key_type.digest);
2562
-            }
2563
-            else
2564
-            {
2565
-                msg(M_FATAL, "ERROR: tls-auth enabled, but no valid --auth "
2566
-                    "algorithm specified ('%s')", options->authname);
2567
-            }
2568
-
2569
-            crypto_read_openvpn_key(&c->c1.ks.tls_auth_key_type,
2570
-                                    &c->c1.ks.tls_wrap_key, options->tls_auth_file,
2571
-                                    options->tls_auth_file_inline, options->key_direction,
2572
-                                    "Control Channel Authentication", "tls-auth");
2573
-        }
2574
-
2575
-        /* TLS handshake encryption+authentication (--tls-crypt) */
2576
-        if (options->tls_crypt_file)
2577
-        {
2578
-            tls_crypt_init_key(&c->c1.ks.tls_wrap_key, options->tls_crypt_file,
2579
-                               options->tls_crypt_inline, options->tls_server);
2580
-        }
2552
+        /* initialize tls-auth/crypt key */
2553
+        do_init_tls_wrap_key(c);
2581 2554
 
2582 2555
         c->c1.ciphername = options->ciphername;
2583 2556
         c->c1.authname = options->authname;
... ...
@@ -2599,6 +2613,12 @@ do_init_crypto_tls_c1(struct context *c)
2599 2599
         c->options.ciphername = c->c1.ciphername;
2600 2600
         c->options.authname = c->c1.authname;
2601 2601
         c->options.keysize = c->c1.keysize;
2602
+
2603
+        /*
2604
+         * tls-auth/crypt key can be configured per connection block, therefore
2605
+         * we must reload it as it may have changed
2606
+         */
2607
+        do_init_tls_wrap_key(c);
2602 2608
     }
2603 2609
 }
2604 2610
 
... ...
@@ -3402,6 +3422,13 @@ do_close_tls(struct context *c)
3402 3402
 static void
3403 3403
 do_close_free_key_schedule(struct context *c, bool free_ssl_ctx)
3404 3404
 {
3405
+    /*
3406
+     * always free the tls_auth/crypt key. If persist_key is true, the key will
3407
+     * be reloaded from memory (pre-cached)
3408
+     */
3409
+    free_key_ctx_bi(&c->c1.ks.tls_wrap_key);
3410
+    CLEAR(c->c1.ks.tls_wrap_key);
3411
+
3405 3412
     if (!(c->sig->signal_received == SIGUSR1 && c->options.persist_key))
3406 3413
     {
3407 3414
         key_schedule_free(&c->c1.ks, free_ssl_ctx);
... ...
@@ -3020,6 +3020,32 @@ options_postprocess_mutate(struct options *o)
3020 3020
         options_postprocess_mutate_ce(o, o->connection_list->array[i]);
3021 3021
     }
3022 3022
 
3023
+    /* pre-cache tls-auth/crypt key file if persist-key was specified */
3024
+    if (o->persist_key)
3025
+    {
3026
+        if (o->tls_auth_file && !o->tls_auth_file_inline)
3027
+        {
3028
+            struct buffer in = buffer_read_from_file(o->tls_auth_file, &o->gc);
3029
+            if (!buf_valid(&in))
3030
+                msg(M_FATAL, "Cannot pre-load tls-auth keyfile (%s)",
3031
+                    o->tls_auth_file);
3032
+
3033
+            o->tls_auth_file = INLINE_FILE_TAG;
3034
+            o->tls_auth_file_inline = (char *)in.data;
3035
+        }
3036
+
3037
+        if (o->tls_crypt_file && !o->tls_crypt_inline)
3038
+        {
3039
+            struct buffer in = buffer_read_from_file(o->tls_crypt_file, &o->gc);
3040
+            if (!buf_valid(&in))
3041
+                msg(M_FATAL, "Cannot pre-load tls-crypt keyfile (%s)",
3042
+                    o->tls_auth_file);
3043
+
3044
+            o->tls_crypt_file = INLINE_FILE_TAG;
3045
+            o->tls_crypt_inline = (char *)in.data;
3046
+        }
3047
+    }
3048
+
3023 3049
     if (o->tls_server)
3024 3050
     {
3025 3051
         /* Check that DH file is specified, or explicitly disabled */
... ...
@@ -28,7 +28,6 @@ buffer_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir) -I$(compat_srcdir)
28 28
 buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) -Wl,--wrap=parse_line
29 29
 buffer_testdriver_SOURCES = test_buffer.c mock_msg.c \
30 30
 	mock_get_random.c \
31
-	$(openvpn_srcdir)/buffer.c \
32 31
 	$(openvpn_srcdir)/platform.c
33 32
 
34 33
 crypto_testdriver_CFLAGS  = @TEST_CFLAGS@ \
... ...
@@ -33,6 +33,7 @@
33 33
 #include <cmocka.h>
34 34
 
35 35
 #include "buffer.h"
36
+#include "buffer.c"
36 37
 
37 38
 static void
38 39
 test_buffer_strprefix(void **state)
... ...
@@ -197,6 +198,48 @@ test_buffer_list_aggregate_separator_emptybuffers(void **state)
197 197
     assert_int_equal(BLEN(buf), 0);
198 198
 }
199 199
 
200
+static void
201
+test_buffer_free_gc_one(void **state)
202
+{
203
+    struct gc_arena gc = gc_new();
204
+    struct buffer buf = alloc_buf_gc(1024, &gc);
205
+
206
+    assert_ptr_equal(gc.list + 1, buf.data);
207
+    free_buf_gc(&buf, &gc);
208
+    assert_null(gc.list);
209
+
210
+    gc_free(&gc);
211
+}
212
+
213
+static void
214
+test_buffer_free_gc_two(void **state)
215
+{
216
+    struct gc_arena gc = gc_new();
217
+    struct buffer buf1 = alloc_buf_gc(1024, &gc);
218
+    struct buffer buf2 = alloc_buf_gc(1024, &gc);
219
+    struct buffer buf3 = alloc_buf_gc(1024, &gc);
220
+
221
+    struct gc_entry *e;
222
+
223
+    e = gc.list;
224
+
225
+    assert_ptr_equal(e + 1, buf3.data);
226
+    assert_ptr_equal(e->next + 1, buf2.data);
227
+    assert_ptr_equal(e->next->next + 1, buf1.data);
228
+
229
+    free_buf_gc(&buf2, &gc);
230
+
231
+    assert_non_null(gc.list);
232
+
233
+    while (e)
234
+    {
235
+        assert_ptr_not_equal(e + 1, buf2.data);
236
+        e = e->next;
237
+    }
238
+
239
+    gc_free(&gc);
240
+}
241
+
200 242
 int
201 243
 main(void)
202 244
 {
... ...
@@ -226,6 +269,8 @@ main(void)
226 226
         cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_emptybuffers,
227 227
                                         test_buffer_list_setup,
228 228
                                         test_buffer_list_teardown),
229
+        cmocka_unit_test(test_buffer_free_gc_one),
230
+        cmocka_unit_test(test_buffer_free_gc_two),
229 231
     };
230 232
 
231 233
     return cmocka_run_group_tests_name("buffer", tests, NULL, NULL);