Browse code

Make sending plain text control message session aware

The control messages coming from auth pending should always be on the
session that triggered them (i.e. INITIAL or ACTIVE) and not always on the
active session. Rework the code path that trigger those messsages from
management and plugin/script to specify the TLS session.

We only support the two TLS sessions that are supposed to be active. TLS
sessions in any lame slot (TM_LAME or KS_LAME) are not considered to be
candidates for sending messages as these slots only serve to keep key
material around.

Unfortunately, this fix requires the management interface to be changed
to allow including the specific session the messages should to go to. As
there are very few users of this interface with auth-pending, I made this
a hard change instead of adding hacky workaround code that is not always
working correctly anyway.

send_control_channel_string() will continue to only use the primary session
and key but the current users of that (push replys and exit notification)
already require the established session to be the active one, so there
no changes needed at the moment.

Github: fixes OpenVPN/openvpn#256

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20230301135353.2811069-2-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26320.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit a261e173341f8e68505a6ab5a413d09b0797a459)

Arne Schwabe authored on 2023/03/01 22:53:53
Showing 10 changed files
... ...
@@ -1,3 +1,16 @@
1
+Overview of changes in 2.6.2
2
+============================
3
+
4
+Bug fixes
5
+---------
6
+- sending of AUTH_PENDING and INFO_PRE messages fixed (OpenVPN/openvpn#256)
7
+
8
+User visible changes
9
+--------------------
10
+- The ``client-pending-auth`` management command now requires also the
11
+  key id. The management version has been changed to 5 to indicate this change.
12
+
13
+
1 14
 Overview of changes in 2.6.1
2 15
 ============================
3 16
 
... ...
@@ -613,10 +613,10 @@ COMMAND -- client-pending-auth  (OpenVPN 2.5 or higher)
613 613
 
614 614
 Instruct OpenVPN server to send AUTH_PENDING and INFO_PRE message
615 615
 to signal a pending authenticating to the client. A pending auth means
616
-that the connecting requires extra authentication like a one time
616
+that connecting requires extra authentication like a one time
617 617
 password or doing a single sign on via web.
618 618
 
619
-    client-pending-auth {CID} {EXTRA} {TIMEOUT}
619
+    client-pending-auth {CID} {KID} {EXTRA} {TIMEOUT}
620 620
 
621 621
 The server will send AUTH_PENDING and INFO_PRE,{EXTRA} to the client. If the
622 622
 client supports accepting keywords to AUTH_PENDING (announced via IV_PROTO),
... ...
@@ -639,11 +639,16 @@ Both client and server limit the maximum timeout to the smaller value of half th
639 639
 
640 640
 For the format of {EXTRA} see below. For OpenVPN server this is a stateless
641 641
 operation and needs to be followed by a client-deny/client-auth[-nt] command
642
-(that is the result of the out of band authentication).
642
+(that is the result of the out-of-band authentication).
643
+
644
+Note that the {KID} argument has been added in management version 5
645
+to specify the pending client key the authentication belongs to.
646
+This ensures that the pending auth message is tied strictly to the
647
+authentication session.
643 648
 
644 649
 Before issuing a client-pending-auth to a client instead of a
645 650
 client-auth/client-deny, the server should check the IV_SSO
646
-environment variable for whether the method is supported. Currently
651
+environment variable for whether the method is supported. Currently,
647 652
 defined methods are crtext for challenge/response using text
648 653
 (e.g., TOTP), openurl (deprecated) and webauth for opening a URL in
649 654
 the client to continue authentication. A client supporting webauth and
... ...
@@ -366,20 +366,20 @@ check_connection_established(struct context *c)
366 366
 }
367 367
 
368 368
 bool
369
-send_control_channel_string_dowork(struct tls_multi *multi,
369
+send_control_channel_string_dowork(struct tls_session *session,
370 370
                                    const char *str, int msglevel)
371 371
 {
372 372
     struct gc_arena gc = gc_new();
373 373
     bool stat;
374 374
 
375
-    ASSERT(multi);
376
-    struct key_state *ks = get_key_scan(multi, 0);
375
+    ASSERT(session);
376
+    struct key_state *ks = &session->key[KS_PRIMARY];
377 377
 
378 378
     /* buffered cleartext write onto TLS control channel */
379 379
     stat = tls_send_payload(ks, (uint8_t *) str, strlen(str) + 1);
380 380
 
381 381
     msg(msglevel, "SENT CONTROL [%s]: '%s' (status=%d)",
382
-        tls_common_name(multi, false),
382
+        session->common_name ? session->common_name : "UNDEF",
383 383
         sanitize_control_message(str, &gc),
384 384
         (int) stat);
385 385
 
... ...
@@ -399,8 +399,8 @@ send_control_channel_string(struct context *c, const char *str, int msglevel)
399 399
 {
400 400
     if (c->c2.tls_multi)
401 401
     {
402
-        bool ret = send_control_channel_string_dowork(c->c2.tls_multi,
403
-                                                      str, msglevel);
402
+        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
403
+        bool ret = send_control_channel_string_dowork(session, str, msglevel);
404 404
         reschedule_multi_process(c);
405 405
 
406 406
         return ret;
... ...
@@ -265,21 +265,22 @@ send_control_channel_string(struct context *c, const char *str, int msglevel);
265 265
 
266 266
 /*
267 267
  * Send a string to remote over the TLS control channel.
268
- * Used for push/pull messages, passing username/password,
269
- * etc.
268
+ * Used for push/pull messages, auth pending and other clear text
269
+ * control messages.
270 270
  *
271 271
  * This variant does not schedule the actual sending of the message
272 272
  * The caller needs to ensure that it is scheduled or call
273 273
  * send_control_channel_string
274 274
  *
275
- * @param multi      - The tls_multi structure of the VPN tunnel associated
276
- *                     with the packet.
275
+ * @param session    - The session structure of the VPN tunnel associated
276
+ *                     with the packet. The method will always use the
277
+ *                     primary key (KS_PRIMARY) for sending the message
277 278
  * @param str        - The message to be sent
278 279
  * @param msglevel   - Message level to use for logging
279 280
  */
280 281
 
281 282
 bool
282
-send_control_channel_string_dowork(struct tls_multi *multi,
283
+send_control_channel_string_dowork(struct tls_session *session,
283 284
                                    const char *str, int msglevel);
284 285
 
285 286
 
... ...
@@ -1042,22 +1042,25 @@ parse_uint(const char *str, const char *what, unsigned int *uint)
1042 1042
  *
1043 1043
  * @param man           The management interface struct
1044 1044
  * @param cid_str       The CID in string form
1045
+ * @param kid_str       The key ID in string form
1045 1046
  * @param extra         The string to be send to the client containing
1046 1047
  *                      the information of the additional steps
1047 1048
  */
1048 1049
 static void
1049 1050
 man_client_pending_auth(struct management *man, const char *cid_str,
1050
-                        const char *extra, const char *timeout_str)
1051
+                        const char *kid_str, const char *extra,
1052
+                        const char *timeout_str)
1051 1053
 {
1052 1054
     unsigned long cid = 0;
1055
+    unsigned int kid = 0;
1053 1056
     unsigned int timeout = 0;
1054
-    if (parse_cid(cid_str, &cid)
1057
+    if (parse_cid(cid_str, &cid) && parse_uint(kid_str, "KID", &kid)
1055 1058
         && parse_uint(timeout_str, "TIMEOUT", &timeout))
1056 1059
     {
1057 1060
         if (man->persist.callback.client_pending_auth)
1058 1061
         {
1059 1062
             bool ret = (*man->persist.callback.client_pending_auth)
1060
-                           (man->persist.callback.arg, cid, extra, timeout);
1063
+                           (man->persist.callback.arg, cid, kid, extra, timeout);
1061 1064
 
1062 1065
             if (ret)
1063 1066
             {
... ...
@@ -1594,9 +1597,9 @@ man_dispatch_command(struct management *man, struct status_output *so, const cha
1594 1594
     }
1595 1595
     else if (streq(p[0], "client-pending-auth"))
1596 1596
     {
1597
-        if (man_need(man, p, 3, 0))
1597
+        if (man_need(man, p, 4, 0))
1598 1598
         {
1599
-            man_client_pending_auth(man, p[1], p[2], p[3]);
1599
+            man_client_pending_auth(man, p[1], p[2], p[3], p[4]);
1600 1600
         }
1601 1601
     }
1602 1602
     else if (streq(p[0], "rsa-sig"))
... ...
@@ -52,7 +52,7 @@
52 52
 #include "socket.h"
53 53
 #include "mroute.h"
54 54
 
55
-#define MANAGEMENT_VERSION                      4
55
+#define MANAGEMENT_VERSION                      5
56 56
 #define MANAGEMENT_N_PASSWORD_RETRIES           3
57 57
 #define MANAGEMENT_LOG_HISTORY_INITIAL_SIZE   100
58 58
 #define MANAGEMENT_ECHO_BUFFER_SIZE           100
... ...
@@ -194,6 +194,7 @@ struct management_callback
194 194
                          struct buffer_list *cc_config); /* ownership transferred */
195 195
     bool (*client_pending_auth) (void *arg,
196 196
                                  const unsigned long cid,
197
+                                 const unsigned int kid,
197 198
                                  const char *extra,
198 199
                                  unsigned int timeout);
199 200
     char *(*get_peer_info) (void *arg, const unsigned long cid);
... ...
@@ -3989,15 +3989,33 @@ management_kill_by_cid(void *arg, const unsigned long cid, const char *kill_msg)
3989 3989
 static bool
3990 3990
 management_client_pending_auth(void *arg,
3991 3991
                                const unsigned long cid,
3992
+                               const unsigned int mda_key_id,
3992 3993
                                const char *extra,
3993 3994
                                unsigned int timeout)
3994 3995
 {
3995 3996
     struct multi_context *m = (struct multi_context *) arg;
3996 3997
     struct multi_instance *mi = lookup_by_cid(m, cid);
3998
+
3997 3999
     if (mi)
3998 4000
     {
4001
+        struct tls_multi *multi = mi->context.c2.tls_multi;
4002
+        struct tls_session *session;
4003
+
4004
+        if (multi->session[TM_INITIAL].key[KS_PRIMARY].mda_key_id == mda_key_id)
4005
+        {
4006
+            session = &multi->session[TM_INITIAL];
4007
+        }
4008
+        else if (multi->session[TM_ACTIVE].key[KS_PRIMARY].mda_key_id == mda_key_id)
4009
+        {
4010
+            session = &multi->session[TM_ACTIVE];
4011
+        }
4012
+        else
4013
+        {
4014
+            return false;
4015
+        }
4016
+
3999 4017
         /* sends INFO_PRE and AUTH_PENDING messages to client */
4000
-        bool ret = send_auth_pending_messages(mi->context.c2.tls_multi, extra,
4018
+        bool ret = send_auth_pending_messages(multi, session, extra,
4001 4019
                                               timeout);
4002 4020
         reschedule_multi_process(&mi->context);
4003 4021
         multi_schedule_context_wakeup(m, mi);
... ...
@@ -412,7 +412,16 @@ send_auth_failed(struct context *c, const char *client_reason)
412 412
         {
413 413
             buf_printf(&buf, ",%s", client_reason);
414 414
         }
415
-        send_control_channel_string(c, BSTR(&buf), D_PUSH);
415
+
416
+        /* We kill the whole session, send the AUTH_FAILED to any TLS session
417
+         * that might be active */
418
+        send_control_channel_string_dowork(&c->c2.tls_multi->session[TM_INITIAL],
419
+                                           BSTR(&buf), D_PUSH);
420
+        send_control_channel_string_dowork(&c->c2.tls_multi->session[TM_ACTIVE],
421
+                                           BSTR(&buf), D_PUSH);
422
+
423
+        reschedule_multi_process(c);
424
+
416 425
     }
417 426
 
418 427
     gc_free(&gc);
... ...
@@ -420,10 +429,11 @@ send_auth_failed(struct context *c, const char *client_reason)
420 420
 
421 421
 
422 422
 bool
423
-send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra,
424
-                           unsigned int timeout)
423
+send_auth_pending_messages(struct tls_multi *tls_multi,
424
+                           struct tls_session *session,
425
+                           const char *extra, unsigned int timeout)
425 426
 {
426
-    struct key_state *ks = get_key_scan(tls_multi, 0);
427
+    struct key_state *ks = &session->key[KS_PRIMARY];
427 428
 
428 429
     static const char info_pre[] = "INFO_PRE,";
429 430
 
... ...
@@ -440,7 +450,7 @@ send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra,
440 440
     struct gc_arena gc = gc_new();
441 441
     if ((proto & IV_PROTO_AUTH_PENDING_KW) == 0)
442 442
     {
443
-        send_control_channel_string_dowork(tls_multi, "AUTH_PENDING", D_PUSH);
443
+        send_control_channel_string_dowork(session, "AUTH_PENDING", D_PUSH);
444 444
     }
445 445
     else
446 446
     {
... ...
@@ -451,7 +461,7 @@ send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra,
451 451
         struct buffer buf = alloc_buf_gc(len, &gc);
452 452
         buf_printf(&buf, auth_pre);
453 453
         buf_printf(&buf, "%u", timeout);
454
-        send_control_channel_string_dowork(tls_multi, BSTR(&buf), D_PUSH);
454
+        send_control_channel_string_dowork(session, BSTR(&buf), D_PUSH);
455 455
     }
456 456
 
457 457
     size_t len = strlen(extra) + 1 + sizeof(info_pre);
... ...
@@ -464,7 +474,7 @@ send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra,
464 464
     struct buffer buf = alloc_buf_gc(len, &gc);
465 465
     buf_printf(&buf, info_pre);
466 466
     buf_printf(&buf, "%s", extra);
467
-    send_control_channel_string_dowork(tls_multi, BSTR(&buf), D_PUSH);
467
+    send_control_channel_string_dowork(session, BSTR(&buf), D_PUSH);
468 468
 
469 469
     ks->auth_deferred_expire = now + timeout;
470 470
 
... ...
@@ -741,6 +751,7 @@ send_push_reply_auth_token(struct tls_multi *multi)
741 741
 {
742 742
     struct gc_arena gc = gc_new();
743 743
     struct push_list push_list = { 0 };
744
+    struct tls_session *session = &multi->session[TM_ACTIVE];
744 745
 
745 746
     prepare_auth_token_push_reply(multi, &gc, &push_list);
746 747
 
... ...
@@ -751,7 +762,7 @@ send_push_reply_auth_token(struct tls_multi *multi)
751 751
     /* Construct a mimimal control channel push reply message */
752 752
     struct buffer buf = alloc_buf_gc(PUSH_BUNDLE_SIZE, &gc);
753 753
     buf_printf(&buf, "%s,%s", push_reply_cmd, e->option);
754
-    send_control_channel_string_dowork(multi, BSTR(&buf), D_PUSH);
754
+    send_control_channel_string_dowork(session, BSTR(&buf), D_PUSH);
755 755
     gc_free(&gc);
756 756
 }
757 757
 
... ...
@@ -78,16 +78,18 @@ void send_auth_failed(struct context *c, const char *client_reason);
78 78
  * more details on message format
79 79
  */
80 80
 bool
81
-send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra,
81
+send_auth_pending_messages(struct tls_multi *tls_multi,
82
+                           struct tls_session *session, const char *extra,
82 83
                            unsigned int timeout);
83 84
 
84 85
 void send_restart(struct context *c, const char *kill_msg);
85 86
 
86 87
 /**
87 88
  * Sends a push reply message only containin the auth-token to update
88
- * the auth-token on the client
89
+ * the auth-token on the client. Always pushes to the active session
89 90
  *
90
- * @param multi  - The tls_multi structure belonging to the instance to push to
91
+ * @param multi    - The \c tls_multi structure belonging to the instance
92
+ *                   to push to
91 93
  */
92 94
 void send_push_reply_auth_token(struct tls_multi *multi);
93 95
 
... ...
@@ -916,7 +916,8 @@ check_auth_pending_method(const char *peer_info, const char *method)
916 916
  */
917 917
 static bool
918 918
 key_state_check_auth_pending_file(struct auth_deferred_status *ads,
919
-                                  struct tls_multi *multi)
919
+                                  struct tls_multi *multi,
920
+                                  struct tls_session *session)
920 921
 {
921 922
     bool ret = true;
922 923
     if (ads->auth_pending_file)
... ...
@@ -965,7 +966,7 @@ key_state_check_auth_pending_file(struct auth_deferred_status *ads,
965 965
             }
966 966
             else
967 967
             {
968
-                send_auth_pending_messages(multi, BSTR(extra_buf), timeout);
968
+                send_auth_pending_messages(multi, session, BSTR(extra_buf), timeout);
969 969
             }
970 970
         }
971 971
 
... ...
@@ -1390,7 +1391,7 @@ verify_user_pass_script(struct tls_session *session, struct tls_multi *multi,
1390 1390
         /* Check if we the plugin has written the pending auth control
1391 1391
          * file and send the pending auth to the client */
1392 1392
         if (!key_state_check_auth_pending_file(&ks->script_auth,
1393
-                                               multi))
1393
+                                               multi, session))
1394 1394
         {
1395 1395
             retval = OPENVPN_PLUGIN_FUNC_ERROR;
1396 1396
             key_state_rm_auth_control_files(&ks->script_auth);
... ...
@@ -1514,7 +1515,7 @@ verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi,
1514 1514
     {
1515 1515
         /* Check if the plugin has written the pending auth control
1516 1516
          * file and send the pending auth to the client */
1517
-        if (!key_state_check_auth_pending_file(&ks->plugin_auth, multi))
1517
+        if (!key_state_check_auth_pending_file(&ks->plugin_auth, multi, session))
1518 1518
         {
1519 1519
             retval = OPENVPN_PLUGIN_FUNC_ERROR;
1520 1520
         }