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
