[libnftnl PATCH] mxml: test if the root node name is initialized before to compare it.

[libnftnl PATCH] mxml: test if the root node name is initialized before to compare it.

[Alvaro Neira Ayuso]

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

Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it.

[Pablo Neira Ayuso]

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

Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it.

[Álvaro Neira Ayuso]

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

Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it.

[Pablo Neira Ayuso]

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

Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it.

[Pablo Neira Ayuso]

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

Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it.

[Álvaro Neira Ayuso]

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

Re: [libnftnl PATCH] mxml: test if the root node name is initialized before to compare it.

[Pablo Neira Ayuso]

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.