Re: [Qemu-devel] [Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[Philippe Mathieu-Daudé]

On 8/22/19 8:33 PM, Samuel Thibault wrote:
> Philippe Mathieu-Daudé, le jeu. 22 août 2019 16:41:33 +0200, a ecrit:
>>   Later the newly calculated pointer q is converted into ip structure
>>   and values are modified, Due to the wrong calculation of the delta,
>>   ip will be pointing to incorrect location and ip_src and ip_dst can
>>   be used to write controlled data onto the calculated location. This
>>   may also crash qemu if the calculated ip is located in unmaped area.
> 
> That does not seem to be related to this:

Indeed, I wonder if this is the same issue reported in the CVE.

>> Do not queue fragments pointing out of the original payload to avoid
>> to calculate the variable delta.
> 
> I don't understand the relation with having to calculate delta.
> 
>> diff --git a/src/ip_input.c b/src/ip_input.c
>> index 7364ce0..ee52085 100644
>> --- a/src/ip_input.c
>> +++ b/src/ip_input.c
>> @@ -304,6 +304,19 @@ static struct ip *ip_reass(Slirp *slirp, struct ip *ip, 
>> struct ipq *fp)
>>          ip_deq(q->ipf_prev);
>>      }
>>  
>> +    /*
>> +     * If we received the first fragment, we know the original
>> +     * payload size.
> 
> ? We only know the total payload size when receiving the last fragment
> (payload = offset*8 + size).
> 
>> Verify fragments are within our payload.
> 
> By construction of the protocol, fragments can only be within the
> payload, since it's the last fragment which provides the payload size.

I might have misunderstood the RFC, I'll read it again.

>> +    for (q = fp->frag_link.next; q != (struct ipasfrag*)&fp->frag_link;
>> +            q = q->ipf_next) {
>> +        if (!q->ipf_off && q->ipf_len) {
>> +            if (ip->ip_off + ip->ip_len >= q->ipf_len) {
>> +                goto dropfrag;
>> +            }
>> +        }
>> +    }
> 
> Fragments are kept in order, there is no need to go around the list to
> find the fragment with offset zero, if it is there it is the first one.

OK.

> Did you make your test with commit 126c04acbabd ("Fix heap overflow in
> ip_reass on big packet input") applied?

Yes, unfortunately it doesn't fix the issue.

Thanks,

Phil.