mbox series

[v2,0/2] Prevent potential write out of bounds

Message ID 20230927020221.85292-1-joao@overdrivepizza.com
Headers
Series Prevent potential write out of bounds |

Message

Joao Moreira Sept. 27, 2023, 2:02 a.m. UTC 
  From: Joao Moreira <joao.moreira@intel.com>

The function flow_rule_alloc in net/core/flow_offload.c [2] gets an
unsigned int num_actions (line 10) and later traverses the actions in
the rule (line 24) setting hw.stats to FLOW_ACTION_HW_STATS_DONT_CARE.

Within the same file, the loop in the line 24 compares a signed int
(i) to an unsigned int (num_actions), and then uses i as an array
index. If an integer overflow happens, then the array within the loop
is wrongly indexed, causing a write out of bounds.

After checking with maintainers, it seems that the front-end caps the
maximum value of num_action, thus it is not possible to reach the given
write out of bounds, yet, still, to prevent disasters it is better to
fix the signedness here.

Similarly, also it is also good to ensure that an overflow won't happen
in net/netfilter/nf_tables_offload.c's function nft_flow_rule_create by
making the variable unsigned and ensuring that it returns an error if
its value reaches UINT_MAX.

This issue was observed by the commit author while reviewing a write-up
regarding a CVE within the same subsystem [1].

1 - https://nickgregory.me/post/2022/03/12/cve-2022-25636/

Tks,

Joao Moreira (2):
  Make loop indexes unsigned
  Make num_actions unsigned

 net/core/flow_offload.c           | 4 ++--
 net/netfilter/nf_tables_offload.c | 6 +++++-
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Pablo Neira Ayuso Sept. 27, 2023, 8:24 a.m. UTC | #1 
On Tue, Sep 26, 2023 at 07:02:19PM -0700, joao@overdrivepizza.com wrote:
> From: Joao Moreira <joao.moreira@intel.com>
> 
> The function flow_rule_alloc in net/core/flow_offload.c [2] gets an
> unsigned int num_actions (line 10) and later traverses the actions in
> the rule (line 24) setting hw.stats to FLOW_ACTION_HW_STATS_DONT_CARE.
> 
> Within the same file, the loop in the line 24 compares a signed int
> (i) to an unsigned int (num_actions), and then uses i as an array
> index. If an integer overflow happens, then the array within the loop
> is wrongly indexed, causing a write out of bounds.
> 
> After checking with maintainers, it seems that the front-end caps the
> maximum value of num_action, thus it is not possible to reach the given
> write out of bounds, yet, still, to prevent disasters it is better to
> fix the signedness here.
> 
> Similarly, also it is also good to ensure that an overflow won't happen
> in net/netfilter/nf_tables_offload.c's function nft_flow_rule_create by
> making the variable unsigned and ensuring that it returns an error if
> its value reaches UINT_MAX.
> 
> This issue was observed by the commit author while reviewing a write-up
> regarding a CVE within the same subsystem [1].

I keep spinning around this, this is not really an issue.

No frontend uses this amount of actions.

Probably cap this to uint16_t because 2^16 actions is more than
sufficient by now.
Pablo Neira Ayuso Sept. 27, 2023, 8:35 a.m. UTC | #2 
On Wed, Sep 27, 2023 at 10:25:03AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Sep 26, 2023 at 07:02:19PM -0700, joao@overdrivepizza.com wrote:
> > From: Joao Moreira <joao.moreira@intel.com>
> > 
> > The function flow_rule_alloc in net/core/flow_offload.c [2] gets an
> > unsigned int num_actions (line 10) and later traverses the actions in
> > the rule (line 24) setting hw.stats to FLOW_ACTION_HW_STATS_DONT_CARE.
> > 
> > Within the same file, the loop in the line 24 compares a signed int
> > (i) to an unsigned int (num_actions), and then uses i as an array
> > index. If an integer overflow happens, then the array within the loop
> > is wrongly indexed, causing a write out of bounds.
> > 
> > After checking with maintainers, it seems that the front-end caps the
> > maximum value of num_action, thus it is not possible to reach the given
> > write out of bounds, yet, still, to prevent disasters it is better to
> > fix the signedness here.
> > 
> > Similarly, also it is also good to ensure that an overflow won't happen
> > in net/netfilter/nf_tables_offload.c's function nft_flow_rule_create by
> > making the variable unsigned and ensuring that it returns an error if
> > its value reaches UINT_MAX.
> > 
> > This issue was observed by the commit author while reviewing a write-up
> > regarding a CVE within the same subsystem [1].
> 
> I keep spinning around this, this is not really an issue.
> 
> No frontend uses this amount of actions.
> 
> Probably cap this to uint16_t because 2^16 actions is more than
> sufficient by now.

Actually, even 2^8 actions is more than enough by now.