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

[N A]

Attention is currently required from: Kevin Harwell.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18363 )

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


Patch Set 2:

(4 comments)

File channels/chan_iax2.c:

https://gerrit.asterisk.org/c/asterisk/+/18363/comment/55b90235_a7ba1f31 
PS1, Line 6382: static int invalid_key(ast_aes_decrypt_key *ecx)
> As it appears 'ecx' is not altered in this function this can be made const if you want.
Done


https://gerrit.asterisk.org/c/asterisk/+/18363/comment/3056e399_6cf6bb30 
PS1, Line 6384: 	int i, c = 0;
              : 	for (i = 0; i < 60; i++) {
              : 		if (ecx->rd_key[i]) {
              : 			break; /* stop if we encounter anything non-zero */
              : 		}
              : 		c += ecx->rd_key[i];
              : 	}
> My eyes may deceive me but this appears to be of no affect on the function, and its result? […]
Sorry, not sure what happened here...

It was supposed to sum up all the elements in array and then return -1 if c was 0. In practice, it seems that rd_key[0] is never supposed to be 0 which is why this function still works, but I intended it to be more lenient and only ensure that the sum of the array is not 0.

In retrospect, most of this is not really necessary. We can simply return 0 inside the loop on positive values and return -1 at the end.


https://gerrit.asterisk.org/c/asterisk/+/18363/comment/11316369_1af9c002 
PS1, Line 6391: 	if (!ecx->rd_key[0]) {
              : 		return -1;	/* if ast_aes_encrypt or ast_aes_decrypt is called, then
              : 				 * we'll crash when calling AES_encrypt or AES_decrypt */
              : 	}
> As this always just checks at the [0] element then I think this can be moved to the beginning of the […]
Ack


https://gerrit.asterisk.org/c/asterisk/+/18363/comment/7eb4b8ee_01f2f055 
PS1, Line 8554: 			ast_log(LOG_ERROR, "BUG! Encryption key is completely empty!\n");
              : 			return -1;
              : 		} else if (invalid_key(&p->dcx)) {
              : 			ast_log(LOG_ERROR, "BUG! Decryption key is completely empty!\n");
> Should these just be 'ast_assert_return's?
Done



-- 
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: 2
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 26 Apr 2022 22:40:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220426/1c4d708f/attachment.html>