library/4603: <synopsis of the problem (one line)>

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

library/4603: <synopsis of the problem (one line)>

Michael Shuldman
>Number:         4603
>Category:       library
>Synopsis:       <synopsis of the problem (one line)>
>Confidential:   yes
>Severity:       serious
>Priority:       high
>Responsible:    bugs
>State:          open
>Quarter:        
>Keywords:      
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Nov 07 14:00:01 GMT 2005
>Closed-Date:
>Last-Modified:
>Originator:     Michael Shuldman
>Release:        <release number or tag (one line)>
>Organization:
Inferno Nettverk A/S, Oslo, Norway; http://www.inet.no
>Environment:
        <machine, os, target, libraries (multiple lines)>
        System      : OpenBSD 3.7
        Architecture: OpenBSD.i386
        Machine     : i386
>Description:
        lib/libwrap/rfc931.c allocates memory for the fd_set with
        calloc(3), but it later uses FD_ZERO() to zero the fd_set.

        When less than sizeof(struct fd_set) gets allocated for
        the fd_set, FD_ZERO() copies beyond the allocated memory,
        and at best a segmentation-fault will occur.


>How-To-Repeat:
        <code/input/activities to reproduce the problem (multiple lines)>
>Fix:
       
Index: rfc931.c
===================================================================
RCS file: /cvs/src/lib/libwrap/rfc931.c,v
retrieving revision 1.9
diff -u -r1.9 rfc931.c
--- rfc931.c 19 Apr 2003 18:31:48 -0000 1.9
+++ rfc931.c 7 Nov 2005 13:36:55 -0000
@@ -85,6 +85,7 @@
  struct sigaction new_sa, old_sa;
  char user[256];
  char tbuf[1024];
+ int rsize, wsize;
 
  gotit = 0;
  s = -1;
@@ -180,10 +181,13 @@
 
  /* We are connected,  build an ident query and send it. */
 
- readfds = calloc(howmany(s+1, NFDBITS), sizeof(fd_mask));
+ rsize = howmany(s+1, NFDBITS) * sizeof(fd_mask);
+ readfds = calloc(1, rsize);
  if (readfds == NULL)
  goto out;
- writefds = calloc(howmany(s+1, NFDBITS), sizeof(fd_mask));
+
+ wsize = howmany(s+1, NFDBITS) * sizeof(fd_mask);
+ writefds = calloc(1, wsize);
  if (writefds == NULL)
  goto out;
  snprintf(tbuf, sizeof(tbuf), "%u,%u\r\n", ntohs(*rmt_portp),
@@ -192,7 +196,7 @@
  while (i < strlen(tbuf)) {
  int j;
 
- FD_ZERO(writefds);
+ memset(writefds, 0, wsize);
  FD_SET(s, writefds);
  do {
  j = select(s + 1, NULL, writefds, NULL, NULL);
@@ -219,7 +223,7 @@
  while ((cp = strchr(tbuf, '\n')) == NULL && i < sizeof(tbuf) - 1) {
  int j;
 
- FD_ZERO(readfds);
+ memset(readfds, 0, rsize);
  FD_SET(s, readfds);
  do {
  j = select(s + 1, readfds, NULL, NULL, NULL);

>Release-Note:
>Audit-Trail:
>Unformatted:

Reply | Threaded
Open this post in threaded view
|

Re: library/4603: <synopsis of the problem (one line)>

Otto Moerbeek
The following reply was made to PR library/4603; it has been noted by GNATS.

From: Otto Moerbeek <[hidden email]>
To: Michael Shuldman <[hidden email]>
Cc: [hidden email], [hidden email]
Subject: Re: library/4603: <synopsis of the problem (one line)>
Date: Thu, 10 Nov 2005 09:25:39 +0100 (CET)

 On Mon, 7 Nov 2005, Michael Shuldman wrote:
 
 > >Number:         4603
 > >Category:       library
 > >Synopsis:       <synopsis of the problem (one line)>
 
 Hi,
 
 Next time pelase provide a synopsis ;-)
 
 I think this is slighly better. It uses size_t for sizes and makes use
 of calloc's int overflow detection.
 
  -Otto
 
 Index: rfc931.c
 ===================================================================
 RCS file: /cvs/src/lib/libwrap/rfc931.c,v
 retrieving revision 1.9
 diff -u -p -r1.9 rfc931.c
 --- rfc931.c 19 Apr 2003 18:31:48 -0000 1.9
 +++ rfc931.c 10 Nov 2005 08:22:45 -0000
 @@ -85,6 +85,7 @@ rfc1413(rmt_sin, our_sin, dest, dsize, i
  struct sigaction new_sa, old_sa;
  char user[256];
  char tbuf[1024];
 + size_t rsize, wsize;
 
  gotit = 0;
  s = -1;
 @@ -180,10 +181,13 @@ rfc1413(rmt_sin, our_sin, dest, dsize, i
 
  /* We are connected,  build an ident query and send it. */
 
 - readfds = calloc(howmany(s+1, NFDBITS), sizeof(fd_mask));
 + rsize = howmany(s+1, NFDBITS);
 + readfds = calloc(rsize, sizeof(fd_mask));
  if (readfds == NULL)
  goto out;
 - writefds = calloc(howmany(s+1, NFDBITS), sizeof(fd_mask));
 +
 + wsize = howmany(s+1, NFDBITS);
 + writefds = calloc(wsize, sizeof(fd_mask));
  if (writefds == NULL)
  goto out;
  snprintf(tbuf, sizeof(tbuf), "%u,%u\r\n", ntohs(*rmt_portp),
 @@ -192,7 +196,7 @@ rfc1413(rmt_sin, our_sin, dest, dsize, i
  while (i < strlen(tbuf)) {
  int j;
 
 - FD_ZERO(writefds);
 + memset(writefds, 0, wsize * sizeof(fd_mask));
  FD_SET(s, writefds);
  do {
  j = select(s + 1, NULL, writefds, NULL, NULL);
 @@ -219,7 +223,7 @@ rfc1413(rmt_sin, our_sin, dest, dsize, i
  while ((cp = strchr(tbuf, '\n')) == NULL && i < sizeof(tbuf) - 1) {
  int j;
 
 - FD_ZERO(readfds);
 + memset(readfds, 0, rsize * sizeof(fd_mask));
  FD_SET(s, readfds);
  do {
  j = select(s + 1, readfds, NULL, NULL, NULL);

Reply | Threaded
Open this post in threaded view
|

Re: library/4603: <synopsis of the problem (one line)>

Michael Shuldman
In reply to this post by Michael Shuldman
The following reply was made to PR library/4603; it has been noted by GNATS.

From: Michael Shuldman <[hidden email]>
To: Otto Moerbeek <[hidden email]>
Cc: [hidden email], [hidden email]
Subject: Re: library/4603: <synopsis of the problem (one line)>
Date: Thu, 10 Nov 2005 12:16:07 +0100

 Otto Moerbeek wrote,
 >
 > On Mon, 7 Nov 2005, Michael Shuldman wrote:
 >
 > > >Number:         4603
 > > >Category:       library
 > > >Synopsis:       <synopsis of the problem (one line)>
 
 
 > I think this is slighly better. It uses size_t for sizes and makes use
 > of calloc's int overflow detection.
 
 Yes, size_t is better, but I think one should use malloc(3) instead
 of calloc(3), since the fd_set is zero'ed manually inside the loop
 before usage anyway.
 
 
 --
   _ //
   \X/ -- Michael Shuldman <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: library/4603: <synopsis of the problem (one line)>

Otto Moerbeek
In reply to this post by Michael Shuldman
The following reply was made to PR library/4603; it has been noted by GNATS.

From: Otto Moerbeek <[hidden email]>
To: Michael Shuldman <[hidden email]>
Cc: [hidden email]
Subject: Re: library/4603: <synopsis of the problem (one line)>
Date: Thu, 10 Nov 2005 19:40:07 +0100 (CET)

 On Thu, 10 Nov 2005, Michael Shuldman wrote:
 
 > The following reply was made to PR library/4603; it has been noted by GNATS.
 >
 > From: Michael Shuldman <[hidden email]>
 > To: Otto Moerbeek <[hidden email]>
 > Cc: [hidden email], [hidden email]
 > Subject: Re: library/4603: <synopsis of the problem (one line)>
 > Date: Thu, 10 Nov 2005 12:16:07 +0100
 >
 >  Otto Moerbeek wrote,
 >  >
 >  > On Mon, 7 Nov 2005, Michael Shuldman wrote:
 >  >
 >  > > >Number:         4603
 >  > > >Category:       library
 >  > > >Synopsis:       <synopsis of the problem (one line)>
 >  
 >  
 >  > I think this is slighly better. It uses size_t for sizes and makes use
 >  > of calloc's int overflow detection.
 >  
 >  Yes, size_t is better, but I think one should use malloc(3) instead
 >  of calloc(3), since the fd_set is zero'ed manually inside the loop
 >  before usage anyway.
 
 I do not agree. Using calloc(3) gets you the overflow check. The extra
 zero'ing does not matter that much. Probably the memset can be moved
 to the end of the loop, but I'd like to focus on the bug only for now.
 
  -Otto