[Bug libc/756] New: missing arithmetic overflow check

[Bug libc/756] New: missing arithmetic overflow check

[simon at josefsson dot org]

Thanks for installing iconvme!

I proposed to add the module to gnulib again, and another minor nit was spotted.
 This patch fixes that.

Thanks.

2005-02-22  Simon Josefsson  <jas@extundo.com>

	* iconvme.c (iconv_string): Protect arithmetic overflow for
	outbuf_size buffer size computation.  From Paul Eggert
	<eggert@CS.UCLA.EDU>.

Index: libidn/iconvme.c
===================================================================
RCS file: /cvs/glibc/libc/libidn/iconvme.c,v
retrieving revision 1.1
diff -u -p -r1.1 iconvme.c
--- libidn/iconvme.c	22 Feb 2005 01:24:52 -0000	1.1
+++ libidn/iconvme.c	22 Feb 2005 19:51:08 -0000
@@ -67,6 +67,12 @@ iconv_string (const char *str, const cha
   size_t outbytes_remaining = outbuf_size - 1; /* -1 for NUL */
   size_t err;
   int have_error = 0;
+
+  if (1 < MB_LEN_MAX && SIZE_MAX / MB_LEN_MAX <= inbytes_remaining)
+    {
+      errno = ENOMEM;
+      return NULL;
+    }
 #endif
 
   if (strcmp (to_codeset, from_codeset) == 0)

-- 
           Summary: missing arithmetic overflow check
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
        AssignedTo: gotom at debian dot or dot jp
        ReportedBy: simon at josefsson dot org
                CC: glibc-bugs at sources dot redhat dot com


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/756] missing arithmetic overflow check

[jakub at redhat dot com]

------- Additional Comments From jakub at redhat dot com  2005-02-23 17:24 -------
Well, the only problematic case would be if strlen (p) returns (size_t) -1.
But that just isn't going to happen, you couldn't have anything else in the
address space.  In all other cases, even if arithmetic overflow happens,
iconv will know the actual size of the output buffer and if the input is
too long, it will just return E2BIG and return ENOMEM anyway.

-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/756] missing arithmetic overflow check

[simon at josefsson dot org]

------- Additional Comments From simon at josefsson dot org  2005-02-23 23:28 -------
No, I think the problem is if:

  size_t outbuf_size = (inbytes_remaining + 1) * MB_LEN_MAX;

results in 0, due to some overflow in the * operation.  I.e., let's say strlen
(p) is SIZE_MAX / MB_LEN_MAX - 1.  Then outbuf_size would be 0, and then

  size_t outbytes_remaining = outbuf_size - 1; /* -1 for NUL */

outbytes_remaining would be SIZE_MAX, which leads to a buffer overrun because
iconv will think the buffer is SIZE_MAX large, but the allocated size is only 0.

I could be mistaken though, I find the issues slightly subtle at times.


-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/756] missing arithmetic overflow check

[jakub at redhat dot com]

------- Additional Comments From jakub at redhat dot com  2005-02-23 23:44 -------
You are right, the overflow can happen for inbytes_remaining
being (SIZE_MAX/MB_LEN_MAX)+N*(SIZE_MAX/MB_LEN_MAX+1) for N=0,1,...
Still, I think the test would be better written as
if (outbuf_size == 0)
  {
    errno = ENOMEM;
    return NULL;
  }

-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/756] missing arithmetic overflow check

[simon at josefsson dot org]

------- Additional Comments From simon at josefsson dot org  2005-02-23 23:49 -------
I think that would work.  I'll propose it on the gnulib list.  Please go ahead
and apply that change if you feel comfortable with it.

-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/756] missing arithmetic overflow check

[eggert at gnu dot org]

------- Additional Comments From eggert at gnu dot org  2005-02-24 00:36 -------
(In reply to comment #4)
> I think that would work.  I'll propose it on the gnulib list.  Please go ahead
> and apply that change if you feel comfortable with it.

No, a simple check against zero is incorrect.
outbuf_size isn't set to zero when an
overflow occurs.  Unsigned arithmetic wraps around, so outbuf_size
could be any of a wide variety of values in the size_t range.

For example, suppose MB_LEN_MAX is 16, inbytes_remaining is 2**24, and
we are on a 32-bit machine.  Then

  size_t outbuf_size = (inbytes_remaining + 1) * MB_LEN_MAX;

would set inbytes_remaining to 16, not zero.

Please look again at the check of inbytes_remaining against
SIZE_MAX / MB_LEN_MAX; that does the right thing.

-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/756] missing arithmetic overflow check

[jakub at redhat dot com]

------- Additional Comments From jakub at redhat dot com  2005-02-24 00:59 -------
With inbytes_remaining 2**24 outbuf_size will be 0x10000010.
With 2**28 outbuf_size will be 16, sure, but that still doesn't mean the string
can't be converted.
iconv honors whatever outbytes_remaining it is given, so the only problem
is if outbytes_remaining is -1 and outbuf_size is 0, then we allocated less than
we are telling iconv there is available.

The initial outbuf_size computation is in fact just a hint, so the routine
shouldn't fail just because it tried to allocate too large buffer.
The output string might be shortern than input string, or about the same length,
not 16 times longer.
It can do
if (outbuf_size <= inbytes_remaining) outbuf_size = inbytes_remaining + 1;
or something similar and let the ENOMEM only pop up if malloc or realloc
fails.  And perhaps if the first malloc fails, but outbuf_size is bigger than
inbytes_remaining + 1, it could retry with outbuf_size = inbutes_remaining + 1.

-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/756] missing arithmetic overflow check

[eggert at gnu dot org]

------- Additional Comments From eggert at gnu dot org  2005-03-08 20:52 -------
Created an attachment (id=432)
 --> (http://sources.redhat.com/bugzilla/attachment.cgi?id=432&action=view)
avoid iconvme buffer overrun and some malloc failures

Here is a proposed patch that addresses Jakub's comments, I think.

-- 


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/756] missing arithmetic overflow check

[jakub at redhat dot com]

------- Additional Comments From jakub at redhat dot com  2005-03-15 15:38 -------
Sounds good to me.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |drepper at redhat dot com


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/756] missing arithmetic overflow check

[roland at gnu dot org]

------- Additional Comments From roland at gnu dot org  2005-03-15 22:22 -------
The comment#7 patch is in.  Note for future reference, libidn has its own
ChangeLog so libidn/ should not appear in the file names in the log entries.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


http://sources.redhat.com/bugzilla/show_bug.cgi?id=756

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.