harden against corrupt symlinks

harden against corrupt symlinks

[Mike Waychison]

[-- Attachment #1: Type: text/plain, Size: 409 bytes --]

Hi Al,

We've hit the situation a few times where a corrupt symlink could easily 
oops the kernel.  The problem was tracked down to an older e2fsutils 
that didn't do much sanity checking on symlinks during a fsck.  This 
patch uses strnlen when reading in the symlink and ensures that it 
doesn't exceed PATH_MAX.

Would you accept this kind of 'hardening'?

Signed-off-by: Mike Waychison <mikew@google.com>

[-- Attachment #2: symlink_run_off.patch --]
[-- Type: text/plain, Size: 707 bytes --]

--- linux-2.6/fs/namei.c	2005-05-24 11:13:09.000000000 -0700
+++ linux-2.6/fs/namei.c	2005-05-24 11:13:12.000000000 -0700
@@ -1936,7 +1936,12 @@ int vfs_readlink(struct dentry *dentry, 
 	if (IS_ERR(link))
 		goto out;
 
-	len = strlen(link);
+	len = strnlen(link, PATH_MAX);
+	if (len == PATH_MAX) {
+		len = -ENAMETOOLONG;
+		goto out;
+	}
+
 	if (len > (unsigned) buflen)
 		len = buflen;
 	if (copy_to_user(buffer, link, len))
@@ -1953,6 +1958,11 @@ __vfs_follow_link(struct nameidata *nd, 
 	if (IS_ERR(link))
 		goto fail;
 
+	if (strnlen(link, PATH_MAX) == PATH_MAX) {
+		link = ERR_PTR(-ENAMETOOLONG);
+		goto fail;
+	}
+
 	if (*link == '/') {
 		path_release(nd);
 		if (!walk_init_root(link, nd))

Re: harden against corrupt symlinks

[Andreas Dilger]

On May 31, 2005  16:02 -0700, Mike Waychison wrote:
> We've hit the situation a few times where a corrupt symlink could easily 
> oops the kernel.  The problem was tracked down to an older e2fsutils 
> that didn't do much sanity checking on symlinks during a fsck.  This 
> patch uses strnlen when reading in the symlink and ensures that it 
> doesn't exceed PATH_MAX.
> 
> Would you accept this kind of 'hardening'?
> 
> Signed-off-by: Mike Waychison <mikew@google.com>

Andrew Morton suggested a very similar fix 3 years ago to this list:

Subject: Re: ext3 -> crash -> fsck -> readlink -> oops

I thought it made it into the kernel then, but I guess not.

> --- linux-2.6/fs/namei.c	2005-05-24 11:13:09.000000000 -0700
> +++ linux-2.6/fs/namei.c	2005-05-24 11:13:12.000000000 -0700
> @@ -1936,7 +1936,12 @@ int vfs_readlink(struct dentry *dentry, 
>  	if (IS_ERR(link))
>  		goto out;
>  
> -	len = strlen(link);
> +	len = strnlen(link, PATH_MAX);
> +	if (len == PATH_MAX) {
> +		len = -ENAMETOOLONG;
> +		goto out;
> +	}
> +
>  	if (len > (unsigned) buflen)
>  		len = buflen;
>  	if (copy_to_user(buffer, link, len))
> @@ -1953,6 +1958,11 @@ __vfs_follow_link(struct nameidata *nd, 
>  	if (IS_ERR(link))
>  		goto fail;
>  
> +	if (strnlen(link, PATH_MAX) == PATH_MAX) {
> +		link = ERR_PTR(-ENAMETOOLONG);
> +		goto fail;
> +	}
> +
>  	if (*link == '/') {
>  		path_release(nd);
>  		if (!walk_init_root(link, nd))


Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: harden against corrupt symlinks

[Andrew Morton]

On Wed, 1 Jun 2005 02:15:48 -0600
Andreas Dilger <adilger@clusterfs.com> wrote:

> On May 31, 2005  16:02 -0700, Mike Waychison wrote:
> > We've hit the situation a few times where a corrupt symlink could easily 
> > oops the kernel.  The problem was tracked down to an older e2fsutils 
> > that didn't do much sanity checking on symlinks during a fsck.  This 
> > patch uses strnlen when reading in the symlink and ensures that it 
> > doesn't exceed PATH_MAX.
> > 
> > Would you accept this kind of 'hardening'?
> > 
> > Signed-off-by: Mike Waychison <mikew@google.com>
> 
> Andrew Morton suggested a very similar fix 3 years ago to this list:
> 
> Subject: Re: ext3 -> crash -> fsck -> readlink -> oops
> 
> I thought it made it into the kernel then, but I guess not.

This fix has drifted across my google desk - I thought we'd fixed it, but
still not.  Third time lucky, perhaps.

> > --- linux-2.6/fs/namei.c	2005-05-24 11:13:09.000000000 -0700
> > +++ linux-2.6/fs/namei.c	2005-05-24 11:13:12.000000000 -0700
> > @@ -1936,7 +1936,12 @@ int vfs_readlink(struct dentry *dentry, 
> >  	if (IS_ERR(link))
> >  		goto out;
> >  
> > -	len = strlen(link);
> > +	len = strnlen(link, PATH_MAX);
> > +	if (len == PATH_MAX) {
> > +		len = -ENAMETOOLONG;
> > +		goto out;
> > +	}
> > +
> >  	if (len > (unsigned) buflen)
> >  		len = buflen;
> >  	if (copy_to_user(buffer, link, len))
> > @@ -1953,6 +1958,11 @@ __vfs_follow_link(struct nameidata *nd, 
> >  	if (IS_ERR(link))
> >  		goto fail;
> >  
> > +	if (strnlen(link, PATH_MAX) == PATH_MAX) {
> > +		link = ERR_PTR(-ENAMETOOLONG);
> > +		goto fail;
> > +	}
> > +
> >  	if (*link == '/') {
> >  		path_release(nd);
> >  		if (!walk_init_root(link, nd))
> 

A few things...

Should we instead be treating this as a filesystem driver bug, and fix up
the filesystem(s) to not return overly-long pathname components?

Running strnlen() against each component in __vfs_follow_link() looks
expensive.  Can it be avoided?

Given that this is the VFS correcting for filesystem misbehaviour, it would
seem to make sense to perform this correction at a low level, immediately
after the filesytem driver has returned us the pathname component.  An
apparently-obvious way of doing this is to wrap ->follow_link.  Can anyone
think of a smarter way?

Re: harden against corrupt symlinks

[Andreas Dilger]

On Aug 11, 2006  15:48 -0700, Andrew Morton wrote:
> > > --- linux-2.6/fs/namei.c	2005-05-24 11:13:09.000000000 -0700
> > > +++ linux-2.6/fs/namei.c	2005-05-24 11:13:12.000000000 -0700
> > > @@ -1936,7 +1936,12 @@ int vfs_readlink(struct dentry *dentry, 
> > >  	if (IS_ERR(link))
> > >  		goto out;
> > >  
> > > -	len = strlen(link);
> > > +	len = strnlen(link, PATH_MAX);
> > > +	if (len == PATH_MAX) {
> > > +		len = -ENAMETOOLONG;
> > > +		goto out;
> > > +	}
> > > +
> > >  	if (len > (unsigned) buflen)
> > >  		len = buflen;
> > >  	if (copy_to_user(buffer, link, len))
> > > @@ -1953,6 +1958,11 @@ __vfs_follow_link(struct nameidata *nd, 
> > >  	if (IS_ERR(link))
> > >  		goto fail;
> > >  
> > > +	if (strnlen(link, PATH_MAX) == PATH_MAX) {
> > > +		link = ERR_PTR(-ENAMETOOLONG);
> > > +		goto fail;
> > > +	}
> > > +
> > >  	if (*link == '/') {
> > >  		path_release(nd);
> > >  		if (!walk_init_root(link, nd))
> > 
> 
> Should we instead be treating this as a filesystem driver bug, and fix up
> the filesystem(s) to not return overly-long pathname components?
> 
> Running strnlen() against each component in __vfs_follow_link() looks
> expensive.  Can it be avoided?
> 
> Given that this is the VFS correcting for filesystem misbehaviour, it would
> seem to make sense to perform this correction at a low level, immediately
> after the filesytem driver has returned us the pathname component.  An
> apparently-obvious way of doing this is to wrap ->follow_link.  Can anyone
> think of a smarter way?

#define PATH_MAX        4096    /* # chars in a path name including nul */

One possibility is for the case of long symlinks to always set the last byte
of the page to NUL to ensure that the link is terminated.  This appears
easily doable by having page_getlink() do the NUL termination after kmap()
but before returning, something like:

 static char *page_getlink(struct dentry * dentry, struct page **ppage)
 {
	char *link;
 	page = read_cache_page(mapping, 0, (filler_t *)mapping->a_ops->readpage,
 			       NULL);
 	if (IS_ERR(page))
 		goto sync_fail;
 	wait_on_page_locked(page);
 	if (!PageUptodate(page))
 		goto async_fail;
 	*ppage = page;
-	return kmap(page);
+	link = kmap(page);
+	/* PATH_MAX is strictly <= PAGE_SIZE */
+	link[PATH_MAX - 1] = '\0';
+	return link;


Many of the other filesystems that don't use page_follow_link_light()
already do NUL termination themselves.

nfs_follow_link() is very similar, but not identical and needs the same fix.

ocfs2_page_getlink() is an exact duplicate of page_getlink() and the bug
duplication could be avoided if page_getlink() was exported.  Otherwise it
needs the same fix.


I _think_ PATH_MAX is the right thing here (instead of PAGE_SIZE), since
the caller expects at most PATH_MAX in the returned link, and PAGE_SIZE
may be considerably larger.  I don't think PATH_MAX will ever be larger
than PAGE_SIZE.  We could also use min(PAGE_SIZE, PATH_MAX) and it would
be resolved at compile time, but it seems wishy-washy to me.

The other option is to actually get the link length out of the filesystem
itself, and avoid strlen(link) entirely, but that is a more complex change.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: harden against corrupt symlinks

[Dave Kleikamp]

On Fri, 2006-08-18 at 00:27 -0600, Andreas Dilger wrote:
> On Aug 11, 2006  15:48 -0700, Andrew Morton wrote:
> > 
> > Given that this is the VFS correcting for filesystem misbehaviour, it would
> > seem to make sense to perform this correction at a low level, immediately
> > after the filesytem driver has returned us the pathname component.  An
> > apparently-obvious way of doing this is to wrap ->follow_link.  Can anyone
> > think of a smarter way?
> 
> #define PATH_MAX        4096    /* # chars in a path name including nul */
> 
> One possibility is for the case of long symlinks to always set the last byte
> of the page to NUL to ensure that the link is terminated.  This appears
> easily doable by having page_getlink() do the NUL termination after kmap()
> but before returning, something like:
> 
>  static char *page_getlink(struct dentry * dentry, struct page **ppage)
>  {
> 	char *link;
>  	page = read_cache_page(mapping, 0, (filler_t *)mapping->a_ops->readpage,
>  			       NULL);
>  	if (IS_ERR(page))
>  		goto sync_fail;
>  	wait_on_page_locked(page);
>  	if (!PageUptodate(page))
>  		goto async_fail;
>  	*ppage = page;
> -	return kmap(page);
> +	link = kmap(page);
> +	/* PATH_MAX is strictly <= PAGE_SIZE */
> +	link[PATH_MAX - 1] = '\0';
> +	return link;

This seems reasonable.
> 
> 
> Many of the other filesystems that don't use page_follow_link_light()
> already do NUL termination themselves.
> 
> nfs_follow_link() is very similar, but not identical and needs the same fix.

jfs_follow_link() too.  :-)

> ocfs2_page_getlink() is an exact duplicate of page_getlink() and the bug
> duplication could be avoided if page_getlink() was exported.  Otherwise it
> needs the same fix.
> 
> 
> I _think_ PATH_MAX is the right thing here (instead of PAGE_SIZE), since
> the caller expects at most PATH_MAX in the returned link, and PAGE_SIZE
> may be considerably larger.  I don't think PATH_MAX will ever be larger
> than PAGE_SIZE.  We could also use min(PAGE_SIZE, PATH_MAX) and it would
> be resolved at compile time, but it seems wishy-washy to me.
> 
> The other option is to actually get the link length out of the filesystem
> itself, and avoid strlen(link) entirely, but that is a more complex change.

Is it?  Is there any file system where the link length is not i_size?

-- 
David Kleikamp
IBM Linux Technology Center