From 18c0cafa10e77e776483801edfa45e30a34e6ec5 Mon Sep 17 00:00:00 2001 From: George Joseph Date: Thu, 25 Apr 2024 11:56:15 -0600 Subject: [PATCH] stir_shaken: Fix memory leak, typo in config, tn canonicalization * Fixed possible memory leak in tn_config:tn_get_etn() where we weren't releasing etn if tn or eprofile were null. * We now canonicalize TNs before using them for lookups or adding them to Identity headers. * Fixed a typo in stir_shaken.conf.sample. Resolves: #716 (cherry picked from commit b7ed77a7c5e2db6883e16d14a01d886dda1a0d8f) --- configs/samples/stir_shaken.conf.sample | 2 +- res/res_pjsip_stir_shaken.c | 18 +----------- res/res_stir_shaken/attestation.c | 19 +++++++------ res/res_stir_shaken/common_config.c | 38 +++++++++++++++++++++++++ res/res_stir_shaken/common_config.h | 18 ++++++++++++ res/res_stir_shaken/tn_config.c | 1 + res/res_stir_shaken/verification.c | 6 ++-- 7 files changed, 74 insertions(+), 28 deletions(-) diff --git a/configs/samples/stir_shaken.conf.sample b/configs/samples/stir_shaken.conf.sample index 5a15d0f4d0..c7ee89230e 100644 --- a/configs/samples/stir_shaken.conf.sample +++ b/configs/samples/stir_shaken.conf.sample @@ -413,7 +413,7 @@ Must be set to "profile" Default: none --- endpoint_behhavior-------------------------------------------------- +-- endpoint_behavior-------------------------------------------------- Actions to be performed for endpoints referencing this profile. Must be one of: - off - diff --git a/res/res_pjsip_stir_shaken.c b/res/res_pjsip_stir_shaken.c index fa2103a857..f64152f3a2 100644 --- a/res/res_pjsip_stir_shaken.c +++ b/res/res_pjsip_stir_shaken.c @@ -360,23 +360,7 @@ static char *get_dest_tn(pjsip_tx_data *tdata, const char *tag) "%s: Failed to allocate memory for dest_tn\n", tag); } - /* 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_trace(2, "Canonicalized telephone number " PJSTR_PRINTF_SPEC " -> %s\n", - PJSTR_PRINTF_VAR(uri->user), dest_tn); - } + ast_copy_pj_str(dest_tn, &uri->user, uri->user.slen + 1); SCOPE_EXIT_RTN_VALUE(dest_tn, "%s: Done\n", tag); } diff --git a/res/res_stir_shaken/attestation.c b/res/res_stir_shaken/attestation.c index 0583fdbaa0..91e688d16d 100644 --- a/res/res_stir_shaken/attestation.c +++ b/res/res_stir_shaken/attestation.c @@ -72,14 +72,17 @@ enum ast_stir_shaken_as_response_code RAII_VAR(struct profile_cfg *, eprofile, NULL, ao2_cleanup); RAII_VAR(struct attestation_cfg *, as_cfg, NULL, ao2_cleanup); RAII_VAR(struct tn_cfg *, etn, NULL, ao2_cleanup); + RAII_VAR(char *, canon_dest_tn , canonicalize_tn_alloc(dest_tn), ast_free); + RAII_VAR(char *, canon_orig_tn , canonicalize_tn_alloc(orig_tn), ast_free); + SCOPE_ENTER(3, "%s: Enter\n", tag); - if (ast_strlen_zero(orig_tn)) { + if (!canon_orig_tn) { SCOPE_EXIT_LOG_RTN_VALUE(AST_STIR_SHAKEN_AS_INVALID_ARGUMENTS, LOG_ERROR, "%s: Must provide caller_id/orig_tn\n", tag); } - if (ast_strlen_zero(dest_tn)) { + if (!canon_dest_tn) { SCOPE_EXIT_LOG_RTN_VALUE(AST_STIR_SHAKEN_AS_INVALID_ARGUMENTS, LOG_ERROR, "%s: Must provide dest_tn\n", tag); } @@ -117,10 +120,10 @@ enum ast_stir_shaken_as_response_code "%s: Disabled by profile\n", tag); } - etn = tn_get_etn(orig_tn, eprofile); + etn = tn_get_etn(canon_orig_tn, eprofile); if (!etn) { SCOPE_EXIT_RTN_VALUE(AST_STIR_SHAKEN_AS_DISABLED, - "%s: No tn for orig_tn '%s'\n", tag, orig_tn); + "%s: No tn for orig_tn '%s'\n", tag, canon_orig_tn); } /* We don't need eprofile or as_cfg anymore so let's clean em up */ @@ -140,13 +143,13 @@ enum ast_stir_shaken_as_response_code if (ast_strlen_zero(etn->acfg_common.public_cert_url)) { SCOPE_EXIT_LOG_RTN_VALUE(AST_STIR_SHAKEN_AS_NO_PUBLIC_CERT_URL_AVAIL, LOG_ERROR, "%s: No public cert url in tn %s, profile or attestation objects\n", - tag, orig_tn); + tag, canon_orig_tn); } if (etn->acfg_common.raw_key_length == 0) { SCOPE_EXIT_LOG_RTN_VALUE(AST_STIR_SHAKEN_AS_NO_PRIVATE_KEY_AVAIL, LOG_ERROR, "%s: No private key in tn %s, profile or attestation objects\n", - orig_tn, tag); + canon_orig_tn, tag); } ctx = ao2_alloc_options(sizeof(*ctx), ctx_destructor, @@ -166,12 +169,12 @@ enum ast_stir_shaken_as_response_code LOG_ERROR, "%s: Unable to allocate memory for ctx\n", tag); } - if (ast_string_field_set(ctx, orig_tn, orig_tn) != 0) { + if (ast_string_field_set(ctx, orig_tn, canon_orig_tn) != 0) { SCOPE_EXIT_LOG_RTN_VALUE(AST_STIR_SHAKEN_AS_INTERNAL_ERROR, LOG_ERROR, "%s: Unable to allocate memory for ctx\n", tag); } - if (ast_string_field_set(ctx, dest_tn, dest_tn)) { + if (ast_string_field_set(ctx, dest_tn, canon_dest_tn)) { SCOPE_EXIT_LOG_RTN_VALUE(AST_STIR_SHAKEN_AS_INTERNAL_ERROR, LOG_ERROR, "%s: Unable to allocate memory for ctx\n", tag); } diff --git a/res/res_stir_shaken/common_config.c b/res/res_stir_shaken/common_config.c index f7446b7cea..f753b41ca6 100644 --- a/res/res_stir_shaken/common_config.c +++ b/res/res_stir_shaken/common_config.c @@ -351,3 +351,41 @@ int common_config_load(void) SCOPE_EXIT_RTN_VALUE(AST_MODULE_LOAD_SUCCESS, "Stir Shaken Load Done\n"); } + +/* Remove everything except 0-9, *, and # in telephone number according to RFC 8224 + * (required by RFC 8225 as part of canonicalization) */ +char *canonicalize_tn(const char *tn, char *dest_tn) +{ + int i; + const char *s = tn; + size_t len = tn ? strlen(tn) : 0; + char *new_tn = dest_tn; + SCOPE_ENTER(3, "tn: %s\n", S_OR(tn, "(null)")); + + if (ast_strlen_zero(tn)) { + *dest_tn = '\0'; + SCOPE_EXIT_RTN_VALUE(NULL, "Empty TN\n"); + } + + if (!dest_tn) { + SCOPE_EXIT_RTN_VALUE(NULL, "No destination buffer\n"); + } + + for (i = 0; i < len; i++) { + if (isdigit(*s) || *s == '#' || *s == '*') { /* Only characters allowed */ + *new_tn++ = *s; + } + s++; + } + *new_tn = '\0'; + SCOPE_EXIT_RTN_VALUE(dest_tn, "Canonicalized '%s' -> '%s'\n", tn, dest_tn); +} + +char *canonicalize_tn_alloc(const char *tn) +{ + char *canon_tn = ast_strlen_zero(tn) ? NULL : ast_malloc(strlen(tn) + 1); + if (!canon_tn) { + return NULL; + } + return canonicalize_tn(tn, canon_tn); +} diff --git a/res/res_stir_shaken/common_config.h b/res/res_stir_shaken/common_config.h index c0e56b9a91..b4154757c3 100644 --- a/res/res_stir_shaken/common_config.h +++ b/res/res_stir_shaken/common_config.h @@ -564,5 +564,23 @@ int config_object_cli_show(void *obj, void *arg, void *data, int flags); */ char *config_object_tab_complete_name(const char *word, struct ao2_container *container); +/*! + * \brief Canonicalize a TN + * + * \param tn TN to canonicalize + * \param dest_tn Pointer to destination buffer to receive the new TN + * + * \retval dest_tn or NULL on failure + */ +char *canonicalize_tn(const char *tn, char *dest_tn); + +/*! + * \brief Canonicalize a TN into nre buffer + * + * \param tn TN to canonicalize + * + * \retval dest_tn (which must be freed with ast_free) or NULL on failure + */ +char *canonicalize_tn_alloc(const char *tn); #endif /* COMMON_CONFIG_H_ */ diff --git a/res/res_stir_shaken/tn_config.c b/res/res_stir_shaken/tn_config.c index 123b0519d8..89b9f47be5 100644 --- a/res/res_stir_shaken/tn_config.c +++ b/res/res_stir_shaken/tn_config.c @@ -122,6 +122,7 @@ struct tn_cfg *tn_get_etn(const char *id, struct profile_cfg *eprofile) int rc = 0; if (!tn || !eprofile || !etn) { + ao2_cleanup(etn); return NULL; } diff --git a/res/res_stir_shaken/verification.c b/res/res_stir_shaken/verification.c index f8d21cfdf4..4be93a0975 100644 --- a/res/res_stir_shaken/verification.c +++ b/res/res_stir_shaken/verification.c @@ -655,6 +655,8 @@ enum ast_stir_shaken_vs_response_code RAII_VAR(struct ast_stir_shaken_vs_ctx *, ctx, NULL, ao2_cleanup); RAII_VAR(struct profile_cfg *, profile, NULL, ao2_cleanup); RAII_VAR(struct verification_cfg *, vs, NULL, ao2_cleanup); + RAII_VAR(char *, canon_caller_id , canonicalize_tn_alloc(caller_id), ast_free); + const char *t = S_OR(tag, S_COR(chan, ast_channel_name(chan), "")); SCOPE_ENTER(3, "%s: Enter\n", t); @@ -663,7 +665,7 @@ enum ast_stir_shaken_vs_response_code LOG_ERROR, "%s: Must provide tag\n", t); } - if (ast_strlen_zero(caller_id)) { + if (ast_strlen_zero(canon_caller_id)) { SCOPE_EXIT_LOG_RTN_VALUE(AST_STIR_SHAKEN_VS_INVALID_ARGUMENTS, LOG_ERROR, "%s: Must provide caller_id\n", t); } @@ -705,7 +707,7 @@ enum ast_stir_shaken_vs_response_code } ctx->chan = chan; - if (ast_string_field_set(ctx, caller_id, caller_id) != 0) { + if (ast_string_field_set(ctx, caller_id, canon_caller_id) != 0) { SCOPE_EXIT_RTN_VALUE(AST_STIR_SHAKEN_VS_INTERNAL_ERROR); }