[vlc-devel] code security audit: you can help

[Sam Hocevar]

   Given our massive userbase on several platforms and the growing
interest in our browser plugin, maybe it's time to take security in
VLC more seriously than before. This can be done in several independant
steps by different people, even with little programming skills or VLC
knowledge.

   There was a recent MPlayer security advisory [1] on bugtraq. It looks
more like an MPlayer plug than a real advisory to me, but it shows that
these issues are taken seriously by other people.


 1. What should be done
------------------------

   Well, our code should be audited. That seems obvious. Independant
review will always find new things that the original coder did not spot
or think of the first time. We will never be able to say "there are no
security problems in VLC", but the goal is to say "we have reviewed our
code many times and did not find anything".

   If you decide to help, you could just pick up a random source file,
review it, and add comments here and there whenever something security-
related isn't obvious. For instance, if there is code like this:

     char psz_buf[21];
     uint64_t i = GetValue();
     sprintf( psz_buf, "%d", i );

   Comment it like this:

     char psz_buf[21]; /* An uint64_t never has more than 20 digits */
     uint64_t i = GetValue();
     sprintf( psz_buf, "%d", i ); /* psz_buf is large enough, see above */

   Also, you may want to add comments in file headers, after the authors
entry for instance:

     * Copyright (C) 1999, 2000, 2001, 2002 VideoLAN
     * $Id: threads.c,v 1.41 2003/08/28 17:19:41 sam Exp $
     *
     * Authors: Jean-Marc Dressler <polux at via.ecp.fr>
     *          Samuel Hocevar <sam at zoy.org>
     *          Gildas Bazin <gbazin at netcourrier.com>
     * Reviewed: John Doe <jon at doe> 2003/10/05
     *

   Streams can contain nasty non-compliant values that may not be
handled properly by VLC. This can lead to memory leaks or simple crashes
(DoS vulnerabilities), but also to execution of arbitrary code on the
player's machine, which is a lot more dangerous. Reviewing should be
done with these two basic assumptions in mind:

  * Do not trust the stream.

  * Do not trust the user.

   (For those who wonder why the user shouldn't be trusted, think of the
    http interface, or think of situations where the user is on a web
    terminal using VLC as an HTTP plugin, but the admin only wants him
    to browse the web.)


 2. Simple examples of things to check
---------------------------------------

  * Return values: For instance if a function claims to return values in
    the range 0-255, check that it really does. If a call assumes that
    a return value will be in a certain range and it is not obvious,
    document the code that does the assumption. Try to use doxygen-style
    comments, for instance:

     /**
      * Create playlist
      *
      * Create a playlist structure.
      * \param p_parent the vlc object that is to be the parent of this playlist
      * \return a pointer to the created playlist, or NULL on error
      */
     playlist_t * __playlist_Create ( vlc_object_t *p_parent )

  * Loops: Make sure that the exit condition of a loop can always be
    reached, especially if it depends on the stream or on user input.

  * Buffers: each time a buffer is allocated with a fixed size, make
    sure it is big enough for whatever operations are done on it after-
    wards. Here is some food for those who don't know where to start:

     find -name '*.c' | xargs grep 'char[^=]*\[.*\].*;'

  * Values read from the stream: see for instance the following code:

     char buffer[1024+1];
     int size = GetBits(32);
     if( size > 1024 ) size = 1024;
     memcpy(buffer, otherbuffer, size);
     buffer[size] = '\0';

    Looks OK, eh? Because we make sure that size isn't too big? But what
    if size is negative! Always check boundaries of read values!

  * A lot of return values are not checked at all, especially from
    malloc(). These aren't hard to find nor to fix.


 3. That's not all, folks
--------------------------

   Of course this is not perfect. For instance, we don't know how the
libraries we use will behave when given corrupted data, and we cannot
ultimately trust their authors for doing the checks, so maybe we will
need to audit external code as well. But let us first sweep in front of
our own door!

   For those who have time, I suggest reading the Secure Programming
HOWTO [2], which is pretty big but has a lot of sections that directly
apply to VLC.

   There may also be design issues that will not be spotted with such a
keyhole review, such as race conditions. Or what happens if a webpage
contains a "vlc:quit" target, or something even nastier? However I
haven't thought about them yet, so it will be another dicussion. If you
have more ideas, please comment!


[1] http://lists.insecure.org/lists/bugtraq/2003/Sep/0003.html
[2] http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/index.html
-- 
Sam.

-- 
This is the vlc-devel mailing-list, see http://www.videolan.org/vlc/
To unsubscribe, please read http://developers.videolan.org/lists.html
If you are in trouble, please contact <postmaster at videolan.org>