xorl %eax, %eax

CVE-2009-1886: SAMBA smbclient Format String Vulnerabilities

with 2 comments

This issue was reported by Reinhard Nißl to the SAMBA team and it affects Samba 3.2.0 through 3.2.12. However, the attacker should either have the ability to execute ‘smbclient’ on the vulnerable system or trick the victim into requesting a file with specially crafted filename. The buggy code can be found at source/client/client.c.

/****************************************************************************
 Change directory - inner section.
****************************************************************************/

static int do_cd(const char *new_dir)
{
        char *newdir = NULL;
      ...
        TALLOC_CTX *ctx = talloc_stackframe();
      ...
        /* Ensure cur_dir ends in a DIRSEP */
        if ((new_cd[0] != '') && (*(new_cd+strlen(new_cd)-1) != CLI_DIRSEP_CHAR)) {
                new_cd = talloc_asprintf_append(new_cd, CLI_DIRSEP_STR);
                if (!new_cd) {
                        goto out;
                }
        }
        client_set_cur_dir(new_cd);
      ...
        return ret;
}

This function is used when changine a directory. Variable new_cd was previously processed and contains the requested, “new” directory. If this is not NULL and its final character is not CLI_DIRSEP_CHAR which is simply:

static char CLI_DIRSEP_CHAR = '\\';
static char CLI_DIRSEP_STR[] = { '\\', '' };

Then, it uses talloc_asprintf_append() to append CLI_DIRSEP_STR string to new_cd. The latter routine can be found at source/lib/talloc/talloc.c.

/*
  Realloc @p s to append the formatted result of @p fmt and return @p
  s, which may have moved.  Good for gradually accumulating output
  into a string buffer.
 */
char *talloc_asprintf_append(char *s, const char *fmt, ...)
{
        va_list ap;

        va_start(ap, fmt);
        s = talloc_vasprintf_append(s, fmt, ap);
        va_end(ap);
        return s;
}

Obviously, the above invocation lacks of a format string specifier. This was patched like this:

 	if ((new_cd[0] != '') && (*(new_cd+strlen(new_cd)-1) != CLI_DIRSEP_CHAR)) {
-		new_cd = talloc_asprintf_append(new_cd, CLI_DIRSEP_STR);
+		new_cd = talloc_asprintf_append(new_cd, "%s", CLI_DIRSEP_STR);
 		if (!new_cd) {

A more interesting one is located at cmd_dir() which is used when retrieving a directory listing. The concept is the same.

/****************************************************************************
 Get a directory listing.
****************************************************************************/

static int cmd_dir(void)
{
        TALLOC_CTX *ctx = talloc_tos();
        uint16 attribute = aDIR | aSYSTEM | aHIDDEN;
        char *mask = NULL;
        char *buf = NULL;
      ...
        if (next_token_talloc(ctx, &cmd_ptr,&buf,NULL)) {
                normalize_name(buf);
                if (*buf == CLI_DIRSEP_CHAR) {
                        mask = talloc_strdup(ctx, buf);
                } else {
                        mask = talloc_asprintf_append(mask, buf);
                }
      ...
        return rc;
}

Once again, string “buf” that contains user controlled data is used with no format string in the else clause. This was patched in a similar manner.

 		} else {
-			mask = talloc_asprintf_append(mask, buf);
+			mask = talloc_asprintf_append(mask, "%s", buf);
 		}

The next one can be found in the following function.

/****************************************************************************
 Get a directory listing.
****************************************************************************/

static int cmd_du(void)
{
        TALLOC_CTX *ctx = talloc_tos();
        uint16 attribute = aDIR | aSYSTEM | aHIDDEN;
        char *mask = NULL;
        char *buf = NULL;
        int rc = 1;

        dir_total = 0;
        mask = talloc_strdup(ctx, client_get_cur_dir());
        if (!mask) {
                return 1;
        }
        if ((mask[0] != '') && (mask[strlen(mask)-1]!=CLI_DIRSEP_CHAR)) {
                mask = talloc_asprintf_append(mask, CLI_DIRSEP_STR);
                if (!mask) {
                        return 1;
                }
        }
     ...
        if (next_token_talloc(ctx, &cmd_ptr,&buf,NULL)) {
                normalize_name(buf);
                if (*buf == CLI_DIRSEP_CHAR) {
                        mask = talloc_strdup(ctx, buf);
                } else {
                        mask = talloc_asprintf_append(mask, buf);
                }
     ...
        return rc;
}

These are almost identical to those discussed earlier. And of course, they were patched like this.

 	if ((mask[0] != '') && (mask[strlen(mask)-1]!=CLI_DIRSEP_CHAR)) {
-		mask = talloc_asprintf_append(mask, CLI_DIRSEP_STR);
+		mask = talloc_asprintf_append(mask, "%s", CLI_DIRSEP_STR);
 		if (!mask) {
    ...
 		} else {
-			mask = talloc_asprintf_append(mask, buf);
+			mask = talloc_asprintf_append(mask, "%s", buf);
 		}

The next one is probably the most simple one to trigger. It can be found at the function responsible for retrieving a file.

/****************************************************************************
 Get a file.
****************************************************************************/

static int cmd_get(void)
{
        TALLOC_CTX *ctx = talloc_tos();
        char *lname = NULL;
        char *rname = NULL;
        char *fname = NULL;
     ...
        if (!next_token_talloc(ctx, &cmd_ptr,&fname,NULL)) {
                d_printf("get <filename> [localname]\n");
                return 1;
        }
        rname = talloc_asprintf_append(rname, fname);
        if (!rname) {
                return 1;
        }
     ...
        return do_get(rname, lname, false);
}

This one was fixed by applying the following patch.

 	}
-	rname = talloc_asprintf_append(rname, fname);
+	rname = talloc_asprintf_append(rname, "%s", fname);
 	if (!rname) {

The next one…

/****************************************************************************
 View the file using the pager.
****************************************************************************/

static int cmd_more(void)
{
        TALLOC_CTX *ctx = talloc_tos();
        char *rname = NULL;
        char *fname = NULL;
        char *lname = NULL;
      ...
        if (!next_token_talloc(ctx, &cmd_ptr,&fname,NULL)) {
                d_printf("more <filename>\n");
                unlink(lname);
                return 1;
        }
        rname = talloc_asprintf_append(rname, fname);
        if (!rname) {
                return 1;
        }
      ...
        return rc;
}

And its patch…

 	}
-	rname = talloc_asprintf_append(rname, fname);
+	rname = talloc_asprintf_append(rname, "%s", fname);
 	if (!rname) {

The same code was also present in mget operation like this.

/****************************************************************************
 Do a mget command.
****************************************************************************/

static int cmd_mget(void)
{
        TALLOC_CTX *ctx = talloc_tos();
        uint16 attribute = aSYSTEM | aHIDDEN;
        char *mget_mask = NULL;
        char *buf = NULL;
     ...
                } else {
                        mget_mask = talloc_asprintf_append(mget_mask,
                                                        buf);
     ...
        return 0;
}

And it was patche like this.

 			mget_mask = talloc_asprintf_append(mget_mask,
-							buf);
+							"%s", buf);
 		}

On mkdir as well…

/****************************************************************************
 Make a directory.
****************************************************************************/

static int cmd_mkdir(void)
{
        TALLOC_CTX *ctx = talloc_tos();
        char *mask = NULL;
        char *buf = NULL;
      ...
        if (!next_token_talloc(ctx, &cmd_ptr,&buf,NULL)) {
                if (!recurse) {
                        d_printf("mkdir <dirname>\n");
                }
                return 1;
        }
        mask = talloc_asprintf_append(mask, buf);
        if (!mask) {
                return 1;
        }
     ...
                p = strtok_r(ddir, "/\\", &saveptr);
                while (p) {
                        ddir2 = talloc_asprintf_append(ddir2, p);
                        if (!ddir2) {
                                return 1;
                        }
                        if (!cli_chkpath(targetcli, ddir2)) {
                                do_mkdir(ddir2);
                        }
                        ddir2 = talloc_asprintf_append(ddir2, CLI_DIRSEP_STR);
                        if (!ddir2) {
                                return 1;
                        }
     ...
        return 0;
}

Which includes three instances of the same bug. Those were patched like that.

 	}
-	mask = talloc_asprintf_append(mask, buf);
+	mask = talloc_asprintf_append(mask, "%s", buf);
 	if (!mask) {
   ...
 		while (p) {
-			ddir2 = talloc_asprintf_append(ddir2, p);
+			ddir2 = talloc_asprintf_append(ddir2, "%s", p);
 			if (!ddir2) {
 				return 1;
 			}
 			if (!cli_chkpath(targetcli, ddir2)) {
 				do_mkdir(ddir2);
 			}
-			ddir2 = talloc_asprintf_append(ddir2, CLI_DIRSEP_STR);
+			ddir2 = talloc_asprintf_append(ddir2, "%s", CLI_DIRSEP_STR);
 			if (!ddir2) {

Next one…

/****************************************************************************
 Show alt name.
****************************************************************************/

static int cmd_altname(void)
{
        TALLOC_CTX *ctx = talloc_tos();
        char *name;
        char *buf;
       ...
        if (!next_token_talloc(ctx, &cmd_ptr, &buf, NULL)) {
                d_printf("altname <file>\n");
                return 1;
        }
        name = talloc_asprintf_append(name, buf);
        if (!name) {
                return 1;
        }
        do_altname(name);
        return 0;
}

And its patch…

 	}
-	name = talloc_asprintf_append(name, buf);
+	name = talloc_asprintf_append(name, "%s", buf);
 	if (!name) {

When requesting all info…

/****************************************************************************
 Show all info we can get
****************************************************************************/

static int cmd_allinfo(void)
{
        TALLOC_CTX *ctx = talloc_tos();
        char *name;
        char *buf;
       ...
        if (!next_token_talloc(ctx, &cmd_ptr, &buf, NULL)) {
                d_printf("allinfo <file>\n");
                return 1;
        }
        name = talloc_asprintf_append(name, buf);
        if (!name) {
                return 1;
        }

        do_allinfo(name);

        return 0;
}

And its patch…

 	}
-	name = talloc_asprintf_append(name, buf);
+	name = talloc_asprintf_append(name, "%s", buf);
 	if (!name) {

There a couple more…

/****************************************************************************
 Put a file.
****************************************************************************/

static int cmd_put(void)
{
     ...
        if (next_token_talloc(ctx, &cmd_ptr,&buf,NULL)) {
                rname = talloc_asprintf_append(rname, buf);
        } else {
                rname = talloc_asprintf_append(rname, lname);
        }
     ...
        return do_put(rname, lname, false);
}

And its patch…

 	if (next_token_talloc(ctx, &cmd_ptr,&buf,NULL)) {
-		rname = talloc_asprintf_append(rname, buf);
+		rname = talloc_asprintf_append(rname, "%s", buf);
 	} else {
-		rname = talloc_asprintf_append(rname, lname);
+		rname = talloc_asprintf_append(rname, "%s", lname);
 	}

And…

/****************************************************************************
 Delete some files.
****************************************************************************/

static int cmd_del(void)
{
     ...
        if (!next_token_talloc(ctx, &cmd_ptr,&buf,NULL)) {
                d_printf("del <filename>\n");
                return 1;
        }
        mask = talloc_asprintf_append(mask, buf);
     ...
}

Which was patched like that…

 	}
-	mask = talloc_asprintf_append(mask, buf);
+	mask = talloc_asprintf_append(mask, "%s", buf);
 	if (!mask) {

When attempting to re-get a file…

/****************************************************************************
 Get a file restarting at end of local file.
 ****************************************************************************/

static int cmd_reget(void)
{
        ...
        if (!next_token_talloc(ctx, &cmd_ptr, &fname, NULL)) {
                d_printf("reget <filename>\n");
                return 1;
        }
        remote_name = talloc_asprintf_append(remote_name, fname);
        if (!remote_name) {
                return 1;
        ...
        return do_get(remote_name, local_name, true);
}

And its patch

 	}
-	remote_name = talloc_asprintf_append(remote_name, fname);
+	remote_name = talloc_asprintf_append(remote_name, "%s", fname);
 	if (!remote_name) {

Consequently, for the equivalent put operation…

/****************************************************************************
 Put a file restarting at end of local file.
 ****************************************************************************/

static int cmd_reput(void)
{
     ...
        if (next_token_talloc(ctx, &cmd_ptr, &buf, NULL)) {
                remote_name = talloc_asprintf_append(remote_name,
                                                buf);
        } else {
                remote_name = talloc_asprintf_append(remote_name,
                                                local_name);
        }
     ...
        return do_put(remote_name, local_name, true);
}

And finally…

static void completion_remote_filter(const char *mnt,
                                file_info *f,
                                const char *mask,
                                void *state)
{
     ...
                        tmp = talloc_strdup(ctx,info->dirmask);
                        if (!tmp) {
                                TALLOC_FREE(ctx);
                                return;
                        }
                        tmp = talloc_asprintf_append(tmp, f->name);
                        if (!tmp) {
                                TALLOC_FREE(ctx);
                                return;
                        }
                        if (f->mode & aDIR) {
                                tmp = talloc_asprintf_append(tmp, CLI_DIRSEP_STR);
                        }
     ...
}

Which was patched…

 			}
-			tmp = talloc_asprintf_append(tmp, f->name);
+			tmp = talloc_asprintf_append(tmp, "%s", f->name);
 			if (!tmp) {
 				TALLOC_FREE(ctx);
 				return;
 			}
 			if (f->mode & aDIR) {
-				tmp = talloc_asprintf_append(tmp, CLI_DIRSEP_STR);
+				tmp = talloc_asprintf_append(tmp, "%s", CLI_DIRSEP_STR);
 			}

Written by xorl

June 26, 2009 at 00:32

Posted in bugs

2 Responses

Subscribe to comments with RSS.

  1. hello
    are you tell me how to implemente this vulnerability?
    no need to details!
    whit best regards

    mahdi

    August 6, 2009 at 14:15

  2. You probably mean an exploit for that one. Just read papers on how to exploit format string vulnerabilities. To trigger it and then attempt to develop an exploit code you can use something similar to:

    put aa%3Fbb

    As the advisory[1] says :P
    Of course, you will replace this with your format string. If you want any specific details you can contact me via email.

    [1] http://www.samba.org/samba/security/CVE-2009-1886.html

    xorl

    August 6, 2009 at 14:24


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s