[Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload
Philippe Mathieu-Daudé
philmd at redhat.com
Fri Aug 23 15:15:32 UTC 2019
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.
More information about the Slirp
mailing list