Re: [PATCH 01/14] xfs_scrub: avoid buffer overflow when scanning attributes

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Apr 03, 2018 at 12:30:57PM -0500, Eric Sandeen wrote:
> On 3/20/18 10:39 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Avoid a buffer overflow when we're formatting extended attribute names
> > for name checking.
> 
> This won't /actually/ overflow, right, because you are doing
> snprintf(NAME_MAX) into a buffer of size [NAME_MAX + 1].
> 
> However, it might truncate the attribute string if too long.
> (just trying to avoid security folk wig-outs).
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  scrub/phase5.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/scrub/phase5.c b/scrub/phase5.c
> > index 8e0a1be..36821d0 100644
> > --- a/scrub/phase5.c
> > +++ b/scrub/phase5.c
> > @@ -143,6 +143,8 @@ static const struct xfs_attr_ns attr_ns[] = {
> >  	{ATTR_SECURE,		"secure"},
> >  	{0, NULL},
> >  };
> > +/* Enough space to handle the prefix. */
> > +#define ATTR_NAME_MAX		(NAME_MAX + 8)
> 
> Unrelated to this change, really, but should NAME_MAX be
> XATTR_NAME_MAX for clarity & consistency w/ the xattr code?
> 
> (it's defined in /usr/include/linux/limits.h)

Hm, I got bogged down here trying to figure out if XATTR_NAME_MAX
applied to the entire string "security.foo" or just the ".foo" part.
The answer is (according to getxattr()) the first, so I'll fix this to
use XATTR_NAME_MAX directly...

> 
> >  
> >  /*
> >   * Check all the xattr names in a particular namespace of a file handle
> > @@ -158,7 +160,7 @@ xfs_scrub_scan_fhandle_namespace_xattrs(
> >  {
> >  	struct attrlist_cursor		cur;
> >  	char				attrbuf[XFS_XATTR_LIST_MAX];
> > -	char				keybuf[NAME_MAX + 1];
> > +	char				keybuf[ATTR_NAME_MAX + 1];
> >  	struct attrlist			*attrlist = (struct attrlist *)attrbuf;
> >  	struct attrlist_ent		*ent;
> >  	struct unicrash			*uc;
> > @@ -172,14 +174,14 @@ xfs_scrub_scan_fhandle_namespace_xattrs(
> >  
> >  	memset(attrbuf, 0, XFS_XATTR_LIST_MAX);
> >  	memset(&cur, 0, sizeof(cur));
> > -	memset(keybuf, 0, NAME_MAX + 1);
> > +	memset(keybuf, 0, ATTR_NAME_MAX + 1);
> >  	error = attr_list_by_handle(handle, sizeof(*handle), attrbuf,
> >  			XFS_XATTR_LIST_MAX, attr_ns->flags, &cur);
> >  	while (!error) {
> >  		/* Examine the xattrs. */
> >  		for (i = 0; i < attrlist->al_count; i++) {
> >  			ent = ATTR_ENTRY(attrlist, i);
> > -			snprintf(keybuf, NAME_MAX, "%s.%s", attr_ns->name,
> 
> To future proof this rather than relying on a hardcoded 8 based on the
> current namespaces above, how about:
> 
> #define XATTR_NS_MAX 8
> #define ATTR_STRING_MAX		(XATTR_NS_MAX + XATTR_NAME_MAX + 1) /* for '.' */
> 
> char				keybuf[ATTR_STRING_MAX + 1];

...and then we can use it directly here instead of all these weird
NAME_MAX variants.

> 
> 			ASSERT(strlen(attr_ns->name) <= XATTR_NS_MAX);
> 			snprintf(keybuf, ATTR_STRING_MAX, "%s.%s", attr_ns->name,
> 					ent->a_name);
> 
> just in case someone adds a new 
> 
> 	ATTR_SUPERDELUXE, "superdeluxe"
> 
> namespace some day?  Is that overkill?

No, not overkill.

--D

> 
> > +			snprintf(keybuf, ATTR_NAME_MAX, "%s.%s", attr_ns->name,
> >  					ent->a_name);
> >  			moveon = xfs_scrub_check_name(ctx, descr,
> >  					_("extended attribute"), keybuf);
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >