Re: Babel: prevent an infinite loop while parsing sub-TLV

[Pascal Quantin <pascal () wireshark org>]

Le ven. 18 oct. 2019 à 21:16, Pascal Quantin <pascal () wireshark org> a
écrit :

> Le ven. 18 oct. 2019 à 20:58, Pascal Quantin <pascal () wireshark org> a
> écrit :
>
> > Hi Juliusz,
> >
> > Le ven. 18 oct. 2019 à 20:51, Juliusz Chroboczek <jch () irif fr> a écrit :
> >
> > > Dear Pascal,
> > >
> > > I've just seen your commit dd15b2, which I believe is incorrect.
> > >
> > >
> > > https://code.wireshark.org/review/gitweb?p=wireshark.git;a=commit;h=dd15b203c7ec8a8ec5c930cf018c838991ee3182
> > >
> > > In dissect_babel_subtlvs, sublen is the length of the TLV body; the full
> > > TLV is of length sublen+2.  At the end of the loop, the code is correct:
> > >
> > >         beg += (sublen+2);
> > >
> > > Hence, it is perfectly fine to have sublen=0.  Unless I'm wrong, your
> > > commit is incorrect.
> >
> > Maybe, honestly I do not remember the details as it was done a few months
> > ago. It would require me to dive in once I have the time (so not now).
> >
> >
> > > Could you please either explain why I'm wrong, or else revert commit
> > > dd15b2?
> >
> > I will not revert this commit as it solved/workarounded an infinite loop,
> > as seen in https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=15856
> > But is you can come up with a better solution that ensures that we do not
> > have the infinite loop while still being correct, you are welcome and I
> > will try to review it
>
> Without even looking at the capture from bug 15856, I can see an infinite
> loop when subtype = MESSAGE_SUB_PAD1 and sublen = 0:
>         if(subtype == MESSAGE_SUB_PAD1){
>             beg += sublen;
>             continue;
>         }
> Maybe the test for sublen value should be put only when the subtype is
> MESSAGE_SUB_PAD1? Or when subtype is different from MESSAGE_SUB_PADN?
> As you seem to know this protocol, your feedback is welcome.

And the code below seems to assume that sublen cannot be 0:
        if (message_tree) {
            subtlv_tree = proto_item_add_subtree(sub_item, ett_subtlv);
            proto_tree_add_item(subtlv_tree, hf_babel_subtlv_type,
                                tvb, beg+2, 1, ENC_BIG_ENDIAN);
        }
This is probably why I added the check just before, as those lines do not
consider a case where sublen would be 0. But after rereading the code, I
guess it is currently selecting the wrong byte and that beg+2 should be
replaced by beg?
And in the same area, in the code below:
        sub_item =
          proto_tree_add_uint_format(message_tree, hf_babel_subtlv,
                                     tvb, beg, sublen, subtype,
                                     "Sub TLV %s (%u)",
                                     val_to_str_const(subtype, subtlvs,
"unknown"),
                                     subtype);
I guess sublen should be replaced by sublen+2?


> Best regards,
> Pascal.
>
> >
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe