This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Harald Welte gerrit-no-reply at lists.osmocom.orgHarald Welte has submitted this change and it was merged. ( https://gerrit.osmocom.org/9474 )
Change subject: protocol: reject illegal lco options
......................................................................
protocol: reject illegal lco options
At the moment osmo-mgw will accept multiple lco options. (e.g.
p:10, a:PCMU, p:10) If LCO appear multiple times, than the first
appearance of will be parsed and used, all following appearances
will be ignored. However, having multiple appearances of LCO is
illegal and affected requests should be rejected. Also osmo-mgw
should reject illegal formatted LCO strings
- make sure that multiple appearances of LCOs will be rejected
- make sure that illegal formated LCOs are rejected
- add testcases with garbeled LCO and valid LCO examples
Change-Id: Iae2fddfa5f2bcfc952f8ab217b3056694e5f7812
Closes: OS#3119
---
M include/osmocom/mgcp/mgcp_internal.h
M src/libosmo-mgcp/mgcp_protocol.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
4 files changed, 244 insertions(+), 2 deletions(-)
Approvals:
Jenkins Builder: Verified
Harald Welte: Looks good to me, approved
diff --git a/include/osmocom/mgcp/mgcp_internal.h b/include/osmocom/mgcp/mgcp_internal.h
index 7a00c98..bff7da0 100644
--- a/include/osmocom/mgcp/mgcp_internal.h
+++ b/include/osmocom/mgcp/mgcp_internal.h
@@ -282,6 +282,8 @@
struct mgcp_trunk_config *mgcp_trunk_alloc(struct mgcp_config *cfg, int index);
struct mgcp_trunk_config *mgcp_trunk_num(struct mgcp_config *cfg, int index);
+char *get_lco_identifier(const char *options);
+int check_local_cx_options(void *ctx, const char *options);
void mgcp_rtp_end_config(struct mgcp_endpoint *endp, int expect_ssrc_change,
struct mgcp_rtp_end *rtp);
uint32_t mgcp_rtp_packet_duration(struct mgcp_endpoint *endp,
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index ded1552..21f6cf3 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -395,6 +395,121 @@
return -1;
}
+/*! Helper function for check_local_cx_options() to get a pointer of the next
+ * lco option identifier
+ * \param[in] lco string
+ * \returns pointer to the beginning of the LCO identifier, NULL on failure */
+char *get_lco_identifier(const char *options)
+{
+ char *ptr;
+ unsigned int count = 0;
+
+ /* Jump to the end of the lco identifier */
+ ptr = strstr(options, ":");
+ if (!ptr)
+ return NULL;
+
+ /* Walk backwards until the pointer points to the beginning of the
+ * lco identifier. We know that we stand at the beginning when we
+ * are either at the beginning of the memory or see a space or
+ * comma. (this is tolerant, it will accept a:10, b:11 as well as
+ * a:10,b:11) */
+ while (1) {
+ /* Endless loop protection */
+ if (count > 10000)
+ return NULL;
+ else if (ptr < options || *ptr == ' ' || *ptr == ',') {
+ ptr++;
+ break;
+ }
+ ptr--;
+ count++;
+ }
+
+ /* Check if we got any result */
+ if (*ptr == ':')
+ return NULL;
+
+ return ptr;
+}
+
+/*! Check the LCO option. This function checks for multiple appearence of LCO
+ * options, which is illegal
+ * \param[in] ctx talloc context
+ * \param[in] lco string
+ * \returns 0 on success, -1 on failure */
+int check_local_cx_options(void *ctx, const char *options)
+{
+ int i;
+ char *options_copy;
+ char *lco_identifier;
+ char *lco_identifier_end;
+ char *next_lco_identifier;
+
+ char **lco_seen;
+ unsigned int lco_seen_n = 0;
+
+ if (!options)
+ return -1;
+
+ lco_seen =
+ (char **)talloc_zero_size(ctx, strlen(options) * sizeof(char *));
+ options_copy = talloc_strdup(ctx, options);
+ lco_identifier = options_copy;
+
+ do {
+ /* Move the lco_identifier pointer to the beginning of the
+ * current lco option identifier */
+ lco_identifier = get_lco_identifier(lco_identifier);
+ if (!lco_identifier)
+ goto error;
+
+ /* Look ahead to the next LCO option early, since we
+ * will parse destructively */
+ next_lco_identifier = strstr(lco_identifier + 1, ",");
+
+ /* Pinch off the end of the lco field identifier name
+ * and see if we still got something, also check if
+ * there is some value after the colon. */
+ lco_identifier_end = strstr(lco_identifier, ":");
+ if (!lco_identifier_end)
+ goto error;
+ if (*(lco_identifier_end + 1) == ' '
+ || *(lco_identifier_end + 1) == ','
+ || *(lco_identifier_end + 1) == '\0')
+ goto error;
+ *lco_identifier_end = '\0';
+ if (strlen(lco_identifier) == 0)
+ goto error;
+
+ /* Check if we have already seen the current field identifier
+ * before. If yes, we must bail, an LCO must only appear once
+ * in the LCO string */
+ for (i = 0; i < lco_seen_n; i++) {
+ if (strcmp(lco_seen[i], lco_identifier) == 0)
+ goto error;
+ }
+ lco_seen[lco_seen_n] = lco_identifier;
+ lco_seen_n++;
+
+ /* The first identifier must always be found at the beginnning
+ * of the LCO string */
+ if (lco_seen[0] != options_copy)
+ goto error;
+
+ /* Go to the next lco option */
+ lco_identifier = next_lco_identifier;
+ } while (lco_identifier);
+
+ talloc_free(lco_seen);
+ talloc_free(options_copy);
+ return 0;
+error:
+ talloc_free(lco_seen);
+ talloc_free(options_copy);
+ return -1;
+}
+
/* Set the LCO from a string (see RFC 3435).
* The string is stored in the 'string' field. A NULL string is handled exactly
* like an empty string, the 'string' field is never NULL after this function
@@ -410,9 +525,16 @@
if (strlen(options) == 0)
return 0;
- talloc_free(lco->string);
- lco->string = talloc_strdup(ctx, options ? options : "");
+ /* Make sure the encoding of the LCO is consistant before we proceed */
+ if (check_local_cx_options(ctx, options) != 0) {
+ LOGP(DLMGCP, LOGL_ERROR,
+ "local CX options: Internal inconsistency in Local Connection Options!\n");
+ return 524;
+ }
+ talloc_free(lco->string);
+ lco->string = talloc_strdup(ctx, options);
+
p_opt = strstr(lco->string, "p:");
if (p_opt && sscanf(p_opt, "p:%d-%d",
&lco->pkt_period_min, &lco->pkt_period_max) == 1)
diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c
index 4dce64c..3fc8bc0 100644
--- a/tests/mgcp/mgcp_test.c
+++ b/tests/mgcp/mgcp_test.c
@@ -1465,6 +1465,108 @@
.num_cat = ARRAY_SIZE(log_categories),
};
+static void test_get_lco_identifier(void)
+{
+ char *test;
+ printf("Testing get_lco_identifier()\n");
+
+ /* Normal case at the beginning */
+ test = "p:10, a:PCMU";
+ printf("%s -> %s\n", test, get_lco_identifier(test));
+
+ test = "p:10, a:PCMU";
+ printf("%s -> %s\n", test, get_lco_identifier(test));
+
+ /* Begin parsing in the middle of the value part of
+ * the previous LCO option value */
+ test = "XXXX, p:10, a:PCMU";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+
+ test = "XXXX,p:10,a:PCMU";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+
+ test = "10,a:PCMU";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+
+ test = "10, a:PCMU";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+
+ test = "10,a: PCMU";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+
+ test = "10 ,a: PCMU";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+
+ /* Begin parsing right at the end of the previous LCO
+ * option value */
+ test = ", a:PCMU";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+
+ test = " a:PCMU";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+
+ /* Empty string, result should be (null) */
+ test = "";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+
+ /* Missing colons, result should be (null) */
+ test = "p10, aPCMU";
+ printf("%s -> %s\n", test, get_lco_identifier(test));
+
+ /* Colon separated from the identifier, result should be (null) */
+ test = "10,a :PCMU";
+ printf("'%s' -> '%s'\n", test, get_lco_identifier(test));
+}
+
+static void test_check_local_cx_options(void *ctx)
+{
+ /* Legal cases */
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10, a:PCMU") == 0);
+ OSMO_ASSERT(check_local_cx_options(ctx, "a:PCMU") == 0);
+ OSMO_ASSERT(check_local_cx_options(ctx, "a:PCMU, p:10, IN:10") == 0);
+ OSMO_ASSERT(check_local_cx_options(ctx, "one:AAA, two:BB, tree:C") ==
+ 0);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10, a:PCMU") == 0);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10, a:G726-32") == 0);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10-20, b:64") == 0);
+ OSMO_ASSERT(check_local_cx_options(ctx, "b:32-64, e:off") == 0);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10, a:PCMU;G726-32") == 0);
+
+ /* Illegal spaces before and after colon */
+ OSMO_ASSERT(check_local_cx_options(ctx, "a:PCMU, p :10") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "a :PCMU, p:10") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p: 10, a:PCMU") == -1);
+
+ /* Check if multiple appearances of LCOs are rejected */
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10, a:PCMU, p:10") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10, a:PCMU, a:PCMU, p:10") ==
+ -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10, p:10") == -1);
+
+ /* Check if empty LCO are rejected */
+ OSMO_ASSERT(check_local_cx_options(ctx, "p: , a:PCMU") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p: , a: PCMU") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10, a: PCMU") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:, a:PCMU") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "p:10, a:") == -1);
+
+ /* Garbeled beginning and ends */
+ OSMO_ASSERT(check_local_cx_options(ctx, ":10, a:10") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "10, a:PCMU") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, ", a:PCMU") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, " a:PCMU") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "a:PCMU,") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "a:PCMU, ") == -1);
+
+ /* Illegal strings */
+ OSMO_ASSERT(check_local_cx_options(ctx, " ") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, "AAA") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, ":,") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, ",:") == -1);
+ OSMO_ASSERT(check_local_cx_options(ctx, ",,,") == -1);
+}
+
int main(int argc, char **argv)
{
void *ctx = talloc_named_const(NULL, 0, "mgcp_test");
@@ -1486,6 +1588,8 @@
test_no_cycle();
test_no_name();
test_osmux_cid();
+ test_get_lco_identifier();
+ test_check_local_cx_options(ctx);
OSMO_ASSERT(talloc_total_size(msgb_ctx) == 0);
OSMO_ASSERT(talloc_total_blocks(msgb_ctx) == 1);
diff --git a/tests/mgcp/mgcp_test.ok b/tests/mgcp/mgcp_test.ok
index d2879ad..e293533 100644
--- a/tests/mgcp/mgcp_test.ok
+++ b/tests/mgcp/mgcp_test.ok
@@ -1104,4 +1104,18 @@
checking response:
using message with patched conn_id for comparison
Response matches our expectations.
+Testing get_lco_identifier()
+p:10, a:PCMU -> p:10, a:PCMU
+p:10, a:PCMU -> p:10, a:PCMU
+'XXXX, p:10, a:PCMU' -> 'p:10, a:PCMU'
+'XXXX,p:10,a:PCMU' -> 'p:10,a:PCMU'
+'10,a:PCMU' -> 'a:PCMU'
+'10, a:PCMU' -> 'a:PCMU'
+'10,a: PCMU' -> 'a: PCMU'
+'10 ,a: PCMU' -> 'a: PCMU'
+', a:PCMU' -> 'a:PCMU'
+' a:PCMU' -> 'a:PCMU'
+'' -> '(null)'
+p10, aPCMU -> (null)
+'10,a :PCMU' -> '(null)'
Done
--
To view, visit https://gerrit.osmocom.org/9474
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iae2fddfa5f2bcfc952f8ab217b3056694e5f7812
Gerrit-Change-Number: 9474
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180606/b4b2fbc2/attachment.htm>