Review Board 1.7.16


[18839] [Applications/app_voicemail] A voicemail password that starts with a '*' results in a invalid mailbox

Review Request #1316 - Created July 11, 2011 and submitted

Matt Jordan
1.8
ASTERISK-17443
Reviewers
asterisk-dev
lmadsen
Asterisk
The bug was originally reported that a voicemail user with a password beginning with the '*' character would be authenticated if their passwords matched, but the mailbox would be set to NULL.  This would cause the voicemail app to create a new mailbox at the root of the voicemail directory path, which would appear as the user's mailbox, albeit with no mail, etc.

Note that another behavior that occurs is if a user enters a voicemail mailbox beginning with '*'.  In that case, the mailbox is truncated to NULL and the user prompted with a password.  Since the mailbox is NULL, the user cannot enter a valid password, and will eventually be forced out of voicemail.

Upon further inspection, Leif noted that a '*' as the first character in either the mailbox or the password is supposed to route the call to extension 'a' if it exists.

In conversations with Russell, it was decided that a mailbox or password starting with '*' should be treated as invalid.  The code change does the following:
1. If an existing voicemail.conf defines a mailbox beginning with a '*', loading voicemail.conf will log a warning that the mailbox is invalid and should be changed.
2. If an existing voicemail.conf defines a password beginning with a '*', loading voicemail.conf will log a warning that the password is invalid and should be changed
3. Any attempt to change a password (either through new user or change password options) to a password beginning with '*' will be rejected
4. If a user logs in with a password beginning with '*', and that password matches the password in voicemail.conf, the vmu object is set to NULL to prevent a 'dummy' mailbox from being created.  This inevitably causes the login attempts to fail.

Note that since the 'reroute' option appeared to be mostly unknown, additional verbose logging was put in to let an admin know that a reroute to extension 'a' was being attempted.
Prior to making changes to load_config / change_password:
1. vm_authenticate was modified to set vmu to NULL if the password began with '*' but extension 'a' does not exist.  This was tested with a mailbox with a password set to '*'; the login attempt failed and no dummy inbox was created.

After the rest of the code changes:

2. A mailbox of *1234 => 1234,... was created.  The mailbox is dropped due to beginning with * and a warning generated.  A user attempting to log in with a mailbox of *1234 is treated as having a mailbox of '\0'.  If extension 'a' is not present, the login attempts will fail.
3. A mailbox of 1234* => *1234,... was created.  The mailbox is valid, but the password is detected as being invalid and a warning generated.  A user will be unable to authenticate with the password if extension 'a' is not defined as the vmu user will be set to NULL.

Diff revision 1 (Latest)

  1. /branches/1.8/apps/app_voicemail.c: Loading...
/branches/1.8/apps/app_voicemail.c
Revision 327640 New Change
[20] 1219 lines
[+20] [+] static char *vm_check_password_shell(char *command, char *buf, size_t len)
1220
static int check_password(struct ast_vm_user *vmu, char *password)
1220
static int check_password(struct ast_vm_user *vmu, char *password)
1221
{
1221
{
1222
	/* check minimum length */
1222
	/* check minimum length */
1223
	if (strlen(password) < minpassword)
1223
	if (strlen(password) < minpassword)
1224
		return 1;
1224
		return 1;

    
   
1225
	/* check that password does not contain '*' character */

    
   
1226
	if (!ast_strlen_zero(password) && password[0] == '*')

    
   
1227
		return 1;
1225
	if (!ast_strlen_zero(ext_pass_check_cmd)) {
1228
	if (!ast_strlen_zero(ext_pass_check_cmd)) {
1226
		char cmd[255], buf[255];
1229
		char cmd[255], buf[255];
1227

    
   
1230

   
1228
		ast_log(AST_LOG_DEBUG, "Verify password policies for %s\n", password);
1231
		ast_log(AST_LOG_DEBUG, "Verify password policies for %s\n", password);
1229

    
   
1232

   
[+20] [20] 69 lines
[+20] [+] static void apply_options(struct ast_vm_user *vmu, const char *options)
1299
{
1302
{
1300
	for (; var; var = var->next) {
1303
	for (; var; var = var->next) {
1301
		if (!strcasecmp(var->name, "vmsecret")) {
1304
		if (!strcasecmp(var->name, "vmsecret")) {
1302
			ast_copy_string(retval->password, var->value, sizeof(retval->password));
1305
			ast_copy_string(retval->password, var->value, sizeof(retval->password));
1303
		} else if (!strcasecmp(var->name, "secret") || !strcasecmp(var->name, "password")) { /* don't overwrite vmsecret if it exists */
1306
		} else if (!strcasecmp(var->name, "secret") || !strcasecmp(var->name, "password")) { /* don't overwrite vmsecret if it exists */
1304
			if (ast_strlen_zero(retval->password))
1307
			if (ast_strlen_zero(retval->password)) {

    
   
1308
				if (!ast_strlen_zero(var->value) && var->value[0] == '*') {

    
   
1309
					ast_log(LOG_WARNING, "Invalid password detected for mailbox %s.  The password"

    
   
1310
						"\n\tmust be reset in voicemail.conf.\n", retval->mailbox);

    
   
1311
				} else {
1305
				ast_copy_string(retval->password, var->value, sizeof(retval->password));
1312
					ast_copy_string(retval->password, var->value, sizeof(retval->password));

    
   
1313
				}

    
   
1314
			}
1306
		} else if (!strcasecmp(var->name, "uniqueid")) {
1315
		} else if (!strcasecmp(var->name, "uniqueid")) {
1307
			ast_copy_string(retval->uniqueid, var->value, sizeof(retval->uniqueid));
1316
			ast_copy_string(retval->uniqueid, var->value, sizeof(retval->uniqueid));
1308
		} else if (!strcasecmp(var->name, "pager")) {
1317
		} else if (!strcasecmp(var->name, "pager")) {
1309
			ast_copy_string(retval->pager, var->value, sizeof(retval->pager));
1318
			ast_copy_string(retval->pager, var->value, sizeof(retval->pager));
1310
		} else if (!strcasecmp(var->name, "email")) {
1319
		} else if (!strcasecmp(var->name, "email")) {
[+20] [20] 8367 lines
[+20] [+] static int vm_authenticate(struct ast_channel *chan, char *mailbox, int mailbox_size,
9678
				ast_verb(3, "Username not entered\n");	
9687
				ast_verb(3, "Username not entered\n");	
9679
				return -1;
9688
				return -1;
9680
			}
9689
			}
9681
		} else if (mailbox[0] == '*') {
9690
		} else if (mailbox[0] == '*') {
9682
			/* user entered '*' */
9691
			/* user entered '*' */

    
   
9692
			ast_verb(4, "Mailbox begins with '*', attempting jump to extension 'a'\n");
9683
			if (ast_exists_extension(chan, chan->context, "a", 1,
9693
			if (ast_exists_extension(chan, chan->context, "a", 1,
9684
				S_COR(chan->caller.id.number.valid, chan->caller.id.number.str, NULL))) {
9694
				S_COR(chan->caller.id.number.valid, chan->caller.id.number.str, NULL))) {
9685
				return -1;
9695
				return -1;
9686
			}
9696
			}

    
   
9697
			ast_verb(4, "Jump to extension 'a' failed; setting mailbox to NULL\n");
9687
			mailbox[0] = '\0';
9698
			mailbox[0] = '\0';
9688
		}
9699
		}
9689

    
   
9700

   
9690
		if (useadsi)
9701
		if (useadsi)
9691
			adsi_password(chan);
9702
			adsi_password(chan);
[+20] [20] 18 lines
[+20] static int vm_authenticate(struct ast_channel *chan, char *mailbox, int mailbox_size,
9710
			if (ast_readstring(chan, password, sizeof(password) - 1, 2000, 10000, "#") < 0) {
9721
			if (ast_readstring(chan, password, sizeof(password) - 1, 2000, 10000, "#") < 0) {
9711
				ast_log(AST_LOG_WARNING, "Unable to read password\n");
9722
				ast_log(AST_LOG_WARNING, "Unable to read password\n");
9712
				return -1;
9723
				return -1;
9713
			} else if (password[0] == '*') {
9724
			} else if (password[0] == '*') {
9714
				/* user entered '*' */
9725
				/* user entered '*' */

    
   
9726
				ast_verb(4, "Password begins with '*', attempting jump to extension 'a'\n");
9715
				if (ast_exists_extension(chan, chan->context, "a", 1,
9727
				if (ast_exists_extension(chan, chan->context, "a", 1,
9716
					S_COR(chan->caller.id.number.valid, chan->caller.id.number.str, NULL))) {
9728
					S_COR(chan->caller.id.number.valid, chan->caller.id.number.str, NULL))) {
9717
					mailbox[0] = '*';
9729
					mailbox[0] = '*';
9718
					return -1;
9730
					return -1;
9719
				}
9731
				}

    
   
9732
				ast_verb(4, "Jump to extension 'a' failed; setting mailbox and user to NULL\n");
9720
				mailbox[0] = '\0';
9733
				mailbox[0] = '\0';

    
   
9734
				/* if the password entered was '*', do not let a user mailbox be created if the extension 'a' is not defined */

    
   
9735
				vmu = NULL;
9721
			}
9736
			}
9722
		}
9737
		}
9723

    
   
9738

   
9724
		if (vmu) {
9739
		if (vmu) {
9725
			passptr = vmu->password;
9740
			passptr = vmu->password;
[+20] [20] 836 lines
[+20] [+] static int vm_exec(struct ast_channel *chan, const char *data)
10562

    
   
10577

   
10563
static struct ast_vm_user *find_or_create(const char *context, const char *box)
10578
static struct ast_vm_user *find_or_create(const char *context, const char *box)
10564
{
10579
{
10565
	struct ast_vm_user *vmu;
10580
	struct ast_vm_user *vmu;
10566

    
   
10581

   

    
   
10582
	if (!ast_strlen_zero(box) && box[0] == '*') {

    
   
10583
		ast_log(LOG_WARNING, "Mailbox %s in context %s begins with '*' character.  The '*' character,"

    
   
10584
				"\n\twhen it is the first character in a mailbox or password, is used to jump to a"

    
   
10585
				"\n\tpredefined extension 'a'.  A mailbox or password beginning with '*' is not valid"

    
   
10586
				"\n\tand will be ignored.\n", box, context);

    
   
10587
		return NULL;

    
   
10588
	}

    
   
10589

   
10567
	AST_LIST_TRAVERSE(&users, vmu, list) {
10590
	AST_LIST_TRAVERSE(&users, vmu, list) {
10568
		if (ast_test_flag((&globalflags), VM_SEARCH) && !strcasecmp(box, vmu->mailbox)) {
10591
		if (ast_test_flag((&globalflags), VM_SEARCH) && !strcasecmp(box, vmu->mailbox)) {
10569
			if (strcasecmp(vmu->context, context)) {
10592
			if (strcasecmp(vmu->context, context)) {
10570
				ast_log(LOG_WARNING, "\nIt has been detected that you have defined mailbox '%s' in separate\
10593
				ast_log(LOG_WARNING, "\nIt has been detected that you have defined mailbox '%s' in separate\
10571
						\n\tcontexts and that you have the 'searchcontexts' option on. This type of\
10594
						\n\tcontexts and that you have the 'searchcontexts' option on. This type of\
[+20] [20] 38 lines
[+20] [+] static int append_mailbox(const char *context, const char *box, const char *data)
10610

    
   
10633

   
10611
	populate_defaults(vmu);
10634
	populate_defaults(vmu);
10612

    
   
10635

   
10613
	stringp = tmp;
10636
	stringp = tmp;
10614
	if ((s = strsep(&stringp, ","))) {
10637
	if ((s = strsep(&stringp, ","))) {

    
   
10638
		if (!ast_strlen_zero(s) && s[0] == '*') {

    
   
10639
			ast_log(LOG_WARNING, "Invalid password detected for mailbox %s.  The password"

    
   
10640
				"\n\tmust be reset in voicemail.conf.\n", box);

    
   
10641
		}

    
   
10642
		/* assign password regardless of validity to prevent NULL password from being assigned */
10615
		ast_copy_string(vmu->password, s, sizeof(vmu->password));
10643
		ast_copy_string(vmu->password, s, sizeof(vmu->password));
10616
	}
10644
	}
10617
	if (stringp && (s = strsep(&stringp, ","))) {
10645
	if (stringp && (s = strsep(&stringp, ","))) {
10618
		ast_copy_string(vmu->fullname, s, sizeof(vmu->fullname));
10646
		ast_copy_string(vmu->fullname, s, sizeof(vmu->fullname));
10619
	}
10647
	}
[+20] [20] 2746 lines
  1. /branches/1.8/apps/app_voicemail.c: Loading...

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.