[Asterisk-code-review] res_pjsip_stir_shaken: Fix JSON field ordering and disallowed TN char... (asterisk[18])
Friendly Automation
asteriskteam at digium.com
Mon Apr 10 14:35:39 CDT 2023
Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/20054 )
Change subject: res_pjsip_stir_shaken: Fix JSON field ordering and disallowed TN characters.
......................................................................
res_pjsip_stir_shaken: Fix JSON field ordering and disallowed TN characters.
The current STIR/SHAKEN signing process is inconsistent with the
RFCs in a couple ways that can cause interoperability issues.
RFC8225 specifies that the keys must be ordered lexicographically, but
currently the fields are simply ordered according to the order
in which they were added to the JSON object, which is not
compliant with the RFC and can cause issues with some carriers.
To fix this, we now leverage libjansson's ability to dump a JSON
object sorted by key value, yielding the correct field ordering.
Additionally, telephone numbers must have any leading + prefix removed
and must not contain characters outside of 0-9, *, and # in order
to comply with the RFCs. Numbers are now properly formatted as such.
ASTERISK-30407 #close
Change-Id: Iab76d39447c4b8cf133de85657dba02fda07f9a2
---
M include/asterisk/json.h
M main/json.c
M res/ari/cli.c
M res/res_pjsip_stir_shaken.c
M res/res_stir_shaken.c
5 files changed, 77 insertions(+), 12 deletions(-)
Approvals:
George Joseph: Looks good to me, approved
Friendly Automation: Approved for Submit
diff --git a/include/asterisk/json.h b/include/asterisk/json.h
index 5edc3a9..5b2d614 100644
--- a/include/asterisk/json.h
+++ b/include/asterisk/json.h
@@ -777,6 +777,8 @@
AST_JSON_COMPACT,
/*! Formatted for human readability */
AST_JSON_PRETTY,
+ /*! Keys sorted alphabetically */
+ AST_JSON_SORTED,
};
/*!
@@ -804,6 +806,17 @@
*/
char *ast_json_dump_string_format(struct ast_json *root, enum ast_json_encoding_format format);
+/*!
+ * \brief Encode a JSON value to a string, with its keys sorted.
+ *
+ * Returned string must be freed by calling ast_json_free().
+ *
+ * \param root JSON value.
+ * \return String encoding of \a root.
+ * \retval NULL on error.
+ */
+#define ast_json_dump_string_sorted(root) ast_json_dump_string_format(root, AST_JSON_SORTED)
+
#define ast_json_dump_str(root, dst) ast_json_dump_str_format(root, dst, AST_JSON_COMPACT)
/*!
diff --git a/main/json.c b/main/json.c
index 616b12e..afb653a 100644
--- a/main/json.c
+++ b/main/json.c
@@ -456,8 +456,19 @@
*/
static size_t dump_flags(enum ast_json_encoding_format format)
{
- return format == AST_JSON_PRETTY ?
- JSON_INDENT(2) | JSON_PRESERVE_ORDER : JSON_COMPACT;
+ size_t jansson_dump_flags;
+
+ if (format & AST_JSON_PRETTY) {
+ jansson_dump_flags = JSON_INDENT(2);
+ } else {
+ jansson_dump_flags = JSON_COMPACT;
+ }
+
+ if (format & AST_JSON_SORTED) {
+ jansson_dump_flags |= JSON_SORT_KEYS;
+ }
+
+ return jansson_dump_flags;
}
char *ast_json_dump_string_format(struct ast_json *root, enum ast_json_encoding_format format)
diff --git a/res/ari/cli.c b/res/ari/cli.c
index 9d0eb30..f9d9cec 100644
--- a/res/ari/cli.c
+++ b/res/ari/cli.c
@@ -60,13 +60,10 @@
ast_cli(a->fd, "ARI Status:\n");
ast_cli(a->fd, "Enabled: %s\n", AST_CLI_YESNO(conf->general->enabled));
ast_cli(a->fd, "Output format: ");
- switch (conf->general->format) {
- case AST_JSON_COMPACT:
- ast_cli(a->fd, "compact");
- break;
- case AST_JSON_PRETTY:
+ if (conf->general->format & AST_JSON_PRETTY) {
ast_cli(a->fd, "pretty");
- break;
+ } else {
+ ast_cli(a->fd, "compact");
}
ast_cli(a->fd, "\n");
ast_cli(a->fd, "Auth realm: %s\n", conf->general->auth_realm);
diff --git a/res/res_pjsip_stir_shaken.c b/res/res_pjsip_stir_shaken.c
index 82c8df0..1089e60 100644
--- a/res/res_pjsip_stir_shaken.c
+++ b/res/res_pjsip_stir_shaken.c
@@ -399,7 +399,22 @@
return -1;
}
- ast_copy_pj_str(dest_tn, &uri->user, uri->user.slen + 1);
+ /* Remove everything except 0-9, *, and # in telephone number according to RFC 8224
+ * (required by RFC 8225 as part of canonicalization) */
+ {
+ int i;
+ const char *s = uri->user.ptr;
+ char *new_tn = dest_tn;
+ /* We're only removing characters, if anything, so the buffer is guaranteed to be large enough */
+ for (i = 0; i < uri->user.slen; i++) {
+ if (isdigit(*s) || *s == '#' || *s == '*') { /* Only characters allowed */
+ *new_tn++ = *s;
+ }
+ s++;
+ }
+ *new_tn = '\0';
+ ast_debug(4, "Canonicalized telephone number %.*s -> %s\n", (int) uri->user.slen, uri->user.ptr, dest_tn);
+ }
/* x5u (public key URL), attestation, and origid will be added by ast_stir_shaken_sign */
json = ast_json_pack("{s: {s: s, s: s, s: s}, s: {s: {s: [s]}, s: {s: s}}}",
@@ -427,7 +442,9 @@
}
payload = ast_json_object_get(json, "payload");
- dumped_string = ast_json_dump_string(payload);
+ /* Fields must appear in lexiographic order: https://www.rfc-editor.org/rfc/rfc8588.html#section-6
+ * https://www.rfc-editor.org/rfc/rfc8225.html#section-9 */
+ dumped_string = ast_json_dump_string_sorted(payload);
encoded_payload = ast_base64url_encode_string(dumped_string);
ast_json_free(dumped_string);
if (!encoded_payload) {
diff --git a/res/res_stir_shaken.c b/res/res_stir_shaken.c
index a4eae5b..efb8be9 100644
--- a/res/res_stir_shaken.c
+++ b/res/res_stir_shaken.c
@@ -1228,7 +1228,8 @@
tmp_json = ast_json_object_get(json, "header");
header = ast_json_dump_string(tmp_json);
tmp_json = ast_json_object_get(json, "payload");
- payload = ast_json_dump_string(tmp_json);
+
+ payload = ast_json_dump_string_sorted(tmp_json);
msg_len = strlen(header) + strlen(payload) + 2;
msg = ast_calloc(1, msg_len);
if (!msg) {
@@ -1661,7 +1662,7 @@
tmp_json = ast_json_object_get(json, "header");
header = ast_json_dump_string(tmp_json);
tmp_json = ast_json_object_get(json, "payload");
- payload = ast_json_dump_string(tmp_json);
+ payload = ast_json_dump_string_sorted(tmp_json);
/* Test empty header parameter */
returned_payload = ast_stir_shaken_verify("", payload, (const char *)signed_payload->signature,
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/20054
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: Iab76d39447c4b8cf133de85657dba02fda07f9a2
Gerrit-Change-Number: 20054
Gerrit-PatchSet: 2
Gerrit-Owner: N A <asterisk at phreaknet.org>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20230410/001d081a/attachment.html>
More information about the asterisk-code-review
mailing list