Recent security fixes

ellie timoney ellie at
Mon Oct 19 00:38:03 EDT 2015

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:
> <>
> This is great, thanks.  I think this is the relevant commit:
> <>
> 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:

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).


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).


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



More information about the Cyrus-devel mailing list