[Asterisk-code-review] chan_iax2: Prevent crash if dialing RSA-only call without outkey. (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/18363 )

Change subject: chan_iax2: Prevent crash if dialing RSA-only call without outkey.
......................................................................

chan_iax2: Prevent crash if dialing RSA-only call without outkey.

Currently, if attempting to place a call to a peer that only allows
RSA authentication, if we fail to provide an outkey when placing
the call, Asterisk will crash.

This exposes the broader issue that IAX2 is prone to causing a crash
if encryption or decryption is attempted but we never initialized
the encryption and decryption keys. In other words, if the logic
to use encryption in chan_iax2 is not perfectly aligned with the
decision to build keys in the first place, then a crash is not
only possible but probable. This was demonstrated by ASTERISK_29264,
for instance.

This permanently prevents such events from causing a crash by explicitly
checking that keys are initialized properly before setting the flags
to use encryption for the call. Instead of crashing, the call will
now abort.

ASTERISK-30007 #close

Change-Id: If925c3d86099ceac7f621804f2532baac5050c9a
---
M channels/chan_iax2.c
1 file changed, 23 insertions(+), 1 deletion(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Joshua Colp: Approved for Submit



diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index ff235af..6d76dc5 100644
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -6379,6 +6379,18 @@
 	}
 }
 
+static int invalid_key(ast_aes_decrypt_key *ecx)
+{
+	int i;
+	for (i = 0; i < 60; i++) {
+		if (ecx->rd_key[i]) {
+			return 0; /* stop if we encounter anything non-zero */
+		}
+	}
+	/* if ast_aes_encrypt or ast_aes_decrypt is called, then we'll crash when calling AES_encrypt or AES_decrypt */
+	return -1;
+}
+
 static void build_encryption_keys(const unsigned char *digest, struct chan_iax2_pvt *pvt)
 {
 	build_ecx_key(digest, pvt);
@@ -8435,7 +8447,7 @@
 			iax_ie_append_str(ied, IAX_IE_PASSWORD, secret);
 			res = 0;
 		} else
-			ast_log(LOG_NOTICE, "No way to send secret to peer '%s' (their methods: %d)\n", ast_sockaddr_stringify_addr(addr), authmethods);
+			ast_log(LOG_WARNING, "No way to send secret to peer '%s' (their methods: %d)\n", ast_sockaddr_stringify_addr(addr), authmethods);
 	}
 	return res;
 }
@@ -8520,12 +8532,22 @@
 		}
 	}
 
+	if (!(ies->authmethods & (IAX_AUTH_MD5 | IAX_AUTH_PLAINTEXT)) && (ies->authmethods & IAX_AUTH_RSA) && ast_strlen_zero(okey)) {
+		/* If the only thing available is RSA, and we don't have an outkey, we can't do it... */
+		ast_log(LOG_WARNING, "Call terminated. RSA authentication requires an outkey\n");
+		return -1;
+	}
+
 	if (ies->encmethods) {
 		if (ast_strlen_zero(p->secret) &&
 			((ies->authmethods & IAX_AUTH_RSA) || (ies->authmethods & IAX_AUTH_MD5) || (ies->authmethods & IAX_AUTH_PLAINTEXT))) {
 			ast_log(LOG_WARNING, "Call terminated. Encryption requested by peer but no secret available locally\n");
 			return -1;
 		}
+		/* Don't even THINK about trying to encrypt or decrypt anything if we don't have valid keys, for some reason... */
+		/* If either of these happens, it's our fault, not the user's. But we should abort rather than crash. */
+		ast_assert_return(!invalid_key(&p->ecx), -1);
+		ast_assert_return(!invalid_key(&p->dcx), -1);
 		ast_set_flag64(p, IAX_ENCRYPTED | IAX_KEYPOPULATED);
 	} else if (ast_test_flag64(iaxs[callno], IAX_FORCE_ENCRYPT)) {
 		ast_log(LOG_NOTICE, "Call initiated without encryption while forceencryption=yes option is set\n");

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: If925c3d86099ceac7f621804f2532baac5050c9a
Gerrit-Change-Number: 18363
Gerrit-PatchSet: 3
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
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/20220502/af482e06/attachment-0001.html>