xorl %eax, %eax

CVE-2009-4020: Linux kernel HFS Multiple Buffer Overflows

leave a comment »

I saw this bug from Eugene Teo’s email to the oss-security mailing list. This vulnerability was initially reported here and below is the buggy code as seen in 2.6.31 release of the Linux kernel…

/*
 * hfs_cat_move()
 *
 * Rename a file or directory, possibly to a new directory.
 * If the destination exists it is removed and a
 * (struct hfs_cat_entry) for it is returned in '*result'.
 */
int hfs_cat_move(u32 cnid, struct inode *src_dir, struct qstr *src_name,
                 struct inode *dst_dir, struct qstr *dst_name)
{
        struct super_block *sb;
        struct hfs_find_data src_fd, dst_fd;
        union hfs_cat_rec entry;
        int entry_size, type;
        int err;

        dprint(DBG_CAT_MOD, "rename_cat: %u - %lu,%s - %lu,%s\n", cnid, src_dir->i_ino, src_name->name,
                dst_dir->i_ino, dst_name->name);
        sb = src_dir->i_sb;
        hfs_find_init(HFS_SB(sb)->cat_tree, &src_fd);
        dst_fd = src_fd;

        /* find the old dir entry and read the data */
        hfs_cat_build_key(sb, src_fd.search_key, src_dir->i_ino, src_name);
        err = hfs_brec_find(&src_fd);
        if (err)
                goto out;

        hfs_bnode_read(src_fd.bnode, &entry, src_fd.entryoffset,
                            src_fd.entrylength);
    ...
        return err;
}

So, this code can be found at fs/hfs/catalog.c and its purpose is quite obvious from the comment on top of it. The last call to hfs_bnode_read() will lead to the following simple copy operation located in fs/hfs/bnode.c…

void hfs_bnode_read(struct hfs_bnode *node, void *buf,
                int off, int len)
{
        struct page *page;

        off += node->page_offset;
        page = node->page[0];

        memcpy(buf, kmap(page) + off, len);
        kunmap(page);
}

If we move back to hfs_cat_move() we’ll see that there are no bound checks on ‘entry’ buffer and the user controlled ‘src_fd’ members. This structure is defined in fs/hfs/btree.h like this:

struct hfs_find_data {
        btree_key *key;
        btree_key *search_key;
        struct hfs_btree *tree;
        struct hfs_bnode *bnode;
        int record;
        int keyoffset, keylength;
        int entryoffset, entrylength;
};

Consequently, a user could create an HFS image that would return an HFS entry with ‘entrylength’ greater than the size of the statically, stack allocated ‘entry’ variable which is a union defined at fs/hfs/hfs.h like this…

/* A catalog tree record */
typedef union hfs_cat_rec {
        s8 type;                        /* The type of entry */
        struct hfs_cat_file file;
        struct hfs_cat_dir dir;
        struct hfs_cat_thread thread;
} hfs_cat_rec;

Because of this missing check, a buffer overflow can occur during the memcpy() that could overwrite memory space beyond the bounds of ‘buf’ in hfs_bnode_read(). To fix this the following patch was applied:

 	if (err)
 		goto out;
+	if (src_fd.entrylength > sizeof(entry) || src_fd.entrylength < 0) {
+		err = -EIO;
+		goto out;
+	}
 
 	hfs_bnode_read(src_fd.bnode, &entry, src_fd.entryoffset,

Since ‘entrylength’ is a signed integer this new ‘if’ statement will check for both values that are greater than the size of the destination buffer and negative ones. The exact same vulnerability was also present in hfs_readdir() function from fs/hfs/dir.c as you can see below.

/*
 * hfs_readdir
 */
static int hfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
{
        struct inode *inode = filp->f_path.dentry->d_inode;
        struct super_block *sb = inode->i_sb;
        int len, err;
        char strbuf[HFS_MAX_NAMELEN];
    ...
        switch ((u32)filp->f_pos) {
    ...
        case 1:
                hfs_bnode_read(fd.bnode, &entry, fd.entryoffset, fd.entrylength);
                if (entry.type != HFS_CDR_THD) {
                        printk(KERN_ERR "hfs: bad catalog folder thread\n");
                        err = -EIO;
                        goto out;
                }
    ...
        for (;;) {
                if (be32_to_cpu(fd.key->cat.ParID) != inode->i_ino) {
    ...
                hfs_bnode_read(fd.bnode, &entry, fd.entryoffset, fd.entrylength);
    ...
out:
        hfs_find_exit(&fd);
        return err;
}

Which was patched in both cases using the same approach and at last, hfs_fill_super() from fs/hfs/super.c contained a similar bug that you can see below.

/*
 * hfs_read_super()
 *
 * This is the function that is responsible for mounting an HFS
 * filesystem.  It performs all the tasks necessary to get enough data
 * from the disk to read the root inode.  This includes parsing the
 * mount options, dealing with Macintosh partitions, reading the
 * superblock and the allocation bitmap blocks, calling
 * hfs_btree_init() to get the necessary data about the extents and
 * catalog B-trees and, finally, reading the root inode into memory.
 */
static int hfs_fill_super(struct super_block *sb, void *data, int silent)
{
        struct hfs_sb_info *sbi;
        struct hfs_find_data fd;
        hfs_cat_rec rec;
        struct inode *root_inode;
        int res;

        sbi = kzalloc(sizeof(struct hfs_sb_info), GFP_KERNEL);
     ...
        /* try to get the root inode */
        hfs_find_init(HFS_SB(sb)->cat_tree, &fd);
        res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd);
        if (!res)
                hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
     ...
bail:
        hfs_mdb_put(sb);
        return res;
}

That was patched once again using the same range checks on the ‘entrylength’:

 	hfs_find_init(HFS_SB(sb)->cat_tree, &fd);
 	res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd);
-	if (!res)
+	if (!res) {
+		if (fd.entrylength > sizeof(rec) || fd.entrylength < 0) {
+			res =  -EIO;
+			goto bail;
+		}
 		hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
+	}
 	if (res) {
 		hfs_find_exit(&fd);

Written by xorl

December 6, 2009 at 19:20

Posted in bugs, linux

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