Menu
▾
▴
[Openvpn-devel] [M] Change in openvpn[master]: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-28 19:28:37
|
Attention is currently required from: mrbff, plaisthos.
Hello flichtenheld, plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
to look at the new patch set (#2).
Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
......................................................................
PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
The number of messages calculated before the call to message_splitter(), used
in the memory allocation in the buffer array, could in certain cases be less
than one than the actual number of messages, thus causing an override of the
sentinel buffer in message_splitter and therefore an invalid read in
send_single_push_update(). The case in question would be, for example, a
sequence of three options "A, B, C" with the size of B equal to safe_cap - 1
and the sum of the sizes of A and C less than safe_cap - 2.
The buffer array was therefore replaced with a list of buffers to completely
avoid calculating the number of messages before it was actually computed.
The test case in question has been added to the unit tests.
The unit tests have been improved using cmocka macros.
Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Signed-off-by: Marco Baffo <ma...@ma...>
---
M src/openvpn/push_util.c
M tests/unit_tests/openvpn/test_push_update_msg.c
2 files changed, 145 insertions(+), 32 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1316/2
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 552679a..51c7b5f 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -91,7 +91,7 @@
* Return `false` on failure an `true` on success.
*/
static bool
-message_splitter(const char *s, struct buffer *msgs, struct gc_arena *gc, const size_t safe_cap)
+message_splitter(const char *s, struct buffer_list *msgs, struct gc_arena *gc, const size_t safe_cap)
{
if (!s || !*s)
{
@@ -100,7 +100,6 @@
char *str = gc_strdup(s, gc);
size_t i = 0;
- int im = 0;
while (*str)
{
@@ -115,37 +114,38 @@
}
str[ci] = '\0';
/* copy from i to (ci -1) */
- msgs[im] = forge_msg(str, ",push-continuation 2", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 2", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
i = ci + 1;
}
else
{
- if (im)
+ if (msgs->head)
{
- msgs[im] = forge_msg(str, ",push-continuation 1", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 1", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
else
{
- msgs[im] = forge_msg(str, NULL, gc);
+ struct buffer tmp = forge_msg(str, NULL, gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
i = strlen(str);
}
str = &str[i];
- im++;
}
return true;
}
/* send the message(s) prepared to one single client */
static bool
-send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer *msgs)
+send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer_list *msgs)
{
- if (!msgs[0].data || !*(msgs[0].data))
+ if (!msgs->head)
{
return false;
}
- int i = -1;
unsigned int option_types_found = 0;
struct context *c = &mi->context;
struct options o;
@@ -160,9 +160,10 @@
o.ifconfig_local = canary;
o.ifconfig_ipv6_local = canary;
- while (msgs[++i].data && *(msgs[i].data))
+ struct buffer_entry *e = msgs->head;
+ while (e)
{
- if (!send_control_channel_string(c, BSTR(&msgs[i]), D_PUSH))
+ if (!send_control_channel_string(c, BSTR(&e->buf), D_PUSH))
{
return false;
}
@@ -182,13 +183,14 @@
* Also we need to make a temporary copy so we can buf_advance()
* without modifying original buffer.
*/
- struct buffer tmp_msg = msgs[i];
+ struct buffer tmp_msg = e->buf;
buf_string_compare_advance(&tmp_msg, push_update_cmd);
unsigned int permission_mask = pull_permission_mask(c);
if (process_push_update(c, &o, permission_mask, &option_types_found, &tmp_msg, true) == PUSH_MSG_ERROR)
{
msg(M_WARN, "Failed to process push update message sent to client ID: %u", c->c2.tls_multi->peer_id);
}
+ e = e->next;
}
if (option_types_found & OPT_P_UP)
@@ -270,12 +272,11 @@
* we want to send exceeds that size we have to split it into smaller messages */
ASSERT(push_bundle_size > extra);
const size_t safe_cap = push_bundle_size - extra;
- size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0);
- struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc);
+ struct buffer_list *msgs = buffer_list_new();
- msgs[msgs_num].data = NULL;
if (!message_splitter(msg, msgs, &gc, safe_cap))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -EINVAL;
}
@@ -286,6 +287,7 @@
if (!mi)
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -ENOENT;
}
@@ -293,6 +295,7 @@
if (!support_push_update(mi))
{
msg(M_CLIENT, "PUSH_UPDATE: not sending message to unsupported peer with ID: %u", mi->context.c2.tls_multi->peer_id);
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -300,11 +303,13 @@
if (!mi->halt
&& send_single_push_update(m, mi, msgs))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 1;
}
else
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -334,6 +339,7 @@
}
hash_iterator_free(&hi);
+ buffer_list_free(msgs);
gc_free(&gc);
return count;
}
diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c
index 516e94c..e450a31 100644
--- a/tests/unit_tests/openvpn/test_push_update_msg.c
+++ b/tests/unit_tests/openvpn/test_push_update_msg.c
@@ -136,11 +136,7 @@
bool
send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel)
{
- if (res && res[i] && strcmp(res[i], str))
- {
- printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str);
- return false;
- }
+ check_expected(str);
i++;
return true;
}
@@ -301,44 +297,75 @@
#ifdef ENABLE_MANAGEMENT
char *r0[] = {
- "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0"
+ "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0",
+ NULL
};
char *r1[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r3[] = {
- "PUSH_UPDATE,,,"
+ "PUSH_UPDATE,,,",
+ NULL
};
char *r4[] = {
"PUSH_UPDATE,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r5[] = {
"PUSH_UPDATE,,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r6[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8, redirect-gateway 10.10.10.10,,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r7[] = {
"PUSH_UPDATE,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,push-continuation 2",
- "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1"
+ "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1",
+ NULL
};
char *r8[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway\n local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1",
+ NULL
};
char *r9[] = {
- "PUSH_UPDATE,,"
+ "PUSH_UPDATE,,",
+ NULL
};
-
+char *r11[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1",
+ NULL
+};
+char *r12[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2",
+ "PUSH_UPDATE,abc,push-continuation 1",
+ NULL
+};
+char *r13[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,",
+ NULL
+};
+char *r14[] = {
+ "PUSH_UPDATE,a,push-continuation 2",
+ "PUSH_UPDATE,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,push-continuation 2",
+ "PUSH_UPDATE,a,push-continuation 1",
+ NULL
+};
const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0";
const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,"
@@ -365,8 +392,29 @@
"daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline"
"decorate decrease deer defense define defy degree delay deliver demand demise denial";
+const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a";
+
+const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc";
+
+const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,";
+
+const char *msg14 = "a,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,a";
+
#define PUSH_BUNDLE_SIZE_TEST 184
+#define expect_control_channel_strings() \
+ do \
+ { \
+ for (int j = 0; res[j] != NULL; j++) \
+ { \
+ expect_string(send_control_channel_string, str, res[j]); \
+ } \
+ } while (0)
+
static void
test_send_push_msg0(void **state)
{
@@ -374,8 +422,10 @@
res = r0;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg0, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
+
static void
test_send_push_msg1(void **state)
{
@@ -383,6 +433,7 @@
res = r1;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg1, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -403,6 +454,7 @@
res = r3;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg3, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -413,6 +465,7 @@
res = r4;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg4, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -423,6 +476,7 @@
res = r5;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg5, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -433,6 +487,7 @@
res = r6;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg6, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -443,6 +498,7 @@
res = r7;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg7, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -453,6 +509,7 @@
res = r8;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg8, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -463,6 +520,7 @@
res = r9;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg9, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -476,6 +534,50 @@
assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
}
+static void
+test_send_push_msg11(void **state)
+{
+ i = 0;
+ res = r11;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg12(void **state)
+{
+ i = 0;
+ res = r12;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg13(void **state)
+{
+ i = 0;
+ res = r13;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg14(void **state)
+{
+ i = 0;
+ res = r14;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg14, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
#undef PUSH_BUNDLE_SIZE_TEST
static int
@@ -535,6 +637,7 @@
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown),
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown),
#ifdef ENABLE_MANAGEMENT
+
cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg1, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg2, setup2, teardown2),
@@ -545,7 +648,11 @@
cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2),
- cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2)
+ cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg14, setup2, teardown2)
#endif
};
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Gerrit-Change-Number: 1316
Gerrit-PatchSet: 2
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: mrbff <ma...@ma...>
|
