[01/14] xfs_scrub: avoid buffer overflow when scanning attributes
diff mbox

Message ID 152160358636.8288.17415854751640340360.stgit@magnolia
State Superseded
Headers show

Commit Message

Darrick J. Wong March 21, 2018, 3:39 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Avoid a buffer overflow when we're formatting extended attribute names
for name checking.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/phase5.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)



--
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

Comments

Eric Sandeen April 3, 2018, 5:30 p.m. UTC | #1
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)

>  
>  /*
>   * 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];

			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?

> +			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
> 
--
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
Darrick J. Wong April 5, 2018, 3:57 a.m. UTC | #2
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).

Right.

> > 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)

Sure, but it's in a public header file, we're stuck with the name
forever.

> 
> >  
> >  /*
> >   * 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];
> 
> 			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?

Probably not.  Will restructure this one and await the
"robofuzztestcallerhahahahahha." namespace. :)

--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
> > 
> --
> 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
--
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
Darrick J. Wong April 11, 2018, 12:20 a.m. UTC | #3
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
> > 
--
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

Patch
diff mbox

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)
 
 /*
  * 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,
+			snprintf(keybuf, ATTR_NAME_MAX, "%s.%s", attr_ns->name,
 					ent->a_name);
 			moveon = xfs_scrub_check_name(ctx, descr,
 					_("extended attribute"), keybuf);