[PATCH] tree-walk: don't parse incorrect entries

[PATCH] tree-walk: don't parse incorrect entries

[Martin Koegler]

The current code can access memory outside of the tree
buffer in the case of malformed tree entries.

This patch prevent this by:
* The rest of the buffer must be at least 24 bytes
  (at least 1 byte mode, 1 blank, at least one byte path name,
   1 NUL, 20 bytes sha1).
* Check that the last NUL (21 bytes before the end) is present.
  This ensurse, that strlen and get_mode stay within the buffer.
* The mode may not be empty. We have only to reject a blank at the begin,
  as the rest is handled by if (c < '0' || c > '7').
* The blank is ensured by get_mode.
* The path must contain at least one character.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 tree-walk.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index 8d4b673..10f21d7 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -7,6 +7,9 @@ static const char *get_mode(const char *str, unsigned int *modep)
 	unsigned char c;
 	unsigned int mode = 0;
 
+	if (*str == ' ')
+		return NULL;
+
 	while ((c = *str++) != ' ') {
 		if (c < '0' || c > '7')
 			return NULL;
@@ -16,13 +19,16 @@ static const char *get_mode(const char *str, unsigned int *modep)
 	return str;
 }
 
-static void decode_tree_entry(struct tree_desc *desc, const void *buf, unsigned long size)
+static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
 {
 	const char *path;
 	unsigned int mode, len;
 
+	if (size < 24 || buf[size - 21]) 
+		die("corrupt tree file");
+
 	path = get_mode(buf, &mode);
-	if (!path)
+	if (!path || !*path)
 		die("corrupt tree file");
 	len = strlen(path) + 1;
 
-- 
1.4.4.4

[PATCH] tree-walk: don't parse incorrect entries

[Martin Koegler]

The current code can access memory outside of the tree
buffer in the case of malformed tree entries.

This patch prevent this by:
* The rest of the buffer must be at least 24 bytes
  (at least 1 byte mode, 1 blank, at least one byte path name,
   1 zero, 20 bytes sha1).
* Check that the last zero (21 bytes before the end) is present.
  This ensurse, that strlen and get_mode stay within the buffer.
* The mode may not be empty. We have only to reject a blank at the begin,
  as the rest is handled by if (c < '0' || c > '7').
* The blank is ensured by get_mode.
* The start of the path may not be after the last zero (21 bytes before the end).

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 tree-walk.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index 8d4b673..bd88ec7 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -7,6 +7,9 @@ static const char *get_mode(const char *str, unsigned int *modep)
 	unsigned char c;
 	unsigned int mode = 0;
 
+	if (*str == ' ')
+		return NULL;
+
 	while ((c = *str++) != ' ') {
 		if (c < '0' || c > '7')
 			return NULL;
@@ -16,13 +19,17 @@ static const char *get_mode(const char *str, unsigned int *modep)
 	return str;
 }
 
-static void decode_tree_entry(struct tree_desc *desc, const void *buf, unsigned long size)
+static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
 {
 	const char *path;
 	unsigned int mode, len;
+	const char *end = buf + size - 21;
+
+	if (size < 24 || *end) 
+		die("corrupt tree file");
 
 	path = get_mode(buf, &mode);
-	if (!path)
+	if (!path || path > end)
 		die("corrupt tree file");
 	len = strlen(path) + 1;
 
-- 
1.4.4.4

Re: [PATCH] tree-walk: don't parse incorrect entries

[Junio C Hamano]

Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:

> The current code can access memory outside of the tree
> buffer in the case of malformed tree entries.
>
> This patch prevent this by:
> * The rest of the buffer must be at least 24 bytes
>   (at least 1 byte mode, 1 blank, at least one byte path name,
>    1 zero, 20 bytes sha1).

Good ("zero" in this context is better spelled as "NUL").

> * Check that the last zero (21 bytes before the end) is present.
>   This ensurse, that strlen and get_mode stay within the buffer.

Good (ditto).

> * The mode may not be empty. We have only to reject a blank at the begin,
>   as the rest is handled by if (c < '0' || c > '7').

I initially thought this was slightly iffy as tree objects were
written with padding for mode bits, but that was padding with
zero ('0') to the left and not with SP, so it is a good change,
I think (I am saying this primarily so that others can say "you
are wrong, there are valid old trees with SP there" to stop me).

> * The blank is ensured by get_mode.

Ok.

> * The start of the path may not be after the last zero (21 bytes before the end).

How can that be possible?

 - you know end points at NUL and buf < end;

 - get_mode() starts scanning from buf, stops at the first SP if
   returns a non NULL pointer; anything non octal digit before
   it sees that SP results in a NULL return;

 - the return value of get_mode() is the beginning of the path.

The second point above means when get_mode() scans buf, it would
never go beyond end which you already made sure is NUL (which is
not SP and not an octal digit).  If it hits end, you would get NULL
pointer back, wouldn't you?

Rejecting an empty path may be sensible (i.e. checking "!*path"
instead), though.

Re: [PATCH] tree-walk: don't parse incorrect entries

[Martin Koegler]

On Sat, Jan 05, 2008 at 12:50:28PM -0800, Junio C Hamano wrote:
> Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
> > * The start of the path may not be after the last zero (21 bytes before the end).
> 
> How can that be possible?
> 
>  - you know end points at NUL and buf < end;
> 
>  - get_mode() starts scanning from buf, stops at the first SP if
>    returns a non NULL pointer; anything non octal digit before
>    it sees that SP results in a NULL return;
> 
>  - the return value of get_mode() is the beginning of the path.
> 
> The second point above means when get_mode() scans buf, it would
> never go beyond end which you already made sure is NUL (which is
> not SP and not an octal digit).  If it hits end, you would get NULL
> pointer back, wouldn't you?

Yes, I agree with you.

> Rejecting an empty path may be sensible (i.e. checking "!*path"
> instead), though.

I sent a new patch with both changes.

mfg Martin Kögler