| Message ID | 20191016225818.23842-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [libnftnl] obj/ct_timeout: Fix NFTA_CT_TIMEOUT_DATA parser | expand |
On Thu, Oct 17, 2019 at 12:58:18AM +0200, Phil Sutter wrote: > This is a necessary follow-up on commit 00b144bc9d093 ("obj/ct_timeout: > Avoid array overrun in timeout_parse_attr_data()") which fixed array out > of bounds access but missed the logic behind it: > > The nested attribute type values are incremented by one when being > transferred between kernel and userspace, the zero type value is > reserved for "unspecified". > > Kernel uses CTA_TIMEOUT_* symbols for that, libnftnl simply mangles the > type values in nftnl_obj_ct_timeout_build(). > > Return path was broken as it overstepped its nlattr array but apart from > that worked: Type values were decremented by one in > timeout_parse_attr_data(). > > This patch moves the type value mangling into > parse_timeout_attr_policy_cb() (which still overstepped nlattr array). > Consequently, when copying values from nlattr array into ct timeout > object in timeout_parse_attr_data(), loop is adjusted to start at index > 0 and the type value decrement is dropped there. > > Fixes: 0adceeab1597a ("src: add ct timeout support") > Signed-off-by: Phil Sutter <phil@nwl.cc> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c index a09e25ae5d44f..2662cac69438d 100644 --- a/src/obj/ct_timeout.c +++ b/src/obj/ct_timeout.c @@ -108,10 +108,10 @@ parse_timeout_attr_policy_cb(const struct nlattr *attr, void *data) if (mnl_attr_type_valid(attr, data_cb->nlattr_max) < 0) return MNL_CB_OK; - if (type <= data_cb->nlattr_max) { + if (type > 0 && type <= data_cb->nlattr_max) { if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0) abi_breakage(); - tb[type] = attr; + tb[type - 1] = attr; } return MNL_CB_OK; } @@ -134,9 +134,9 @@ timeout_parse_attr_data(struct nftnl_obj *e, if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0) return -1; - for (i = 1; i < array_size(tb); i++) { + for (i = 0; i < array_size(tb); i++) { if (tb[i]) { - nftnl_timeout_policy_attr_set_u32(e, i-1, + nftnl_timeout_policy_attr_set_u32(e, i, ntohl(mnl_attr_get_u32(tb[i]))); } }
This is a necessary follow-up on commit 00b144bc9d093 ("obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()") which fixed array out of bounds access but missed the logic behind it: The nested attribute type values are incremented by one when being transferred between kernel and userspace, the zero type value is reserved for "unspecified". Kernel uses CTA_TIMEOUT_* symbols for that, libnftnl simply mangles the type values in nftnl_obj_ct_timeout_build(). Return path was broken as it overstepped its nlattr array but apart from that worked: Type values were decremented by one in timeout_parse_attr_data(). This patch moves the type value mangling into parse_timeout_attr_policy_cb() (which still overstepped nlattr array). Consequently, when copying values from nlattr array into ct timeout object in timeout_parse_attr_data(), loop is adjusted to start at index 0 and the type value decrement is dropped there. Fixes: 0adceeab1597a ("src: add ct timeout support") Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/obj/ct_timeout.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)