On Wed, 2022-11-02 at 18:34 +0100, Claudio Jeker wrote:
> On Wed, Nov 02, 2022 at 05:56:21PM +0100, Martijn van Duren wrote:
> > On Wed, 2022-11-02 at 17:47 +0100, Claudio Jeker wrote:
> > > On Wed, Nov 02, 2022 at 05:25:12PM +0100, Martijn van Duren wrote:
> > > > 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.
> > >
> > > I just wanted to say that elm->be_sub == NULL is a condition that most
> > > other code handles well. While having an empty element in elm->be_sub is
> > > something other code may not handle well.
> >
> > Ah ok, so confirming that the change is OK. Thanks.
> > >
> > > > >
> > > > > > @@ -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.
> > >
> > > Actually why do we need to reset the errno anyway?
> > > Is anything in userland looking at ECANCELED and retries after receiving
> > > more data?
> >
> > Yes, aldap.c and snmpe.c do this. And even if it didn't, going with
> > EINVAL is more concise than ECANCELED.
>
> I really dislike such errno magic but fixing that is a much bigger
> project.
>
> > So is the comment OK as is, do you still want to go with your comment,
> > or do you want to suggest something else?
>
> My question is should we actually write this as:
>
> if (r == -1 || r > len) {
> errno = EINVAL;
> return -1;
> }
>
> In the end any error of a subelement can be interpreted as an illegal seq/set.
There are a few ERANGE cases in the ber_read_elements code, which are
valid ber, but too big for our ber.c code. So I don't think that
statement is definitionally true.
I agree that the errno magic isn't pretty, but I don't see a clean way
to implement the fix without the errno magic.
So unless we want a too wide brush I guess my original diff still
stands?
>
> > > > >
> > > > > /* 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.
> > >
> > > Sure. It is kind of same same. The check is important though.
> >
> > Yes, but the question still remains: Do I need to add a comment here?
>
> No
>
> > >
> > > > >
> > > > > > + 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@
> > > > >
> > > >
> > >
> >
>
