mxml: test if the root node name is initialized before to compare it.

PATCH Applied Alvaro Neira Ayuso 2 participants 7 messages Mar 2, 2015 – Mar 5, 2015 netfilter

lore

Share & tools

lore

Alvaro Neira Ayuso

author

2015-03-02 19:58 UTC #0 lore raw

PATCH 1/1 mxml: test if the root node name is initialized before to compare it.

If the root node name is not correctly initialized, we have a crash.

Signed-off-by Alvaro Neira Ayuso <[email protected]>

---
 src/mxml.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

src/mxml.c +2 -1

index 0001ba0..b68f86f 100644
@@ -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;

Signature · 1 line

1.7.10.4

Pablo Neira Ayuso

maintainer

2015-03-02 23:31 UTC #1 lore raw

Quoted 1 line · Alvaro Neira Ayuso

If the root node name is not correctly initialized, we have a crash.

Under what circunstances may tree->value.opaque be NULL?

Quoted 25 lines

---
 src/mxml.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

src/mxml.c +2 -1

index 0001ba0..b68f86f 100644
@@ -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;

Signature · 6 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Álvaro Neira Ayuso

author

2015-03-03 13:45 UTC #2 lore raw

Quoted 4 lines · Pablo Neira Ayuso

Quoted 1 line · Alvaro Neira Ayuso
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.

Quoted 18 lines

---
  src/mxml.c |    3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

src/mxml.c +2 -1

index 0001ba0..b68f86f 100644
@@ -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;

Quoted 8 lines

Quoted 8 lines
src/mxml.c +0 -1
  	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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

maintainer

2015-03-04 18:59 UTC #3 lore raw

Quoted 9 lines · Álvaro Neira Ayuso

Quoted 4 lines · Pablo Neira Ayuso
Quoted 1 line · Alvaro Neira Ayuso
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.

Quoted 9 lines

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;
        }

Quoted 12 lines

---
 src/mxml.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

src/mxml.c +0 -0

index 0001ba0..b68f86f 100644
@@ -43,7 +43,8 @@ mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename,
 		goto err;
 	}

Quoted 8 lines

src/mxml.c +2 -2

-	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

maintainer

2015-03-04 19:17 UTC #4 lore raw

Quoted 52 lines · Pablo Neira Ayuso

Quoted 9 lines · Álvaro Neira Ayuso
Quoted 4 lines · Pablo Neira Ayuso
Quoted 1 line · Alvaro Neira Ayuso
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.
Quoted 9 lines
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;
        }
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
---
 src/mxml.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
src/mxml.c +0 -0
index 0001ba0..b68f86f 100644
@@ -43,7 +43,8 @@ mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename,
 		goto err;
 	}
 

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
src/mxml.c mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename, · line 50
Quoted 6 lines
src/mxml.c +2 -1
-	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Álvaro Neira Ayuso

author

2015-03-04 20:25 UTC #5 lore raw

Quoted 58 lines · Pablo Neira Ayuso

Quoted 57 lines · Pablo Neira Ayuso
Quoted 9 lines · Álvaro Neira Ayuso
Quoted 4 lines · Pablo Neira Ayuso
Quoted 1 line · Alvaro Neira Ayuso
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.
Quoted 9 lines
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;
         }
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
---
  src/mxml.c |    3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
src/mxml.c +2 -1
index 0001ba0..b68f86f 100644
@@ -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;
 

Quoted 4 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
src/mxml.c mxml_node_t *nft_mxml_build_tree(const void *data, const char *treename, · line 50
Quoted 1 line
src/mxml.c +0 -0
  	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

maintainer Applied

2015-03-05 20:49 UTC #6 lore raw

In reply to Alvaro Neira Ayuso

Quoted 1 line · Alvaro Neira Ayuso

If the root node name is not correctly initialized, we have a crash.

Applied, thanks.