Re: harden against corrupt symlinks

[Dave Kleikamp <shaggy@austin.ibm.com>]

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