Re: [patch] fix out of bounds memory read in forward_to_info_syntax() / info-utils.c

[Gavin Smith]

On 10 July 2016 at 11:47, Hanno Böck <address@hidden> wrote:
> Hi,
>
> There is an out of bounds invalid memory read in the function
> forward_to_info_syntax().
>
> This is the code (info-utils.c):
>       if (looking_at_string (contents, INFO_MENU_ENTRY_LABEL)
>           || looking_at_string (contents, INFO_XREF_LABEL)
>           || !memcmp (contents, "\0\b[", 3))
>
> The problem is the memcmp, contents can be a string shorter than 3
> bytes. To fix this one can use strncmp instead of memcmp, then the
> comparison will stop in that case. See attached patch. The patch is
> against 6.1, but it also applies against latest svn.

I acknowledge that there is a bug here, but I don't think you have the
right fix.  Merely changing memcmp to strcmp, as in your patch:

--- a/info/info-utils.c 2016-01-30 13:02:41.000000000 +0100
+++ texinfo-6.1/info/info-utils.c       2016-07-10 00:07:33.393993637 +0200
@@ -1621,7 +1621,7 @@
          long index node. */
       if (looking_at_string (contents, INFO_MENU_ENTRY_LABEL)
           || looking_at_string (contents, INFO_XREF_LABEL)
-          || !memcmp (contents, "\0\b[", 3))
+          || !strncmp (contents, "\0\b[", 3))
         return contents;
       contents++;
     }

means that the condition will be true whenever a null byte is reached,
rather when the three bytes "\0\b[" are reached. This is because
strncmp has a weaker condition for equality than memcmp, in that
anything after null bytes is ignored. Example program:

===== cut here ==========
#include <stdlib.h>
#include <string.h>

int
main( void)
{
  if (!strncmp ("\0xy", "\0ab", 3))
    puts ("equal");
  if (!strncmp ("\0xy", "\0xy", 3))
    puts ("equal");
  if (!memcmp ("\0xy", "\0ab", 3))
    puts ("equal");
  else
    puts ("unequal");
  if (!memcmp ("\0xy", "\0xy", 3))
    puts ("equal");
}
======= cut here ==========

Output:

equal
equal
unequal
equal

(Admittedly, there isn't a good reason for null bytes to occur in Info
files, but it's safer to guard against the possibility.)

I think the following would be a correct fix, because none of the
strings that are sought are shorter than three bytes:

Index: info-utils.c
===================================================================
--- info-utils.c        (revision 7149)
+++ info-utils.c        (working copy)
@@ -1576,7 +1576,7 @@
 static char *
 forward_to_info_syntax (char *contents)
 {
-  while (contents < input_start + input_length)
+  while (contents < input_start + input_length - 3)
     {
       /* Menu entry comes first to optimize for the case of looking through a
          long index node. */

I wonder if you could try this patch and report whether it works?