[PATCH] listxattr: Fix read past end of array

[PATCH] listxattr: Fix read past end of array

[Nikolay Borisov]

listxattr reaturns a null-terminated list of entries that represent
the xattr names. However, if it is passed larger buffer than it
requires it won't zero-out the rest of the memory. The way the
loop iterator in listxattr.c is written makes it go print every
null-terminated entry up to bufsize (which is user passed parameter).
This can lead to a situation where listxattr users N bytes out of
M bytes big buffer ( M > N). This will leave the rest (M-N)
as garbage, which in turn will be printed by listxattr. Fix this
by modifying the terminating condition of the loop to print
up to the return value of listxattr which in case of success is the
amount of bytes used in the passed buffer.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 src/listxattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/listxattr.c b/src/listxattr.c
index cd46637a..f07c23f8 100644
--- a/src/listxattr.c
+++ b/src/listxattr.c
@@ -61,7 +61,7 @@ int main(int argc, char **argv)
                 perror("listxattr");
         } else {
                 char *l;
-                for (l = buf; l != (buf + bufsize) && *l != '\0';
+                for (l = buf; l != (buf + ret) && *l != '\0';
                                 l = strchr(l, '\0') + 1) {
                         printf("xattr: %s\n", l);
                 }
-- 
2.12.3

[PATCHv2] src/listxattr: Fix reading past the end of the user buffer

[Nikolay Borisov]

listxattr reaturns a null-terminated list of entries that represent
the xattr names. However, if it is passed larger buffer than it
requires it won't zero-out the rest of the memory. The way the
loop iterator in listxattr.c is written makes it go print every
null-terminated entry up to bufsize (which is user passed parameter).
This can lead to a situation where listxattr users N bytes out of
M bytes big buffer ( M > N). This will leave the rest (M-N)
as garbage, which in turn will be printed by listxattr. Fix this
by converting the 'for' loop to 'while' and properly ensuring
we are reading at most howevermany elements the syscall reported
it returned

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

v2: 
 Rewrite the loop, hopefully making the code a bit more legible.
 Functionally it's the same as my previous fix

 src/listxattr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/listxattr.c b/src/listxattr.c
index cd46637a..584ebb2d 100644
--- a/src/listxattr.c
+++ b/src/listxattr.c
@@ -60,10 +60,10 @@ int main(int argc, char **argv)
         if (ret < 0) {
                 perror("listxattr");
         } else {
-                char *l;
-                for (l = buf; l != (buf + bufsize) && *l != '\0';
-                                l = strchr(l, '\0') + 1) {
+                char *l = buf;
+                while (l != (buf + ret)) {
                         printf("xattr: %s\n", l);
+                        l = strchr(l, '\0') + 1;
                 }
         }
 
-- 
2.12.3

Re: [PATCHv2] src/listxattr: Fix reading past the end of the user buffer

[Nikolay Borisov]

On 30.05.2017 17:10, Nikolay Borisov wrote:
> listxattr reaturns a null-terminated list of entries that represent
> the xattr names. However, if it is passed larger buffer than it
> requires it won't zero-out the rest of the memory. The way the
> loop iterator in listxattr.c is written makes it go print every
> null-terminated entry up to bufsize (which is user passed parameter).
> This can lead to a situation where listxattr users N bytes out of
> M bytes big buffer ( M > N). This will leave the rest (M-N)
> as garbage, which in turn will be printed by listxattr. Fix this
> by converting the 'for' loop to 'while' and properly ensuring
> we are reading at most howevermany elements the syscall reported
> it returned
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> v2: 
>  Rewrite the loop, hopefully making the code a bit more legible.
>  Functionally it's the same as my previous fix
> 
>  src/listxattr.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/listxattr.c b/src/listxattr.c
> index cd46637a..584ebb2d 100644
> --- a/src/listxattr.c
> +++ b/src/listxattr.c
> @@ -60,10 +60,10 @@ int main(int argc, char **argv)
>          if (ret < 0) {
>                  perror("listxattr");
>          } else {
> -                char *l;
> -                for (l = buf; l != (buf + bufsize) && *l != '\0';
> -                                l = strchr(l, '\0') + 1) {
> +                char *l = buf;
> +                while (l != (buf + ret)) {

Actually I believe while (l < (buf + ret)) is the correct form.

>                          printf("xattr: %s\n", l);
> +                        l = strchr(l, '\0') + 1;
>                  }
>          }
>  
> 

Re: [PATCHv2] src/listxattr: Fix reading past the end of the user buffer

[Eryu Guan]

On Wed, May 31, 2017 at 11:58:12AM +0300, Nikolay Borisov wrote:
> 
> 
> On 30.05.2017 17:10, Nikolay Borisov wrote:
> > listxattr reaturns a null-terminated list of entries that represent
> > the xattr names. However, if it is passed larger buffer than it
> > requires it won't zero-out the rest of the memory. The way the
> > loop iterator in listxattr.c is written makes it go print every
> > null-terminated entry up to bufsize (which is user passed parameter).
> > This can lead to a situation where listxattr users N bytes out of
> > M bytes big buffer ( M > N). This will leave the rest (M-N)
> > as garbage, which in turn will be printed by listxattr. Fix this
> > by converting the 'for' loop to 'while' and properly ensuring
> > we are reading at most howevermany elements the syscall reported
> > it returned
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> > 
> > v2: 
> >  Rewrite the loop, hopefully making the code a bit more legible.
> >  Functionally it's the same as my previous fix
> > 
> >  src/listxattr.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/listxattr.c b/src/listxattr.c
> > index cd46637a..584ebb2d 100644
> > --- a/src/listxattr.c
> > +++ b/src/listxattr.c
> > @@ -60,10 +60,10 @@ int main(int argc, char **argv)
> >          if (ret < 0) {
> >                  perror("listxattr");
> >          } else {
> > -                char *l;
> > -                for (l = buf; l != (buf + bufsize) && *l != '\0';
> > -                                l = strchr(l, '\0') + 1) {
> > +                char *l = buf;
> > +                while (l != (buf + ret)) {
> 
> Actually I believe while (l < (buf + ret)) is the correct form.

Fixed while committing, thanks for the update!

Eryu

> 
> >                          printf("xattr: %s\n", l);
> > +                        l = strchr(l, '\0') + 1;
> >                  }
> >          }
> >  
> >