Thanks, I will apply your patch, especially since (for the record)
Hideaki posted in a reply:
> > The suggested fix Daniel Hartmeier (an OpenBSD developer) had when
> > I asked (for I'm not really a coder and I wanted to double check
> > that this was a valid problem) was making the mask[w] assignment
> > conditional like thus:
> >
> > if (w < addrlen)
> > mask[w] = 0xff & (0xff<<(8-b));
>
> Absolutely... Please apply the suggested fix.
> Thanks for finding / fixing this serious bug.
By the way, in the future I would appreciate it if you would post bugs to
the rsync mailing list instead of to me because other rsync team members
should see these things too and might be able to reply faster than me.
- Dave Dykstra
On Sat, Jan 18, 2003 at 06:21:28PM -0500, Brian Poole wrote (in private
email):> Hello,
>
> Here is a proposed patch to fix a couple problems with recent changes
> incorporated into access.c.
>
> The first is the biggest problem, there is a possible overflow
> in make_mask() if w is the same size as addrlen. Feel free to
> skip the explanation, its just there in case you don't see what
> I see.. patch is attached & at the bottom.
>
> I will walkthrough where I see the bug to have this make sense. I
> start in match_address() where I note char mask[16] at the top.
> Continuing on, we get to a switch statement for the address family;
> we imagine we take the PF_INET6 case. Two things of note here, 1st
> is addrlen = 16; 2nd is what appears to be a pasto, two copies of
>
> a = (char *)&sin6a->sin6_addr;
> t = (char *)&sin6t->sin6_addr;
>
> are found, surrounding an if statement that shouldn't affect the
> values. Thats the 2nd, minor, problem I wanted to note. I also
> moved the addrlen in my diff for cosmetic purposes. Doesn't
> matter which set of assignments is taken out.
>
> Continuing on for the bigger problem, we exit the switch statement
> and come to an if (p). Imagine p is not set and we take the else.
> This sets bits = 128.
>
> Next up is a make_mask() call with args char mask[16], 128, and 16.
> Going up to make_mask we set w = 128 >> 3, or w = 16. A few lines
> later you then have mask[w] = .... mask[16] is not in mask[] and
> so you have your overflow.
>
> The suggested fix Daniel Hartmeier (an OpenBSD developer) had when
> I asked (for I'm not really a coder and I wanted to double check
> that this was a valid problem) was making the mask[w] assignment
> conditional like thus:
>
> if (w < addrlen)
> mask[w] = 0xff & (0xff<<(8-b));
>
> That sounds like the correct fix to me but I shall leave it to you
> guys to finish this off. I've inlined the patch as well as attaching
> it in case you have a preference.
>
> This bug was noticed due to the propolice patches that are in
> OpenBSD-current which caused the rsync tests (which I do regularly
> for the build farm) to log messages like this:
>
> Jan 18 14:33:41 pandemonium rsync: stack overflow in function match_address
>
> I should have noticed this about 10 days ago but as this is a test
> machine I don't routinely watch the logs ;-) This gets the
> OpenBSD/sparc64 build moving along a bit further than it had been,
> though there is still one test failing (daemon-gzip-download now
> passes, daemon-gzip-upload still fails.)
>
> Will look into the other failure if I have time.
>
>
> -b
>
>
> Index: access.c
> ==================================================================> RCS
file: /cvsroot/rsync/access.c,v
> retrieving revision 1.7
> diff -u -r1.7 access.c
> --- access.c 9 Jan 2003 21:30:24 -0000 1.7
> +++ access.c 18 Jan 2003 23:12:57 -0000
> @@ -51,7 +51,8 @@
>
> if (w)
> memset(mask, 0xff, w);
> - mask[w] = 0xff & (0xff<<(8-b));
> + if (w < addrlen)
> + mask[w] = 0xff & (0xff<<(8-b));
> if (w+1 < addrlen)
> memset(mask+w+1, 0, addrlen-w-1);
>
> @@ -121,6 +122,8 @@
> a = (char *)&sin6a->sin6_addr;
> t = (char *)&sin6t->sin6_addr;
>
> + addrlen = 16;
> +
> #ifdef HAVE_SOCKADDR_IN6_SCOPE_ID
> if (sin6t->sin6_scope_id &&
> sin6a->sin6_scope_id != sin6t->sin6_scope_id) {
> @@ -128,10 +131,6 @@
> goto out;
> }
> #endif
> -
> - a = (char *)&sin6a->sin6_addr;
> - t = (char *)&sin6t->sin6_addr;
> - addrlen = 16;
>
> break;
> }
