Bug 300 - Segmentation fault in sudo's 'glob' usage.
Segmentation fault in sudo's 'glob' usage.
Status: RESOLVED FIXED
Product: Sudo
Classification: Unclassified
Component: Sudo
1.6.9
PC Linux
: low normal
Assigned To: Todd C. Miller
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-10 17:01 MDT by Patrick Williams
Modified: 2008-09-11 07:12 MDT (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Williams 2008-09-10 17:01:53 MDT
On a Gentoo AMD64 installation with sudo-1.6.9p16, I experience a segmentation fault when the /etc/sudoers file contains a glob with a very large number of results.

Example:

ALL ALL=(root) NOPASSWD: /usr/portage/*/*/


$ sudo
Segmentation fault


Bug is in parse.c and is also present in 1.7.0 development release.  In 'command_matches', there is a globfree call performed while the function still has a pointer into the glob's buffer.  After the globfree, a dereference of the pointer is performed.  

If the glob has a large number of results, the buffer could be allocated into its own heap region. Then the globfree is performed the region is released.  Typically, I imagine the glob is small enough that the memory is in the normal heap region and the pointer is still good enough at the point of the memory access.

--- sudo-1.6.9p17_fix/parse.c	2008-09-10 15:25:23.000000000 -0500
+++ sudo-1.6.9p17/parse.c	2008-02-09 08:44:48.000000000 -0600
@@ -311,21 +311,19 @@ command_matches(sudoers_cmnd, sudoers_ar
 		continue;
 	    if (user_stat->st_dev == sudoers_stat.st_dev &&
 		user_stat->st_ino == sudoers_stat.st_ino) {
 		efree(safe_cmnd);
 		safe_cmnd = estrdup(*ap);
 		break;
 	    }
 	}
-	if (*ap == NULL) { 
-	    globfree(&gl); 
-	    return(FALSE); 
-	}
 	globfree(&gl);
+	if (*ap == NULL)
+	    return(FALSE);
 
 	if (!sudoers_args ||
 	    (!user_args && sudoers_args && !strcmp("\"\"", sudoers_args)) ||
 	    (sudoers_args &&
 	     fnmatch(sudoers_args, user_args ? user_args : "", 0) == 0)) {
 	    efree(safe_cmnd);
 	    safe_cmnd = estrdup(user_cmnd);
 	    return(TRUE);
Comment 1 Todd C. Miller 2008-09-11 07:12:30 MDT
Thanks for the detailed report and patch.  I've committed a change that reworks the loop a bit so there is no need to deref that pointer to determine whether we got a match or not.