* [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it. @ 2015-03-02 19:58 Alvaro Neira Ayuso 2015-03-02 23:35 ` Pablo Neira Ayuso 2015-03-05 20:52 ` Pablo Neira Ayuso 0 siblings, 2 replies; 7+ messages in thread From: Alvaro Neira Ayuso @ 2015-03-02 19:58 UTC (permalink / raw) To: netfilter-devel If the root node name is not correctly initialized, we have a crash. Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com> --- src/mxml.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mxml.c b/src/mxml.c index 0001ba0..b68f86f 100644 --- a/src/mxml.c +++ b/src/mxml.c @@ -43,7 +43,8 @@ mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename, goto err; } - if (strcmp(tree->value.opaque, treename) == 0) + if (tree->value.opaque != NULL && + strcmp(tree->value.opaque, treename) == 0) return tree; err->error = NFT_PARSE_EMISSINGNODE; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it. 2015-03-02 19:58 [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it Alvaro Neira Ayuso @ 2015-03-02 23:35 ` Pablo Neira Ayuso 2015-03-03 13:45 ` Álvaro Neira Ayuso 2015-03-05 20:52 ` Pablo Neira Ayuso 1 sibling, 1 reply; 7+ messages in thread From: Pablo Neira Ayuso @ 2015-03-02 23:35 UTC (permalink / raw) To: Alvaro Neira Ayuso; +Cc: netfilter-devel On Mon, Mar 02, 2015 at 08:58:38PM +0100, Alvaro Neira Ayuso wrote: > If the root node name is not correctly initialized, we have a crash. Under what circunstances may tree->value.opaque be NULL? > --- > src/mxml.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/mxml.c b/src/mxml.c > index 0001ba0..b68f86f 100644 > --- a/src/mxml.c > +++ b/src/mxml.c > @@ -43,7 +43,8 @@ mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename, > goto err; > } > > - if (strcmp(tree->value.opaque, treename) == 0) > + if (tree->value.opaque != NULL && > + strcmp(tree->value.opaque, treename) == 0) > return tree; > > err->error = NFT_PARSE_EMISSINGNODE; > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it. 2015-03-02 23:35 ` Pablo Neira Ayuso @ 2015-03-03 13:45 ` Álvaro Neira Ayuso 2015-03-04 19:03 ` Pablo Neira Ayuso 0 siblings, 1 reply; 7+ messages in thread From: Álvaro Neira Ayuso @ 2015-03-03 13:45 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel El 03/03/15 a las 00:35, Pablo Neira Ayuso escribió: > On Mon, Mar 02, 2015 at 08:58:38PM +0100, Alvaro Neira Ayuso wrote: >> If the root node name is not correctly initialized, we have a crash. > > Under what circunstances may tree->value.opaque be NULL? The function mxmlLoadFile doesn't work correctly. With my patch to import ruleset in json and xml. I have seen that if we use a ruleset in json and use the command nft import xml. The function mxmlLoadFile returns a tree without any information inside. Therefore, when we try to verify that the name of the first node is the same of the parameter, we have a crash. We have two ways to solve it. Test if the node is NULL and: 1- show an error that the node is not found. 2- show an error that the input is not correctly. I have thought that the best is the first. But I can change it. > >> --- >> src/mxml.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/mxml.c b/src/mxml.c >> index 0001ba0..b68f86f 100644 >> --- a/src/mxml.c >> +++ b/src/mxml.c >> @@ -43,7 +43,8 @@ mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename, >> goto err; >> } >> >> - if (strcmp(tree->value.opaque, treename) == 0) >> + if (tree->value.opaque != NULL && >> + strcmp(tree->value.opaque, treename) == 0) >> return tree; >> >> err->error = NFT_PARSE_EMISSINGNODE; >> -- >> 1.7.10.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it. 2015-03-03 13:45 ` Álvaro Neira Ayuso @ 2015-03-04 19:03 ` Pablo Neira Ayuso 2015-03-04 19:20 ` Pablo Neira Ayuso 0 siblings, 1 reply; 7+ messages in thread From: Pablo Neira Ayuso @ 2015-03-04 19:03 UTC (permalink / raw) To: Álvaro Neira Ayuso; +Cc: netfilter-devel On Tue, Mar 03, 2015 at 02:45:34PM +0100, Álvaro Neira Ayuso wrote: > El 03/03/15 a las 00:35, Pablo Neira Ayuso escribió: > >On Mon, Mar 02, 2015 at 08:58:38PM +0100, Alvaro Neira Ayuso wrote: > >>If the root node name is not correctly initialized, we have a crash. > > > >Under what circunstances may tree->value.opaque be NULL? > > The function mxmlLoadFile doesn't work correctly. With my patch to > import ruleset in json and xml. I have seen that if we use a ruleset > in json and use the command nft import xml. OK, so this bug can be triggered by malformed input. > The function mxmlLoadFile returns a tree without any information > inside. Therefore, when we try to verify that the name of the first > node is the same of the parameter, we have a crash. > > We have two ways to solve it. Test if the node is NULL and: > 1- show an error that the node is not found. > 2- show an error that the input is not correctly. > > I have thought that the best is the first. But I can change it. I think we have to make robust checks on the input, if what we get is not "sane" then it's not worth going further. So I'd suggest you modify this lines a bit below: if (tree->value.opaque == NULL) { err->error = NFT_PARSE_EBADINPUT; goto err; } to look like: if (tree == NULL || tree->value.opaque == NULL) { err->error = NFT_PARSE_EBADINPUT; goto err; } > >>--- > >> src/mxml.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >>diff --git a/src/mxml.c b/src/mxml.c > >>index 0001ba0..b68f86f 100644 > >>--- a/src/mxml.c > >>+++ b/src/mxml.c > >>@@ -43,7 +43,8 @@ mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename, > >> goto err; > >> } > >> > >>- if (strcmp(tree->value.opaque, treename) == 0) > >>+ if (tree->value.opaque != NULL && > >>+ strcmp(tree->value.opaque, treename) == 0) > >> return tree; > >> > >> err->error = NFT_PARSE_EMISSINGNODE; > >>-- > >>1.7.10.4de -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it. 2015-03-04 19:03 ` Pablo Neira Ayuso @ 2015-03-04 19:20 ` Pablo Neira Ayuso 2015-03-04 20:25 ` Álvaro Neira Ayuso 0 siblings, 1 reply; 7+ messages in thread From: Pablo Neira Ayuso @ 2015-03-04 19:20 UTC (permalink / raw) To: Álvaro Neira Ayuso; +Cc: netfilter-devel On Wed, Mar 04, 2015 at 08:03:18PM +0100, Pablo Neira Ayuso wrote: > On Tue, Mar 03, 2015 at 02:45:34PM +0100, Álvaro Neira Ayuso wrote: > > El 03/03/15 a las 00:35, Pablo Neira Ayuso escribió: > > >On Mon, Mar 02, 2015 at 08:58:38PM +0100, Alvaro Neira Ayuso wrote: > > >>If the root node name is not correctly initialized, we have a crash. > > > > > >Under what circunstances may tree->value.opaque be NULL? > > > > The function mxmlLoadFile doesn't work correctly. With my patch to > > import ruleset in json and xml. I have seen that if we use a ruleset > > in json and use the command nft import xml. > > OK, so this bug can be triggered by malformed input. > > > The function mxmlLoadFile returns a tree without any information > > inside. Therefore, when we try to verify that the name of the first > > node is the same of the parameter, we have a crash. > > > > We have two ways to solve it. Test if the node is NULL and: > > 1- show an error that the node is not found. > > 2- show an error that the input is not correctly. > > > > I have thought that the best is the first. But I can change it. > > I think we have to make robust checks on the input, if what we get is > not "sane" then it's not worth going further. > > So I'd suggest you modify this lines a bit below: > > if (tree->value.opaque == NULL) { > err->error = NFT_PARSE_EBADINPUT; > goto err; > } > > to look like: > > if (tree == NULL || tree->value.opaque == NULL) { > err->error = NFT_PARSE_EBADINPUT; > goto err; > } > > > >>--- > > >> src/mxml.c | 3 ++- > > >> 1 file changed, 2 insertions(+), 1 deletion(-) > > >> > > >>diff --git a/src/mxml.c b/src/mxml.c > > >>index 0001ba0..b68f86f 100644 > > >>--- a/src/mxml.c > > >>+++ b/src/mxml.c > > >>@@ -43,7 +43,8 @@ mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename, > > >> goto err; > > >> } > > >> > > >>- if (strcmp(tree->value.opaque, treename) == 0) > > >>+ if (tree->value.opaque != NULL && > > >>+ strcmp(tree->value.opaque, treename) == 0) > > >> return tree; > > >> > > >> err->error = NFT_PARSE_EMISSINGNODE; Actually your patch looks correct because you're returning "missing node" which is triggering the crash, right? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it. 2015-03-04 19:20 ` Pablo Neira Ayuso @ 2015-03-04 20:25 ` Álvaro Neira Ayuso 0 siblings, 0 replies; 7+ messages in thread From: Álvaro Neira Ayuso @ 2015-03-04 20:25 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel El 04/03/15 a las 20:20, Pablo Neira Ayuso escribió: > On Wed, Mar 04, 2015 at 08:03:18PM +0100, Pablo Neira Ayuso wrote: >> On Tue, Mar 03, 2015 at 02:45:34PM +0100, Álvaro Neira Ayuso wrote: >>> El 03/03/15 a las 00:35, Pablo Neira Ayuso escribió: >>>> On Mon, Mar 02, 2015 at 08:58:38PM +0100, Alvaro Neira Ayuso wrote: >>>>> If the root node name is not correctly initialized, we have a crash. >>>> >>>> Under what circunstances may tree->value.opaque be NULL? >>> >>> The function mxmlLoadFile doesn't work correctly. With my patch to >>> import ruleset in json and xml. I have seen that if we use a ruleset >>> in json and use the command nft import xml. >> >> OK, so this bug can be triggered by malformed input. >> >>> The function mxmlLoadFile returns a tree without any information >>> inside. Therefore, when we try to verify that the name of the first >>> node is the same of the parameter, we have a crash. >>> >>> We have two ways to solve it. Test if the node is NULL and: >>> 1- show an error that the node is not found. >>> 2- show an error that the input is not correctly. >>> >>> I have thought that the best is the first. But I can change it. >> >> I think we have to make robust checks on the input, if what we get is >> not "sane" then it's not worth going further. >> >> So I'd suggest you modify this lines a bit below: >> >> if (tree->value.opaque == NULL) { >> err->error = NFT_PARSE_EBADINPUT; >> goto err; >> } >> >> to look like: >> >> if (tree == NULL || tree->value.opaque == NULL) { >> err->error = NFT_PARSE_EBADINPUT; >> goto err; >> } >> >>>>> --- >>>>> src/mxml.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/src/mxml.c b/src/mxml.c >>>>> index 0001ba0..b68f86f 100644 >>>>> --- a/src/mxml.c >>>>> +++ b/src/mxml.c >>>>> @@ -43,7 +43,8 @@ mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename, >>>>> goto err; >>>>> } >>>>> >>>>> - if (strcmp(tree->value.opaque, treename) == 0) >>>>> + if (tree->value.opaque != NULL && >>>>> + strcmp(tree->value.opaque, treename) == 0) >>>>> return tree; >>>>> >>>>> err->error = NFT_PARSE_EMISSINGNODE; > > Actually your patch looks correct because you're returning "missing > node" which is triggering the crash, right? Yes, the first node of the tree is missing and we have a crash. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it. 2015-03-02 19:58 [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it Alvaro Neira Ayuso 2015-03-02 23:35 ` Pablo Neira Ayuso @ 2015-03-05 20:52 ` Pablo Neira Ayuso 1 sibling, 0 replies; 7+ messages in thread From: Pablo Neira Ayuso @ 2015-03-05 20:52 UTC (permalink / raw) To: Alvaro Neira Ayuso; +Cc: netfilter-devel On Mon, Mar 02, 2015 at 08:58:38PM +0100, Alvaro Neira Ayuso wrote: > If the root node name is not correctly initialized, we have a crash. Applied, thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-03-05 20:49 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-02 19:58 [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it Alvaro Neira Ayuso 2015-03-02 23:35 ` Pablo Neira Ayuso 2015-03-03 13:45 ` Álvaro Neira Ayuso 2015-03-04 19:03 ` Pablo Neira Ayuso 2015-03-04 19:20 ` Pablo Neira Ayuso 2015-03-04 20:25 ` Álvaro Neira Ayuso 2015-03-05 20:52 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).