Hi Florian, Sorry about the late reply. Comments inline.
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. 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) Does this seem about right? > I also saw some (otherwise unrelated) commits which might be > security-relevant: > > https://cyrus.foundation/cyrus-imapd/commit/?id=d81a712401418cc0bd1daa49ded8e5bcc4b69f21 I don't think this needs to be tracked as a fix for a security vulnerability. imtest is a tool for testing a remote imap server, but the potential overflow being fixed here would occur within the imtest program itself (and probably just crash imtest). > https://cyrus.foundation/cyrus-imapd/commit/?id=ff4e6c71d932b3e6bbfa67d76f095e27ff21bad0 I don't think this needs to be tracked as a fix for a security vulnerability (unless your snprintf doesn't always 0-terminate, in which case the previous implementation was fine). > 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. > Could you comment on whether these fixes need to be tracked as fixes for > security vulnerabilities? > > Thanks, > Florian Cheers, ellie
