On Wed, 2022-11-02 at 17:00 +0100, Claudio Jeker wrote:
> On Wed, Nov 02, 2022 at 07:33:14AM +0100, Martijn van Duren wrote:
> > I found 2 minor issues in the handling of sequences/sets in
> > ober_read_element():
> > 1) An empty sequence/set (which is basically always) unconditionally
> > creates an (uninitialised) sub-element. Add the same length check
> > used to check the next element
> > 2) For each sub-element r is only checked for -1, but not if it
> > overflows the length of the sequence itself. This is not a big risk
> > since each sub-element is length-checked against the buffer
> > availability and simply returns an ECANCELED, which would be no
> > worse memory-wise than sending an extremely large packet.
> >
> > While here, only having the NULL of the comparison on the next line
> > annoyed me.
> >
> > OK?
>
> See below.
>
> > martijn@
> >
> > Index: ber.c
> > ===================================================================
> > RCS file: /cvs/src/lib/libutil/ber.c,v
> > retrieving revision 1.23
> > diff -u -p -r1.23 ber.c
> > --- ber.c 21 Oct 2021 08:17:33 -0000 1.23
> > +++ ber.c 2 Nov 2022 06:28:33 -0000
> > @@ -1375,7 +1375,7 @@ ober_read_element(struct ber *ber, struc
> > break;
> > case BER_TYPE_SEQUENCE:
> > case BER_TYPE_SET:
> > - if (elm->be_sub == NULL) {
> > + if (len > 0 && elm->be_sub == NULL) {
> > if ((elm->be_sub = ober_get_element(0)) == NULL)
> > return -1;
> > }
>
> OK, be_sub == NULL is something checked and better than an empty object.
I'm not sure I understand what you mean here.
>
> > @@ -1390,13 +1390,21 @@ ober_read_element(struct ber *ber, struc
> > return -1;
> > }
> > r = ober_read_element(ber, next);
> > - if (r == -1)
> > + if (r == -1) {
> > + /* sub-element overflows sequence/set */
>
> This comment is not quite right. I think the proper comment here is:
I'm not sure. Yes the sub-element overflows the buffer, so it is more
accurate in the literal sense. However, since the sequence/set already
has its entire length in the buffer it implies that an ECANCELED is an
overflow of the sub-element of the sequence and therefore makes it
clearer why the errno is overwritten.
>
> /* sub-element overflows buffer */
> > + if (errno == ECANCELED)
> > + errno = EINVAL;
> > return -1;
> > + }
> > + if (r > len) {
>
> This check here is actually checking that the sub_element does not
> overflow the sequence/set. So maybe move the abcve comment down here.
I think it does exactly the same thing, with the only difference that
the ECANCELED case didn't reside fully in the buffer. The reason I
didn't add the comment here was because I think it's clear from the
context, where that context might be less obvious at ECANCELED.
>
> > + errno = EINVAL;
> > + return -1;
> > + }
> > elements++;
> > len -= r;
> > if (len > 0 && next->be_next == NULL) {
> > - if ((next->be_next = ober_get_element(0)) ==
> > - NULL)
> > + next->be_next = ober_get_element(0);
> > + if (next->be_next == NULL)
> > return -1;
> > }
> > next = next->be_next;
> >
>
> Apart from that OK claudio@
>
