[Asterisk-code-review] app voicemail: Fix stack overrun in append mailbox (asterisk[13])

[George Joseph]

George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/10235 )

Change subject: app_voicemail:  Fix stack overrun in append_mailbox
......................................................................

app_voicemail:  Fix stack overrun in append_mailbox

The append_mailbox function wasn't calculating the correct length
to pass to ast_alloca and it wasn't handling the case where context
might be empty.

Found by the Address Sanitizer.

Change-Id: I7eb51c7bd18a7a8dbdba261462a95cc69e84f161
---
M apps/app_voicemail.c
1 file changed, 16 insertions(+), 13 deletions(-)

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



diff --git a/apps/app_voicemail.c b/apps/app_voicemail.c
index f2b6c97..866cfce 100644
--- a/apps/app_voicemail.c
+++ b/apps/app_voicemail.c
@@ -795,11 +795,16 @@
 	unsigned char iobuf[BASEMAXINLINE];
 };
 
+#define MAX_VM_MBOX_ID_LEN (AST_MAX_EXTENSION)
+#define MAX_VM_CONTEXT_LEN (AST_MAX_CONTEXT)
+/* MAX_VM_MAILBOX_LEN allows enough room for the '@' and NULL terminator */
+#define MAX_VM_MAILBOX_LEN (MAX_VM_MBOX_ID_LEN + MAX_VM_CONTEXT_LEN)
+
 /*! Structure for linked list of users
  * Use ast_vm_user_destroy() to free one of these structures. */
 struct ast_vm_user {
-	char context[AST_MAX_CONTEXT];   /*!< Voicemail context */
-	char mailbox[AST_MAX_EXTENSION]; /*!< Mailbox id, unique within vm context */
+	char context[MAX_VM_CONTEXT_LEN];/*!< Voicemail context */
+	char mailbox[MAX_VM_MBOX_ID_LEN];/*!< Mailbox id, unique within vm context */
 	char password[80];               /*!< Secret pin code, numbers only */
 	char fullname[80];               /*!< Full name, for directory app */
 	char *email;                     /*!< E-mail address */
@@ -12192,23 +12197,21 @@
 {
 	size_t len;
 	struct poll_state *poll_state;
-	char *mailbox;
+	char mailbox_full[MAX_VM_MAILBOX_LEN];
 
 	if (ast_strlen_zero(vmu->mailbox)) {
 		ast_log(LOG_ERROR, "Mailbox can't be empty\n");
 		return -1;
 	}
 
-	len = sizeof(vmu->mailbox) + sizeof(vmu->context) + sizeof('@') + 1;
-	mailbox = ast_alloca(len);
-	len = snprintf(mailbox, len, "%s%s%s",
+	len = snprintf(mailbox_full, MAX_VM_MAILBOX_LEN, "%s%s%s",
 		vmu->mailbox,
 		ast_strlen_zero(vmu->context) ? "" : "@",
 		vmu->context);
 
 	len++; /* For NULL terminator */
 
-	poll_state = ao2_find(poll_list, mailbox, OBJ_SEARCH_KEY);
+	poll_state = ao2_find(poll_list, mailbox_full, OBJ_SEARCH_KEY);
 	if (poll_state) {
 		poll_state->marked_used = 1;
 		ao2_ref(poll_state, -1);
@@ -12220,7 +12223,7 @@
 	if (!poll_state) {
 		return -1;
 	}
-	strcpy(poll_state->mailbox, mailbox); /* Safe */
+	strcpy(poll_state->mailbox, mailbox_full); /* Safe */
 	poll_state->marked_used = 1;
 
 	ao2_link_flags(poll_list, poll_state, OBJ_NOLOCK);
@@ -12285,7 +12288,7 @@
 	char *stringp;
 	char *s;
 	struct ast_vm_user *vmu;
-	char *mailbox_full;
+	char mailbox_full[MAX_VM_MAILBOX_LEN];
 	int new = 0, old = 0, urgent = 0;
 	char secretfn[PATH_MAX] = "";
 
@@ -12324,10 +12327,10 @@
 		read_password_from_file(secretfn, vmu->password, sizeof(vmu->password));
 	}
 
-	mailbox_full = ast_alloca(strlen(box) + strlen(context) + 1);
-	strcpy(mailbox_full, box);
-	strcat(mailbox_full, "@");
-	strcat(mailbox_full, context);
+	snprintf(mailbox_full, MAX_VM_MAILBOX_LEN, "%s%s%s",
+		box,
+		ast_strlen_zero(context) ? "" : "@",
+		context);
 
 	inboxcount2(mailbox_full, &urgent, &new, &old);
 #ifdef IMAP_STORAGE

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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: merged
Gerrit-Change-Id: I7eb51c7bd18a7a8dbdba261462a95cc69e84f161
Gerrit-Change-Number: 10235
Gerrit-PatchSet: 3
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180924/034b74a0/attachment.html>