diff mbox

[FFmpeg-devel] ffserver: local OOB write with custom program name

Message ID 20170106223315.GA20693@localhost
State Accepted
Commit 95d9a85ca3e662388d5fa7ef1937d1c3fbe2dcd5
Headers show

Commit Message

Tobias Stoeckmann Jan. 6, 2017, 10:33 p.m. UTC 
When the command line for children is created, it is assumed that
my_program_name always ends with "ffserver", which doesn't have to
be true if ffserver is called through a symbolic link.

In such a case, it could be that not enough space for "ffmpeg" is
available at the end, leading to a buffer overflow.

One example would be:

$ ln -s /usr/bin/ffserver ~/f; ~/f

As this is only a local buffer overflow, i.e. is based on a weird
program call, this has NO security impact.
---
 ffserver.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Comments

Michael Niedermayer Jan. 8, 2017, 2:43 a.m. UTC | #1 
On Fri, Jan 06, 2017 at 11:33:16PM +0100, Tobias Stoeckmann wrote:
> When the command line for children is created, it is assumed that
> my_program_name always ends with "ffserver", which doesn't have to
> be true if ffserver is called through a symbolic link.
> 
> In such a case, it could be that not enough space for "ffmpeg" is
> available at the end, leading to a buffer overflow.
> 
> One example would be:
> 
> $ ln -s /usr/bin/ffserver ~/f; ~/f
> 
> As this is only a local buffer overflow, i.e. is based on a weird
> program call, this has NO security impact.
> ---
>  ffserver.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)

applied

thx

[...]
compn Jan. 8, 2017, 5:09 p.m. UTC | #2 
On Fri, 6 Jan 2017 23:33:16 +0100
Tobias Stoeckmann <tobias@stoeckmann.org> wrote:

> +    slash = strrchr(my_program_name, '/');
> +            memcpy(pathname, my_program_name, slash - my_program_name);

> -    strcpy(slash, "ffmpeg");
> +    strcat(pathname, "ffmpeg");

this replaces a strcpy with a memcpy, is this intended (and safe?)?

(possibly this is a dumb question, if so, please ignore this mail.)

-compn
diff mbox

Patch

diff --git a/ffserver.c b/ffserver.c
index 02a583464b..8b819b6934 100644
--- a/ffserver.c
+++ b/ffserver.c
@@ -495,20 +495,22 @@  static void start_children(FFServerStream *feed)
         return;
     }
 
-    pathname = av_strdup (my_program_name);
+    slash = strrchr(my_program_name, '/');
+    if (!slash) {
+        pathname = av_mallocz(sizeof("ffmpeg"));
+    } else {
+        pathname = av_mallocz(slash - my_program_name + sizeof("ffmpeg"));
+        if (pathname != NULL) {
+            memcpy(pathname, my_program_name, slash - my_program_name);
+        }
+    }
     if (!pathname) {
         http_log("Could not allocate memory for children cmd line\n");
         return;
     }
-   /* replace "ffserver" with "ffmpeg" in the path of current
-    * program. Ignore user provided path */
+   /* use "ffmpeg" in the path of current program. Ignore user provided path */
 
-    slash = strrchr(pathname, '/');
-    if (!slash)
-        slash = pathname;
-    else
-        slash++;
-    strcpy(slash, "ffmpeg");
+    strcat(pathname, "ffmpeg");
 
     for (; feed; feed = feed->next) {