Re: ber.c: Fix some minor issues in ober_read_element()

2022-11-02 Thread Martijn van Duren
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 -  1.23
> > > > > > +++ ber.c   2 Nov 2022 06:28:33 -
> > > > > > @@ -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) {
> > > > > 
> > > 

Re: ber.c: Fix some minor issues in ober_read_element()

2022-11-02 Thread Claudio Jeker
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 -  1.23
> > > > > +++ ber.c 2 Nov 2022 06:28:33 -
> > > > > @@ -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.

> > > > 
> > > > /* 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 

Re: ber.c: Fix some minor issues in ober_read_element()

2022-11-02 Thread Martijn van Duren
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 -  1.23
> > > > +++ ber.c   2 Nov 2022 06:28:33 -
> > > > @@ -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.

So is the comment OK as is, do you still want to go with your comment,
or do you want to suggest something else?
> 
> > > 
> > >   /* 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?
> 
> > > 
> > > > +   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)
> > > >   

Re: ber.c: Fix some minor issues in ober_read_element()

2022-11-02 Thread Claudio Jeker
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 -  1.23
> > > +++ ber.c 2 Nov 2022 06:28:33 -
> > > @@ -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.

> > 
> > > @@ -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?

> > 
> > /* 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.

> > 
> > > + 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@
> > 
> 

-- 
:wq Claudio



Re: ber.c: Fix some minor issues in ober_read_element()

2022-11-02 Thread Martijn van Duren
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 -  1.23
> > +++ ber.c   2 Nov 2022 06:28:33 -
> > @@ -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@
> 



Re: ber.c: Fix some minor issues in ober_read_element()

2022-11-02 Thread Claudio Jeker
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 -  1.23
> +++ ber.c 2 Nov 2022 06:28:33 -
> @@ -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.

> @@ -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:

/* 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.

> + 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@

-- 
:wq Claudio



ber.c: Fix some minor issues in ober_read_element()

2022-11-02 Thread Martijn van Duren
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?

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 -  1.23
+++ ber.c   2 Nov 2022 06:28:33 -
@@ -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;
}
@@ -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 */
+   if (errno == ECANCELED)
+   errno = EINVAL;
return -1;
+   }
+   if (r > len) {
+   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;