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