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. >> 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? Florian
