[Asterisk-code-review] res_pjsip: allow TLS verification of wildcard cert-bearing servers (asterisk[certified/18.9])
Kevin Harwell
asteriskteam at digium.com
Thu Jun 30 16:19:45 CDT 2022
Kevin Harwell has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/18682 )
Change subject: res_pjsip: allow TLS verification of wildcard cert-bearing servers
......................................................................
res_pjsip: allow TLS verification of wildcard cert-bearing servers
Rightly the use of wildcards in certificates is disallowed in accordance
with RFC5922. However, RFC2818 does make some allowances with regards to
their use when using subject alt names with DNS name types.
As such this patch creates a new setting for TLS transports called
'allow_wildcard_certs', which when it and 'verify_server' are both enabled
allows DNS name types, as well as the common name that start with '*.'
to match as a wildcard.
For instance: *.example.com
will match for: foo.example.com
Partial matching is not allowed, e.g. f*.example.com, foo.*.com, etc...
And the starting wildcard only matches for a single level.
For instance: *.example.com
will NOT match for: foo.bar.example.com
The new setting is disabled by default.
ASTERISK-30072 #close
Change-Id: If0be3fdab2e09c2a66bb54824fca406ebaac3da4
---
M configs/samples/pjsip.conf.sample
A contrib/ast-db-manage/config/versions/58e440314c2a_allow_wildcard_certs.py
A doc/CHANGES-staging/allow_wildcard_certs.txt
M include/asterisk/res_pjsip.h
M res/res_pjsip.c
M res/res_pjsip/config_transport.c
M res/res_pjsip/pjsip_transport_events.c
7 files changed, 218 insertions(+), 2 deletions(-)
Approvals:
Joshua Colp: Looks good to me, but someone else must approve
Kevin Harwell: Looks good to me, approved; Approved for Submit
diff --git a/configs/samples/pjsip.conf.sample b/configs/samples/pjsip.conf.sample
index 94e1776..498b8d4 100644
--- a/configs/samples/pjsip.conf.sample
+++ b/configs/samples/pjsip.conf.sample
@@ -1091,6 +1091,15 @@
; URI is not a hostname, the saved transport will be
; used and the 'x-ast-txp' parameter stripped from the
; outgoing packet.
+;allow_wildcard_certs=no ; In conjunction with verify_server, if 'yes' allow use
+ ; of wildcards, i.e. '*.' in certs for common, and
+ ; subject alt names of type DNS for TLS transport
+ ; types. Note, names must start with the wildcard.
+ ; Partial wildcards, e.g. 'f*.example.com' and
+ ; 'foo.*.com' are disallowed. As well, names only
+ ; match against a single level meaning '*.example.com'
+ ; matches 'foo.example.com', but not
+ ; 'foo.bar.example.com'. Defaults to 'no'.
;==========================AOR SECTION OPTIONS=========================
;[aor]
diff --git a/contrib/ast-db-manage/config/versions/58e440314c2a_allow_wildcard_certs.py b/contrib/ast-db-manage/config/versions/58e440314c2a_allow_wildcard_certs.py
new file mode 100644
index 0000000..7aafcb2
--- /dev/null
+++ b/contrib/ast-db-manage/config/versions/58e440314c2a_allow_wildcard_certs.py
@@ -0,0 +1,29 @@
+"""allow_wildcard_certs
+
+Revision ID: 58e440314c2a
+Revises: 0bee61aa9425
+Create Date: 2022-05-12 12:15:55.343743
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '58e440314c2a'
+down_revision = 'a06d8f8462d9'
+
+from alembic import op
+import sqlalchemy as sa
+from sqlalchemy.dialects.postgresql import ENUM
+
+YESNO_NAME = 'yesno_values'
+YESNO_VALUES = ['yes', 'no']
+
+def upgrade():
+ yesno_values = ENUM(*YESNO_VALUES, name=YESNO_NAME, create_type=False)
+
+ op.add_column('ps_transports', sa.Column('allow_wildcard_certs', type_=yesno_values))
+
+
+def downgrade():
+ if op.get_context().bind.dialect.name == 'mssql':
+ op.drop_constraint('ck_ps_transports_allow_wildcard_certs_yesno_values', 'ps_transports')
+ op.drop_column('ps_transports', 'allow_wildcard_certs')
diff --git a/doc/CHANGES-staging/allow_wildcard_certs.txt b/doc/CHANGES-staging/allow_wildcard_certs.txt
new file mode 100644
index 0000000..29a53dd
--- /dev/null
+++ b/doc/CHANGES-staging/allow_wildcard_certs.txt
@@ -0,0 +1,9 @@
+Subject: res_pjsip
+
+A new transport option 'allow_wildcard_certs' has been added that when it
+and 'verify_server' are both set to 'yes', enables verification against
+wildcards, i.e. '*.' in certs for common, and subject alt names of type DNS
+for TLS transport types. Names must start with the wildcard. Partial wildcards,
+e.g. 'f*.example.com' and 'foo.*.com' are not allowed. As well, names only
+match against a single level meaning '*.example.com' matches 'foo.example.com',
+but not 'foo.bar.example.com'.
diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h
index fba59f4..cfc3b04 100644
--- a/include/asterisk/res_pjsip.h
+++ b/include/asterisk/res_pjsip.h
@@ -173,6 +173,14 @@
* \since 17.0.0
*/
struct ast_sip_service_route_vector *service_routes;
+ /*!
+ * Disregard RFC5922 7.2, and allow wildcard certs (TLS only)
+ */
+ int allow_wildcard_certs;
+ /*!
+ * If true, fail if server certificate cannot verify (TLS only)
+ */
+ int verify_server;
};
#define ast_sip_transport_is_nonlocal(transport_state, addr) \
diff --git a/res/res_pjsip.c b/res/res_pjsip.c
index 697767d..c102782 100644
--- a/res/res_pjsip.c
+++ b/res/res_pjsip.c
@@ -1811,6 +1811,18 @@
in-progress calls.</para>
</description>
</configOption>
+ <configOption name="allow_wildcard_certs" default="false">
+ <synopsis>Allow use of wildcards in certificates (TLS ONLY)</synopsis>
+ <description>
+ <para>In combination with verify_server, when enabled allow use of wildcards,
+ i.e. '*.' in certs for common,and subject alt names of type DNS for TLS
+ transport types. Names must start with the wildcard. Partial wildcards, e.g.
+ 'f*.example.com' and 'foo.*.com' are not allowed. As well, names only match
+ against a single level meaning '*.example.com' matches 'foo.example.com',
+ but not 'foo.bar.example.com'.
+ </para>
+ </description>
+ </configOption>
<configOption name="symmetric_transport" default="no">
<synopsis>Use the same transport for outgoing requests as incoming ones.</synopsis>
<description>
diff --git a/res/res_pjsip/config_transport.c b/res/res_pjsip/config_transport.c
index d0a28cd..da05c25 100644
--- a/res/res_pjsip/config_transport.c
+++ b/res/res_pjsip/config_transport.c
@@ -833,6 +833,16 @@
&temp_state->state->host, NULL, transport->async_operations,
&temp_state->state->factory);
}
+
+ if (res == PJ_SUCCESS) {
+ temp_state->state->factory->info = pj_pool_alloc(
+ temp_state->state->factory->pool, (strlen(transport_id) + 1));
+ /*
+ * Store transport id on the factory instance so it can be used
+ * later to look up the transport state.
+ */
+ sprintf(temp_state->state->factory->info, "%s", transport_id);
+ }
#else
ast_log(LOG_ERROR, "Transport: %s: PJSIP has not been compiled with TLS transport support, ensure OpenSSL development packages are installed\n",
ast_sorcery_object_get_id(obj));
@@ -1057,11 +1067,13 @@
}
if (!strcasecmp(var->name, "verify_server")) {
- state->tls.verify_server = ast_true(var->value) ? PJ_TRUE : PJ_FALSE;
+ state->verify_server = ast_true(var->value);
} else if (!strcasecmp(var->name, "verify_client")) {
state->tls.verify_client = ast_true(var->value) ? PJ_TRUE : PJ_FALSE;
} else if (!strcasecmp(var->name, "require_client_cert")) {
state->tls.require_client_cert = ast_true(var->value) ? PJ_TRUE : PJ_FALSE;
+ } else if (!strcasecmp(var->name, "allow_wildcard_certs")) {
+ state->allow_wildcard_certs = ast_true(var->value);
} else {
return -1;
}
@@ -1078,7 +1090,7 @@
return -1;
}
- *buf = ast_strdup(AST_YESNO(state->tls.verify_server));
+ *buf = ast_strdup(AST_YESNO(state->verify_server));
return 0;
}
@@ -1111,6 +1123,20 @@
return 0;
}
+static int allow_wildcard_certs_to_str(const void *obj, const intptr_t *args, char **buf)
+{
+ struct ast_sip_transport_state *state = find_state_by_transport(obj);
+
+ if (!state) {
+ return -1;
+ }
+
+ *buf = ast_strdup(AST_YESNO(state->allow_wildcard_certs));
+ ao2_ref(state, -1);
+
+ return 0;
+}
+
/*! \brief Custom handler for TLS method setting */
static int transport_tls_method_handler(const struct aco_option *opt, struct ast_variable *var, void *obj)
{
@@ -1653,6 +1679,7 @@
ast_sorcery_object_field_register_custom(sorcery, "transport", "verify_server", "", transport_tls_bool_handler, verify_server_to_str, NULL, 0, 0);
ast_sorcery_object_field_register_custom(sorcery, "transport", "verify_client", "", transport_tls_bool_handler, verify_client_to_str, NULL, 0, 0);
ast_sorcery_object_field_register_custom(sorcery, "transport", "require_client_cert", "", transport_tls_bool_handler, require_client_cert_to_str, NULL, 0, 0);
+ ast_sorcery_object_field_register_custom(sorcery, "transport", "allow_wildcard_certs", "", transport_tls_bool_handler, allow_wildcard_certs_to_str, NULL, 0, 0);
ast_sorcery_object_field_register_custom(sorcery, "transport", "method", "", transport_tls_method_handler, tls_method_to_str, NULL, 0, 0);
#if defined(PJ_HAS_SSL_SOCK) && PJ_HAS_SSL_SOCK != 0
ast_sorcery_object_field_register_custom(sorcery, "transport", "cipher", "", transport_tls_cipher_handler, transport_tls_cipher_to_str, NULL, 0, 0);
diff --git a/res/res_pjsip/pjsip_transport_events.c b/res/res_pjsip/pjsip_transport_events.c
index 4df1d5e..a816f46 100644
--- a/res/res_pjsip/pjsip_transport_events.c
+++ b/res/res_pjsip/pjsip_transport_events.c
@@ -142,6 +142,122 @@
}
}
+static void verify_log_result(int log_level, const pjsip_transport *transport,
+ pj_uint32_t verify_status)
+{
+ const char *status[32];
+ unsigned int count;
+ unsigned int i;
+
+ count = ARRAY_LEN(status);
+
+ if (pj_ssl_cert_get_verify_status_strings(verify_status, status, &count) != PJ_SUCCESS) {
+ ast_log(LOG_ERROR, "Error retrieving certificate verification result(s)\n");
+ return;
+ }
+
+ for (i = 0; i < count; ++i) {
+ ast_log(log_level, _A_, "Transport '%s' to remote '%.*s' - %s\n", transport->factory->info,
+ (int)pj_strlen(&transport->remote_name.host), pj_strbuf(&transport->remote_name.host),
+ status[i]);
+ }
+}
+
+static int verify_cert_name(const pj_str_t *local, const pj_str_t *remote)
+{
+ const char *p;
+ pj_ssize_t size;
+
+ ast_debug(3, "Verify certificate name: local = %.*s, remote = %.*s\n",
+ (unsigned int)pj_strlen(local), pj_strbuf(local),
+ (unsigned int)pj_strlen(remote), pj_strbuf(remote));
+
+ if (!pj_stricmp(remote, local)) {
+ return 1;
+ }
+
+ if (pj_strnicmp2(remote, "*.", 2)) {
+ return 0;
+ }
+
+ p = pj_strchr(local, '.');
+ if (!p) {
+ return 0;
+ }
+
+ size = pj_strbuf(local) + pj_strlen(local) - ++p;
+
+ return size == pj_strlen(remote) - 2 ?
+ !pj_memcmp(pj_strbuf(remote) + 2, p, size) : 0;
+}
+
+static int verify_cert_names(const pj_str_t *host, const pj_ssl_cert_info *remote)
+{
+ unsigned int i;
+
+ for (i = 0; i < remote->subj_alt_name.cnt; ++i) {
+ /*
+ * DNS is the only type we're matching wildcards against,
+ * so only recheck those.
+ */
+ if (remote->subj_alt_name.entry[i].type == PJ_SSL_CERT_NAME_DNS
+ && verify_cert_name(host, &remote->subj_alt_name.entry[i].name)) {
+ return 1;
+ }
+ }
+
+ return verify_cert_name(host, &remote->subject.cn);
+}
+
+static int transport_tls_verify(const pjsip_transport *transport,
+ const pjsip_tls_state_info *state_info)
+{
+ pj_uint32_t verify_status;
+ const struct ast_sip_transport_state *state;
+
+ if (transport->dir == PJSIP_TP_DIR_INCOMING) {
+ return 1;
+ }
+
+ /* transport_id should always be in factory info (see config_transport) */
+ ast_assert(!ast_strlen_zero(transport->factory->info));
+
+ state = ast_sip_get_transport_state(transport->factory->info);
+ if (!state) {
+ /*
+ * There should always be an associated state, but if for some
+ * reason there is not then fail verification
+ */
+ ast_log(LOG_ERROR, "Transport state not found for '%s'\n", transport->factory->info);
+ return 0;
+ }
+
+ verify_status = state_info->ssl_sock_info->verify_status;
+
+ /*
+ * By this point pjsip has already completed its verification process. If
+ * there was a name matching error it could be because they disallow wildcards.
+ * If this transport has been configured to allow wildcards then we'll need
+ * to re-check the name(s) for such.
+ */
+ if (state->allow_wildcard_certs &&
+ (verify_status & PJ_SSL_CERT_EIDENTITY_NOT_MATCH)) {
+ if (verify_cert_names(&transport->remote_name.host,
+ state_info->ssl_sock_info->remote_cert_info)) {
+ /* A name matched a wildcard, so clear the error */
+ verify_status &= ~PJ_SSL_CERT_EIDENTITY_NOT_MATCH;
+ }
+ }
+
+ if (state->verify_server && verify_status != PJ_SSL_CERT_ESUCCESS) {
+ verify_log_result(__LOG_ERROR, transport, verify_status);
+ return 0;
+ }
+
+ verify_log_result(__LOG_NOTICE, transport, verify_status);
+ return 1;
+}
+
/*! \brief Callback invoked when transport state changes occur */
static void transport_state_callback(pjsip_transport *transport,
pjsip_transport_state state, const pjsip_transport_state_info *info)
@@ -157,6 +273,12 @@
transport->obj_name, transport_state2str(state));
switch (state) {
case PJSIP_TP_STATE_CONNECTED:
+ if (PJSIP_TRANSPORT_IS_SECURE(transport) &&
+ !transport_tls_verify(transport, info->ext_info)) {
+ pjsip_transport_shutdown(transport);
+ return;
+ }
+
monitored = ao2_alloc_options(sizeof(*monitored),
transport_monitor_dtor, AO2_ALLOC_OPT_LOCK_NOLOCK);
if (!monitored) {
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18682
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: certified/18.9
Gerrit-Change-Id: If0be3fdab2e09c2a66bb54824fca406ebaac3da4
Gerrit-Change-Number: 18682
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220630/9b88a0ba/attachment-0001.html>
More information about the asterisk-code-review
mailing list