some potential security issue for edid-decode

[Alan Coopersmith]

In general, we'd prefer reports of security issues go to our security list
(which I've cc'ed) instead of our general discussion list, but in this case,
unless I'm missing something, in our code, these are just plain bugs, as
there's no trust or privilege boundary someone can exploit here.

If someone made that code setuid-root or used it to process input from an
untrusted source, then the situation might be different.

	-Alan Coopersmith-              alan.coopersmith at oracle.com
	 Oracle Solaris Engineering - http://blogs.oracle.com/alanc

On 09/29/16 07:48 AM, shirish शिरीष wrote:
> Hi all,
>
> First of all thank you all for the wonderful work you are during in
> X.org and its components. I have been using x.org and it components
> forever.
>
> Please make sure to CC me in case anybody replies as I have turned off
> general replies from the list as I'm already info. overloaded.
>
> I have been looking at a tool called I-Nex. During course of things I
> came across the following -
>
> ┌─[shirish at debian] - [~/games/I-Nex] - [10043]
> └─[$] flawfinder -Q -c .
>
> Flawfinder version 1.31, (C) 2001-2014 David A. Wheeler.
> Number of rules (primarily dangerous function names) in C/C++ ruleset: 169
> ./JSON/i-nex-edid.c:137:  [2] (buffer) char:
>   Statically-sized arrays can be improperly restricted, leading to potential
>   overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
>   functions that limit length, or ensure that the size is larger than the
>   maximum possible length.
>     static char name[4];
> ./JSON/i-nex-edid.c:153:  [2] (buffer) char:
>   Statically-sized arrays can be improperly restricted, leading to potential
>   overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
>   functions that limit length, or ensure that the size is larger than the
>   maximum possible length.
>     const unsigned char empty[3] = { 0, 0, 0 };
> ./JSON/i-nex-edid.c:211:  [2] (buffer) char:
>   Statically-sized arrays can be improperly restricted, leading to potential
>   overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
>   functions that limit length, or ensure that the size is larger than the
>   maximum possible length.
>     static char ret[128];
> ./JSON/i-nex-edid.c:241:  [2] (buffer) char:
>   Statically-sized arrays can be improperly restricted, leading to potential
>   overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
>   functions that limit length, or ensure that the size is larger than the
>   maximum possible length.
>     static unsigned char name[53];
> ./JSON/i-nex-edid.c:1587:  [2] (buffer) char:
>   Statically-sized arrays can be improperly restricted, leading to potential
>   overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
>   functions that limit length, or ensure that the size is larger than the
>   maximum possible length.
>         char buf[3];
> ./JSON/i-nex-edid.c:1621:  [2] (buffer) char:
>   Statically-sized arrays can be improperly restricted, leading to potential
>   overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
>   functions that limit length, or ensure that the size is larger than the
>   maximum possible length.
>         char buf[3];
> ./JSON/i-nex-edid.c:1683:  [2] (buffer) char:
>   Statically-sized arrays can be improperly restricted, leading to potential
>   overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
>   functions that limit length, or ensure that the size is larger than the
>   maximum possible length.
>         char buf[3];
> ./JSON/i-nex-edid.c:1776:  [2] (misc) open:
>   Check when opening files - can an attacker redirect it (via symlinks),
>   force the opening of special file type (e.g., device files), move things
>   around to create a race condition, control its ancestors, or change its
>   contents? (CWE-362).
>         if ((fd = open(argv[1], O_RDONLY)) == -1) {
> ./JSON/i-nex-edid.c:1783:  [2] (misc) open:
>   Check when opening files - can an attacker redirect it (via symlinks),
>   force the opening of special file type (e.g., device files), move things
>   around to create a race condition, control its ancestors, or change its
>   contents? (CWE-362).
>         if ((fd = open(argv[1], O_RDONLY)) == -1) {
> ./JSON/i-nex-edid.c:1787:  [2] (misc) open:
>   Check when opening files - can an attacker redirect it (via symlinks),
>   force the opening of special file type (e.g., device files), move things
>   around to create a race condition, control its ancestors, or change its
>   contents? (CWE-362).
>         if ((ofd = open(argv[2], O_WRONLY)) == -1) {
> ./JSON/i-nex-edid.c:319:  [1] (buffer) strncat:
>   Easily used incorrectly (e.g., incorrectly computing the correct maximum
>   size to add) (CWE-120). Consider strcat_s, strlcat, or automatically
>   resizing strings.
>         strncat((char *)name, (char *)x + 5, 13);
> ./JSON/i-nex-edid.c:324:  [1] (buffer) strlen:
>   Does not handle strings that are not \0-terminated; if given one it may
>   perform an over-read (it could cause a crash if unprotected) (CWE-126).
>                       strlen((char *)name)));
> ./JSON/i-nex-edid.c:1521:  [1] (buffer) read:
>   Check buffer boundaries if used in a loop including recursive loops
>   (CWE-120, CWE-20).
>     i = read(fd, ret + len, size - len);
> ./JSON/i-nex-edid.c:1576:  [1] (buffer) strlen:
>   Does not handle strings that are not \0-terminated; if given one it may
>   perform an over-read (it could cause a crash if unprotected) (CWE-126).
>         start = s + strlen(indentation);
> ./JSON/i-nex-edid.c:1735:  [1] (buffer) strlen:
>   Does not handle strings that are not \0-terminated; if given one it may
>   perform an over-read (it could cause a crash if unprotected) (CWE-126).
>     for (i = strlen(name); i < 15; i++)
>
> ANALYSIS SUMMARY:
>
> Hits = 15
> Lines analyzed = 3128 in approximately 0.39 seconds (7995 lines/second)
> Physical Source Lines of Code (SLOC) = 2745
> Hits at level = [0]   0 [1]   5 [2]  10 [3]   0 [4]   0 [5]   0
> Hits at level+ = [0+]  15 [1+]  15 [2+]  10 [3+]   0 [4+]   0 [5+]   0
> Hits/KSLOC at level+ = [0+] 5.46448 [1+] 5.46448 [2+] 3.64299 [3+]   0
> [4+]   0 [5+]   0
> Dot directories skipped = 7 (--followdotdir overrides)
> Minimum risk level = 1
> Not every hit is necessarily a security vulnerability.
> There may be other security vulnerabilities; review your code!
> See 'Secure Programming for Linux and Unix HOWTO'
> (http://www.dwheeler.com/secure-programs) for more information.
>
> I filed an issue with the author at https://github.com/eloaders/I-Nex/issues/47
>
> and came to know that this is a clone of the edid-decode package
> managed by freedesktop.
>
> I am not really a coder hence can't help. It is also possible that the
> errors or warnings may be false positives but that's upto the X.org
> developers to see if there is any issue or not.
>
> I did have a cursory look at
> https://bugs.freedesktop.org/buglist.cgi?quicksearch=edid-decode but
> didn't find anything which corresponds to the warnings highlighted
> above.
>
> Maybe somebody can take a look at it. Please let me know if you need
> me to share some more info. from my side.
>