| Message ID | 20210616203448.2324124-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofp-actions: Report an error if there are too many actions to parse. | expand |
On 6/16/21 10:34 PM, Ilya Maximets wrote: > Not a very important fix, but fuzzer times out trying to test parsing > of a huge number of actions. Fixing that by reporting an error as > soon as ofpacts oversized. > > It would be great to use ofpacts_oversized() function instead of manual > size checking, but ofpacts->header here always points to the last > pushed action, so the value that ofpacts_oversized() would check is > always small. Ugh. s/ofpacts_oversized/ofpbuf_oversized/ I can fix that on commit if the new version will not be necessary otherwise. > > Adding a unit test for this, plus the extra test for too deep nesting. > > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20254 > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > lib/ofp-actions.c | 4 ++++ > tests/ofp-actions.at | 6 ++++++ > 2 files changed, 10 insertions(+) > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index 6fb3da507..ecf914eac 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -9326,6 +9326,7 @@ static char * OVS_WARN_UNUSED_RESULT > ofpacts_parse__(char *str, const struct ofpact_parse_params *pp, > bool allow_instructions, enum ofpact_type outer_action) > { > + uint32_t orig_size = pp->ofpacts->size; > char *key, *value; > bool drop = false; > char *pos; > @@ -9370,6 +9371,9 @@ ofpacts_parse__(char *str, const struct ofpact_parse_params *pp, > if (error) { > return error; > } > + if (pp->ofpacts->size - orig_size > UINT16_MAX) { > + return xasprintf("input too big"); > + } > } > > if (drop && pp->ofpacts->size) { > diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at > index 59093c03c..ef4898abb 100644 > --- a/tests/ofp-actions.at > +++ b/tests/ofp-actions.at > @@ -1144,4 +1144,10 @@ bad_action 'apply_actions' 'apply_actions is the default instruction' > bad_action 'xyzzy' 'unknown action xyzzy' > bad_action 'drop,3' '"drop" must not be accompanied by any other action or instruction' > > +# Too many actions > +writes=$(printf 'write_actions(%.0s' $(seq 100)) > +bad_action "${writes}" 'Action nested too deeply' > +outputs=$(printf '1,%.0s' $(seq 4096)) > +bad_action "${outputs}" 'input too big' > + > AT_CLEANUP >
From: Alin-Gabriel Serdean <aserdean@ovn.org> >Not a very important fix, but fuzzer times out trying to test parsing >of a huge number of actions. Fixing that by reporting an error as >soon as ofpacts oversized. > >It would be great to use ofpacts_oversized() function instead of manual >size checking, but ofpacts->header here always points to the last >pushed action, so the value that ofpacts_oversized() would check is >always small. > >Adding a unit test for this, plus the extra test for too deep nesting. > >Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20254 >Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > Acked-by: Alin-Gabriel Serdean <aserdean@ovn.org>
On 7/2/21 1:40 PM, aserdean@ovn.org wrote: > From: Alin-Gabriel Serdean <aserdean@ovn.org> > >> Not a very important fix, but fuzzer times out trying to test parsing >> of a huge number of actions. Fixing that by reporting an error as >> soon as ofpacts oversized. >> >> It would be great to use ofpacts_oversized() function instead of manual >> size checking, but ofpacts->header here always points to the last >> pushed action, so the value that ofpacts_oversized() would check is >> always small. >> >> Adding a unit test for this, plus the extra test for too deep nesting. >> >> Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20254 >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> > Acked-by: Alin-Gabriel Serdean <aserdean@ovn.org> Thanks! Applied. Best regards, Ilya Maximets.
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 6fb3da507..ecf914eac 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -9326,6 +9326,7 @@ static char * OVS_WARN_UNUSED_RESULT ofpacts_parse__(char *str, const struct ofpact_parse_params *pp, bool allow_instructions, enum ofpact_type outer_action) { + uint32_t orig_size = pp->ofpacts->size; char *key, *value; bool drop = false; char *pos; @@ -9370,6 +9371,9 @@ ofpacts_parse__(char *str, const struct ofpact_parse_params *pp, if (error) { return error; } + if (pp->ofpacts->size - orig_size > UINT16_MAX) { + return xasprintf("input too big"); + } } if (drop && pp->ofpacts->size) { diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at index 59093c03c..ef4898abb 100644 --- a/tests/ofp-actions.at +++ b/tests/ofp-actions.at @@ -1144,4 +1144,10 @@ bad_action 'apply_actions' 'apply_actions is the default instruction' bad_action 'xyzzy' 'unknown action xyzzy' bad_action 'drop,3' '"drop" must not be accompanied by any other action or instruction' +# Too many actions +writes=$(printf 'write_actions(%.0s' $(seq 100)) +bad_action "${writes}" 'Action nested too deeply' +outputs=$(printf '1,%.0s' $(seq 4096)) +bad_action "${outputs}" 'input too big' + AT_CLEANUP
Not a very important fix, but fuzzer times out trying to test parsing of a huge number of actions. Fixing that by reporting an error as soon as ofpacts oversized. It would be great to use ofpacts_oversized() function instead of manual size checking, but ofpacts->header here always points to the last pushed action, so the value that ofpacts_oversized() would check is always small. Adding a unit test for this, plus the extra test for too deep nesting. Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20254 Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/ofp-actions.c | 4 ++++ tests/ofp-actions.at | 6 ++++++ 2 files changed, 10 insertions(+)