On Tue, 2023-08-22 at 11:16 +0200, Martijn van Duren wrote:
> On Mon, 2023-08-21 at 07:35 +0000, Gerhard Roth wrote:
> > Hi Martijn,
> >
> > last November you fixed ber.c so that sequences won't generate
> > an uninitialized subelement.
> >
> > This revealed another bug in ober_scanf_elements(): it couldn't
> > process sequences with an empty list of subelements. The following
> > code failed in ober_scanf_elements():
> >
> > struct ber_element *root;
> > struct ber_element *sub;
> >
> > if ((root = ober_add_sequence(NULL)) == NULL)
> > err(1, "ober_add_sequence() failed");
> >
> > errno = 0;
> > if (ober_scanf_elements(root, "{e", &sub))
> > err(1, "ober_scanf_elements() failed");
> >
> > printf("sub = %p\n", sub);
> >
> >
> > The patch below fixes that.
> >
> > Gerhard
> >
> >
> > Index: lib/libutil/ber.c
> > ===================================================================
> > RCS file: /cvs/src/lib/libutil/ber.c,v
> > retrieving revision 1.24
> > diff -u -p -u -p -r1.24 ber.c
> > --- lib/libutil/ber.c 3 Nov 2022 17:58:10 -0000 1.24
> > +++ lib/libutil/ber.c 21 Aug 2023 07:24:21 -0000
> > @@ -700,7 +700,8 @@ ober_scanf_elements(struct ber_element *
> >
> > va_start(ap, fmt);
> > while (*fmt) {
> > - if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt !=
> > ')')
> > + if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt !=
> > ')' &&
> > + *fmt != 'e')
> > goto fail;
>
> I'm not sure about this part. An ober_scanf_elements of "{}e" on your
> example above also fails. The 'e' element might not increment the ber
> pointer, but I do think it should fail if an expected element is
> missing.
You're right. And digging into it, only makes it look worse.
I think, the 'e' format should behave differently.
1) in case of '{e' it is the first element of a sequence.
Empty sequences are allowed and we should not fail but
set the argument to NULL instead.
2) in all other cases, 'e' is the next element in the list.
And here we should fail if there are less elements in the
list than expected.
Below is an updated diff that fixes the problem by remembering
whether the last traversal was via 'be_sub' (fsub == 1) or 'be_next'
and then behaves differently. Not very elegant although.
>
> > switch (*fmt++) {
> > case '$':
> > @@ -797,7 +798,7 @@ ober_scanf_elements(struct ber_element *
> > if (ber->be_encoding != BER_TYPE_SEQUENCE &&
> > ber->be_encoding != BER_TYPE_SET)
> > goto fail;
> > - if (ber->be_sub == NULL || level >= _MAX_SEQ-1)
> > + if (level >= _MAX_SEQ-1)
>
> This part is OK martijn@
>
> > goto fail;
> > parent[++level] = ber;
> > ber = ber->be_sub;
> >
>
Index: lib/libutil/ber.c
===================================================================
RCS file: /cvs/src/lib/libutil/ber.c,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 ber.c
--- lib/libutil/ber.c 3 Nov 2022 17:58:10 -0000 1.24
+++ lib/libutil/ber.c 22 Aug 2023 10:10:43 -0000
@@ -695,12 +695,14 @@ ober_scanf_elements(struct ber_element *
off_t *pos;
struct ber_oid *o;
struct ber_element *parent[_MAX_SEQ], **e;
+ int fsub = 0;
memset(parent, 0, sizeof(struct ber_element *) * _MAX_SEQ);
va_start(ap, fmt);
while (*fmt) {
- if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')')
+ if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')' &&
+ *fmt != 'e')
goto fail;
switch (*fmt++) {
case '$':
@@ -732,6 +734,8 @@ ober_scanf_elements(struct ber_element *
case 'e':
e = va_arg(ap, struct ber_element **);
*e = ber;
+ if (fsub)
+ goto fail;
ret++;
continue;
case 'E':
@@ -797,10 +801,11 @@ ober_scanf_elements(struct ber_element *
if (ber->be_encoding != BER_TYPE_SEQUENCE &&
ber->be_encoding != BER_TYPE_SET)
goto fail;
- if (ber->be_sub == NULL || level >= _MAX_SEQ-1)
+ if (level >= _MAX_SEQ-1)
goto fail;
parent[++level] = ber;
ber = ber->be_sub;
+ fsub = 1;
ret++;
continue;
case '}':
@@ -808,6 +813,7 @@ ober_scanf_elements(struct ber_element *
if (level < 0 || parent[level] == NULL)
goto fail;
ber = parent[level--];
+ fsub = 0;
ret++;
break;
default:
smime.p7s
Description: S/MIME cryptographic signature
