Browse code

Allow trailing \r and \n in control channel message

Writing a reason from a script will easily end up adding extra \r\n characters
at the end of the reason. Our current code pushes this to the peer. So be more
liberal in accepting these message.

Github: closes OpenVPN/openvpn#568

Change-Id: I47c992b6b73b1475cbff8a28f720cf50dc1fbe3e
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Message-Id: <20240710140623.172829-1-frank@lichtenheld.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28910.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit be31325e1dfdffbb152374985c2ae7b6644e3519)

Arne Schwabe authored on 2024/07/10 23:06:23
Showing 4 changed files
... ...
@@ -290,41 +290,14 @@ check_incoming_control_channel(struct context *c)
290 290
     struct buffer buf = alloc_buf_gc(len, &gc);
291 291
     if (tls_rec_payload(c->c2.tls_multi, &buf))
292 292
     {
293
-
294 293
         while (BLEN(&buf) > 1)
295 294
         {
296
-            /* commands on the control channel are seperated by 0x00 bytes.
297
-             * cmdlen does not include the 0 byte of the string */
298
-            int cmdlen = (int)strnlen(BSTR(&buf), BLEN(&buf));
299
-
300
-            if (cmdlen < BLEN(&buf))
301
-            {
302
-                /* include the NUL byte and ensure NUL termination */
303
-                int cmdlen = (int)strlen(BSTR(&buf)) + 1;
295
+            struct buffer cmdbuf = extract_command_buffer(&buf, &gc);
304 296
 
305
-                /* Construct a buffer that only holds the current command and
306
-                 * its closing NUL byte */
307
-                struct buffer cmdbuf = alloc_buf_gc(cmdlen, &gc);
308
-                buf_write(&cmdbuf, BPTR(&buf), cmdlen);
309
-
310
-                /* check we have only printable characters or null byte in the
311
-                 * command string and no newlines */
312
-                if (!string_check_buf(&buf, CC_PRINT | CC_NULL, CC_CRLF))
313
-                {
314
-                    msg(D_PUSH_ERRORS, "WARNING: Received control with invalid characters: %s",
315
-                        format_hex(BPTR(&buf), BLEN(&buf), 256, &gc));
316
-                }
317
-                else
318
-                {
319
-                    parse_incoming_control_channel_command(c, &cmdbuf);
320
-                }
321
-            }
322
-            else
297
+            if (cmdbuf.len > 0)
323 298
             {
324
-                msg(D_PUSH_ERRORS, "WARNING: Ignoring control channel "
325
-                    "message command without NUL termination");
299
+                parse_incoming_control_channel_command(c, &cmdbuf);
326 300
             }
327
-            buf_advance(&buf, cmdlen);
328 301
         }
329 302
     }
330 303
     else
... ...
@@ -557,3 +557,43 @@ check_session_id_hmac(struct tls_pre_decrypt_state *state,
557 557
     }
558 558
     return false;
559 559
 }
560
+
561
+struct buffer
562
+extract_command_buffer(struct buffer *buf, struct gc_arena *gc)
563
+{
564
+    /* commands on the control channel are seperated by 0x00 bytes.
565
+     * cmdlen does not include the 0 byte of the string */
566
+    int cmdlen = (int)strnlen(BSTR(buf), BLEN(buf));
567
+
568
+    if (cmdlen >= BLEN(buf))
569
+    {
570
+        buf_advance(buf, cmdlen);
571
+        /* Return empty buffer */
572
+        struct buffer empty = { 0 };
573
+        return empty;
574
+    }
575
+
576
+    /* include the NUL byte and ensure NUL termination */
577
+    cmdlen +=  1;
578
+
579
+    /* Construct a buffer that only holds the current command and
580
+     * its closing NUL byte */
581
+    struct buffer cmdbuf = alloc_buf_gc(cmdlen, gc);
582
+    buf_write(&cmdbuf, BPTR(buf), cmdlen);
583
+
584
+    /* Remove \r and \n at the end of the buffer to avoid
585
+     * problems with scripts and other that add extra \r and \n */
586
+    buf_chomp(&cmdbuf);
587
+
588
+    /* check we have only printable characters or null byte in the
589
+     * command string and no newlines */
590
+    if (!string_check_buf(&cmdbuf, CC_PRINT | CC_NULL, CC_CRLF))
591
+    {
592
+        msg(D_PUSH_ERRORS, "WARNING: Received control with invalid characters: %s",
593
+            format_hex(BPTR(&cmdbuf), BLEN(&cmdbuf), 256, gc));
594
+        cmdbuf.len = 0;
595
+    }
596
+
597
+    buf_advance(buf, cmdlen);
598
+    return cmdbuf;
599
+}
... ...
@@ -230,6 +230,20 @@ tls_reset_standalone(struct tls_wrap_ctx *ctx,
230 230
                      uint8_t header,
231 231
                      bool request_resend_wkc);
232 232
 
233
+
234
+/**
235
+ * Extracts a control channel message from buf and adjusts the size of
236
+ * buf after the message has been extracted
237
+ * @param buf   The buffer the message should be extracted from
238
+ * @param gc    gc_arena to be used for the returned buffer and displaying
239
+ *              diagnostic messages
240
+ * @return      A buffer with a control channel message or a buffer with
241
+ *              with length 0 if there is no message or the message has
242
+ *              invalid characters.
243
+ */
244
+struct buffer
245
+extract_command_buffer(struct buffer *buf, struct gc_arena *gc);
246
+
233 247
 static inline const char *
234 248
 packet_opcode_name(int op)
235 249
 {
... ...
@@ -625,6 +625,40 @@ test_generate_reset_packet_tls_auth(void **ut_state)
625 625
     free_tas(&tas_server);
626 626
 }
627 627
 
628
+static void
629
+test_extract_control_message(void **ut_state)
630
+{
631
+    struct gc_arena gc = gc_new();
632
+    struct buffer input_buf = alloc_buf_gc(1024, &gc);
633
+
634
+    /* This message will have a \0x00 at the end since it is a C string */
635
+    const char input[] = "valid control message\r\n\0\0Invalid\r\none\0valid one again";
636
+
637
+    buf_write(&input_buf, input, sizeof(input));
638
+    struct buffer cmd1 = extract_command_buffer(&input_buf, &gc);
639
+    struct buffer cmd2 = extract_command_buffer(&input_buf, &gc);
640
+    struct buffer cmd3 = extract_command_buffer(&input_buf, &gc);
641
+    struct buffer cmd4 = extract_command_buffer(&input_buf, &gc);
642
+    struct buffer cmd5 = extract_command_buffer(&input_buf, &gc);
643
+
644
+    assert_string_equal(BSTR(&cmd1), "valid control message");
645
+    /* empty message with just a \0x00 */
646
+    assert_int_equal(cmd2.len, 1);
647
+    assert_string_equal(BSTR(&cmd2), "");
648
+    assert_int_equal(cmd3.len, 0);
649
+    assert_string_equal(BSTR(&cmd4), "valid one again");
650
+    assert_int_equal(cmd5.len, 0);
651
+
652
+    const uint8_t nonull[6] = { 'n', 'o', ' ', 'N', 'U', 'L'};
653
+    struct buffer nonull_buf = alloc_buf_gc(1024, &gc);
654
+
655
+    buf_write(&nonull_buf, nonull, sizeof(nonull));
656
+    struct buffer nonullcmd = extract_command_buffer(&nonull_buf, &gc);
657
+    assert_int_equal(nonullcmd.len, 0);
658
+
659
+    gc_free(&gc);
660
+}
661
+
628 662
 int
629 663
 main(void)
630 664
 {
... ...
@@ -638,6 +672,7 @@ main(void)
638 638
         cmocka_unit_test(test_verify_hmac_tls_auth),
639 639
         cmocka_unit_test(test_generate_reset_packet_plain),
640 640
         cmocka_unit_test(test_generate_reset_packet_tls_auth),
641
+        cmocka_unit_test(test_extract_control_message)
641 642
     };
642 643
 
643 644
 #if defined(ENABLE_CRYPTO_OPENSSL)