[app/xinit] Buffer overflow with many arguments.

Submitted by Tobias Stoeckmann on Jan. 18, 2019, 8:59 p.m.

Details

Message ID 20190118205941.zqbrnaofwy27yixu@localhost
State New
Series "Buffer overflow with many arguments."
Headers show

Commit Message

Tobias Stoeckmann Jan. 18, 2019, 8:59 p.m.
Command line arguments are copied into clientargv and serverargv without
verifying that enough space is available. A high amount of arguments can
therefore trigger a buffer overflow like this:

$ xinit $(seq 1 500)

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
---
 xinit.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/xinit.c b/xinit.c
index f826b7a..8efd0be 100644
--- a/xinit.c
+++ b/xinit.c
@@ -151,7 +151,6 @@  main(int argc, char *argv[])
     register char **ptr;
     pid_t pid;
     int client_given = 0, server_given = 0;
-    int client_args_given = 0, server_args_given = 0;
     int start_of_client_args, start_of_server_args;
     struct sigaction sa, si;
 #ifdef __APPLE__
@@ -174,7 +173,8 @@  main(int argc, char *argv[])
     }
     start_of_client_args = (cptr - client);
     while (argc && strcmp(*argv, "--")) {
-        client_args_given++;
+        if (cptr > clientargv + 98)
+            Fatalx("too many client arguments");
         *cptr++ = *argv++;
         argc--;
     }
@@ -202,7 +202,8 @@  main(int argc, char *argv[])
 
     start_of_server_args = (sptr - server);
     while (--argc >= 0) {
-        server_args_given++;
+        if (sptr > serverargv + 98)
+            Fatalx("too many server arguments");
         *sptr++ = *argv++;
     }
     *sptr = NULL;

Comments

Walter Harms Jan. 19, 2019, 4:54 p.m.
> Tobias Stoeckmann <tobias@stoeckmann.org> hat am 18. Januar 2019 um 21:59
> geschrieben:
> 
> 
> Command line arguments are copied into clientargv and serverargv without
> verifying that enough space is available. A high amount of arguments can
> therefore trigger a buffer overflow like this:
> 
> $ xinit $(seq 1 500)
> 
> Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
> ---
>  xinit.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xinit.c b/xinit.c
> index f826b7a..8efd0be 100644
> --- a/xinit.c
> +++ b/xinit.c
> @@ -151,7 +151,6 @@ main(int argc, char *argv[])
>      register char **ptr;
>      pid_t pid;
>      int client_given = 0, server_given = 0;
> -    int client_args_given = 0, server_args_given = 0;
>      int start_of_client_args, start_of_server_args;
>      struct sigaction sa, si;
>  #ifdef __APPLE__
> @@ -174,7 +173,8 @@ main(int argc, char *argv[])
>      }
>      start_of_client_args = (cptr - client);
>      while (argc && strcmp(*argv, "--")) {
> -        client_args_given++;
> +        if (cptr > clientargv + 98)
> +            Fatalx("too many client arguments");
>          *cptr++ = *argv++;
>          argc--;
>      }
> @@ -202,7 +202,8 @@ main(int argc, char *argv[])
>  
>      start_of_server_args = (sptr - server);
>      while (--argc >= 0) {
> -        server_args_given++;
> +        if (sptr > serverargv + 98)
> +            Fatalx("too many server arguments");
>          *sptr++ = *argv++;
>      }
>      *sptr = NULL;
> -- 

hi,
nice catch.

instead of letting 98 magicly popup what is about
sizeof(serverargv)/sizeof(*serverargv) ?
Dito clientargv,

re,
 wh
Tobias Stoeckmann Jan. 19, 2019, 7:37 p.m.
> hi,
> nice catch.
> 
> instead of letting 98 magicly popup what is about
> sizeof(serverargv)/sizeof(*serverargv) ?
> Dito clientargv,
> 
> re,
>  wh

There is still a pseudo-magical - 2 missing there, to keep space for the
last NULL assignment.

But I'm fine with both. As long as 98 is the result. :)


Tobias