Recent security fixes

[ellie timoney]

On Thu, Oct 22, 2015, at 07:22 AM, Florian Weimer wrote:
> On 10/19/2015 06:38 AM, ellie timoney wrote:
> 
> > On Mon, Oct 5, 2015, at 08:09 PM, Florian Weimer wrote:
> >> Hi,
> >>
> >> Martin Prpic pointed out that you apparently fixed a security issue:
> >>
> >> <http://openwall.com/lists/oss-security/2015/09/29/2>
> >>
> >> This is great, thanks.  I think this is the relevant commit:
> >>
> >> <https://cyrus.foundation/cyrus-imapd/commit/?id=07de4ff1bf2fa340b9d77b8e7de8d43d47a33921>
> >>
> >> However, I wonder if the fix is complete.  Could n turn negative
> >> (possibly after truncation)?  Then the range checks seem incomplete.
> > 
> > Good catch.  For some reason n is declared as int, even though
> > everything that uses it expects some sort of unsigned value (unsigned,
> > unsigned long, size_t).  But if the message being fetched is larger than
> > the maximum value of an int (unlikely, but I guess not impossible), then
> > n can wrap around to negative and "else if (start_octet + n > size)"
> > will be incorrect.
> > 
> > I guess the fix will be to make n an unsigned type, probably unsigned
> > long for consistency with the parameters to index_urlfetch, though it'd
> > be nice to fix all this to use size_t.
> 
> Yes, that's probably the best approach if the code already uses unsigned
> variables elsewhere.  Overflow checking for additions is also easier
> with unsigned types.
> 
> > Looking closer, and assuming n has already been made unsigned, I can
> > also see a case where if n is towards the upper end of values that fit
> > in an unsigned long, such that start_octet + n wraps around, then we
> > have a problem there too.
> > 
> > Changing this check to something like:
> > 
> >      else if (n > size || start_octet + n > size)
> > 
> > ... protects us from that sort of thing iff n is greater than size, but
> > if size was already toward the upper limit itself, then start_octet + n
> > could wrap around even if n alone is less than size.
> > 
> > So maybe something like:
> > 
> >      else if (n > size || start_octet + n < start_octet || start_octet +
> >      n > size)
> > 
> > That seems like it'll protect us from:
> >    * n is too big regardless of start_octet
> >    * start_octet + n overflows and wraps around
> >    * start_octet + n is bigger than size (original case)
> 
> I think if n > size, and start_octet + n does not overflow, then
> start_octet + n > size.  So the n > size condition seems redundant.
> 
> But I may have missed something, I'm not familiar with this code at all.
> 

I was thinking the same thing about "n > size" after I hit send.

These two commits make the changes discussed above:

  https://cyrus.foundation/cyrus-imapd/commit/?id=745e161c834f1eb6d62fc14477f51dae799e1e08
and
  https://cyrus.foundation/cyrus-imapd/commit/?id=6fb6a272171f49c79ba6ab7c6403eb25b39ec1b2

(They have also been backported to the 2.5 and 2.4 branches.)

> >> https://cyrus.foundation/cyrus-imapd/commit/?id=c21e179c1f6b968fe69bebe079176714e511587b
> > 
> > This commit is part of the same fix as the "n" one discussed above, so I
> > guess it should be tracked or not tracked as per that one.
> 
> Oh, I wasn't ware of that.  Are there any other commits missing, apart
> from the follow-up fix discussed above?

I don't think so.

Cheers,

ellie