| Message ID | 20241127180103.15076-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [libnftnl,1/3] set: Fix for array overrun when setting NFTNL_SET_DESC_CONCAT | expand |
Hi Phil, On Wed, Nov 27, 2024 at 07:01:01PM +0100, Phil Sutter wrote: > Assuming max data_len of 16 * 4B and no zero bytes in 'data': > The while loop will increment field_count, use it as index for the > field_len array and afterwards make sure it hasn't increased to > NFT_REG32_COUNT. Thus a value of NFT_REG32_COUNT - 1 (= 15) will pass > the check, get incremented to 16 and used as index to the 16 fields long > array. > Use a less fancy for-loop to avoid the increment vs. check problem. for-loop is indeed better. Patch LGTM, thanks. > Fixes: 407f616ea5318 ("set: buffer overflow in NFTNL_SET_DESC_CONCAT setter") > Signed-off-by: Phil Sutter <phil@nwl.cc> Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > src/set.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/set.c b/src/set.c > index f127c19b7b8b8..5746397277c48 100644 > --- a/src/set.c > +++ b/src/set.c > @@ -185,8 +185,10 @@ int nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data, > return -1; > > memcpy(&s->desc.field_len, data, data_len); > - while (s->desc.field_len[++s->desc.field_count]) { > - if (s->desc.field_count >= NFT_REG32_COUNT) > + for (s->desc.field_count = 0; > + s->desc.field_count < NFT_REG32_COUNT; > + s->desc.field_count++) { > + if (!s->desc.field_len[s->desc.field_count]) > break; > } > break; > -- > 2.47.0 >
On Wed, Dec 04, 2024 at 03:30:56PM +0100, Pablo Neira Ayuso wrote: > Hi Phil, > > On Wed, Nov 27, 2024 at 07:01:01PM +0100, Phil Sutter wrote: > > Assuming max data_len of 16 * 4B and no zero bytes in 'data': > > The while loop will increment field_count, use it as index for the > > field_len array and afterwards make sure it hasn't increased to > > NFT_REG32_COUNT. Thus a value of NFT_REG32_COUNT - 1 (= 15) will pass > > the check, get incremented to 16 and used as index to the 16 fields long > > array. > > Use a less fancy for-loop to avoid the increment vs. check problem. > > for-loop is indeed better. > > Patch LGTM, thanks. > > > Fixes: 407f616ea5318 ("set: buffer overflow in NFTNL_SET_DESC_CONCAT setter") > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org> Series applied, thanks!
diff --git a/src/set.c b/src/set.c index f127c19b7b8b8..5746397277c48 100644 --- a/src/set.c +++ b/src/set.c @@ -185,8 +185,10 @@ int nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data, return -1; memcpy(&s->desc.field_len, data, data_len); - while (s->desc.field_len[++s->desc.field_count]) { - if (s->desc.field_count >= NFT_REG32_COUNT) + for (s->desc.field_count = 0; + s->desc.field_count < NFT_REG32_COUNT; + s->desc.field_count++) { + if (!s->desc.field_len[s->desc.field_count]) break; } break;
Assuming max data_len of 16 * 4B and no zero bytes in 'data': The while loop will increment field_count, use it as index for the field_len array and afterwards make sure it hasn't increased to NFT_REG32_COUNT. Thus a value of NFT_REG32_COUNT - 1 (= 15) will pass the check, get incremented to 16 and used as index to the 16 fields long array. Use a less fancy for-loop to avoid the increment vs. check problem. Fixes: 407f616ea5318 ("set: buffer overflow in NFTNL_SET_DESC_CONCAT setter") Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/set.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)