[Asterisk-code-review] res_pjsip_stir_shaken: Fix JSON field ordering and canonicalization. (asterisk[master])

[N A]

N A has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/19897 )


Change subject: res_pjsip_stir_shaken: Fix JSON field ordering and canonicalization.
......................................................................

res_pjsip_stir_shaken: Fix JSON field ordering and canonicalization.

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, a JSON API is added that makes use of libjansson's
ability to dump a JSON object sorted by key value. This results
in the correct ordering in the dumped string.

Additionally, the destination number must be canonicalized, i.e.
have any leading + prefix removed, in order to comply with the RFCs.
This is now done, so the payload contains a properly formatted number.

ASTERISK-30407 #close

Change-Id: Iab76d39447c4b8cf133de85657dba02fda07f9a2
---
M include/asterisk/json.h
M main/json.c
M res/res_pjsip_stir_shaken.c
M res/res_stir_shaken.c
4 files changed, 69 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/97/19897/1

diff --git a/include/asterisk/json.h b/include/asterisk/json.h
index 5edc3a9..0a52312 100644
--- a/include/asterisk/json.h
+++ b/include/asterisk/json.h
@@ -804,6 +804,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.
+ */
+char *ast_json_dump_string_sorted(struct ast_json *root);
+
 #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..7a571bc 100644
--- a/main/json.c
+++ b/main/json.c
@@ -465,6 +465,11 @@
 	return json_dumps((json_t *)root, dump_flags(format));
 }
 
+char *ast_json_dump_string_sorted(struct ast_json *root)
+{
+	return json_dumps((json_t *)root, JSON_SORT_KEYS);
+}
+
 static int write_to_ast_str(const char *buffer, size_t size, void *data)
 {
 	struct ast_str **dst = data;
diff --git a/res/res_pjsip_stir_shaken.c b/res/res_pjsip_stir_shaken.c
index 82c8df0..d4a4ca3 100644
--- a/res/res_pjsip_stir_shaken.c
+++ b/res/res_pjsip_stir_shaken.c
@@ -375,6 +375,7 @@
 	RAII_VAR(char *, encoded_payload, NULL, ast_free);
 	RAII_VAR(char *, combined_str, NULL, ast_free);
 	size_t combined_size;
+	char *canonical_num;
 
 	old_identity = pjsip_msg_find_hdr_by_name(tdata->msg, &identity_str, NULL);
 	if (old_identity) {
@@ -401,11 +402,30 @@
 
 	ast_copy_pj_str(dest_tn, &uri->user, uri->user.slen + 1);
 
+	/* Canonicalize the telephone number according to RFC 8224 (required by RFC 8225) */
+	canonical_num = ast_malloc(strlen(dest_tn) + 1);
+	if (!canonical_num) {
+		return -1;
+	} else {
+		const char *s = dest_tn;
+		char *new = canonical_num;
+		/* We're only removing characters, if anything, so the buffer is guaranteed to be large enough */
+		while (*s) {
+			if (isdigit(*s) || *s == '#' || *s == '*') { /* Only characters allowed */
+				*new++ = *s;
+			}
+			s++;
+		}
+		*new = '\0';
+		ast_debug(4, "Canonicalized telephone number %s -> %s\n", dest_tn, canonical_num);
+	}
+
 	/* 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}}}",
 		"header", "alg", "ES256", "ppt", "shaken", "typ", "passport",
-		"payload", "dest", "tn", dest_tn, "orig", "tn",
+		"payload", "dest", "tn", canonical_num, "orig", "tn",
 		session->id.number.str);
+	ast_free(canonical_num);
 	if (!json) {
 		ast_log(LOG_ERROR, "Failed to allocate memory for STIR/SHAKEN JSON\n");
 		return -1;
@@ -427,7 +447,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..b8ae6c4 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) {

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/19897
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Iab76d39447c4b8cf133de85657dba02fda07f9a2
Gerrit-Change-Number: 19897
Gerrit-PatchSet: 1
Gerrit-Owner: N A <asterisk at phreaknet.org>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20230217/ab9341a9/attachment-0001.html>