Recent security fixes

Florian Weimer fweimer at
Wed Oct 21 16:22:16 EDT 2015

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

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.

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


More information about the Cyrus-devel mailing list