[PATCH] win32: ensure len does not cause any overreads

[PATCH] win32: ensure len does not cause any overreads

[Rose via GitGitGadget]

From: Seija Kijin <doremylover123@gmail.com>

Check to make sure len is always less than MAX_PATH,
otherwise an overread will occur, which is
undefined behavior.

Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
    win32: ensure len does not cause any overreads
    
    Check to make sure len is always less than MAX_PATH, otherwise an
    overread will occur, which is undefined behavior.
    
    Signed-off-by: Seija Kijin doremylover123@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1404%2FAtariDreams%2Foverread-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AtariDreams/overread-v1
Pull-Request: https://github.com/git/git/pull/1404

 compat/win32/dirent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c
index 52420ec7d4d..0c1bdccdd58 100644
--- a/compat/win32/dirent.c
+++ b/compat/win32/dirent.c
@@ -27,7 +27,7 @@ DIR *opendir(const char *name)
 	DIR *dir;
 
 	/* convert name to UTF-16 and check length < MAX_PATH */
-	if ((len = xutftowcs_path(pattern, name)) < 0)
+	if ((len = xutftowcs_path(pattern, name)) < 0 || len > MAX_PATH)
 		return NULL;
 
 	/* append optional '/' and wildcard '*' */

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
-- 
gitgitgadget

Re: [PATCH] win32: ensure len does not cause any overreads

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 19 2022, Rose via GitGitGadget wrote:

> From: Seija Kijin <doremylover123@gmail.com>
>
> Check to make sure len is always less than MAX_PATH,
> otherwise an overread will occur, which is
> undefined behavior.
>
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
>     win32: ensure len does not cause any overreads
>     
>     Check to make sure len is always less than MAX_PATH, otherwise an
>     overread will occur, which is undefined behavior.
>     
>     Signed-off-by: Seija Kijin doremylover123@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1404%2FAtariDreams%2Foverread-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AtariDreams/overread-v1
> Pull-Request: https://github.com/git/git/pull/1404
>
>  compat/win32/dirent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c
> index 52420ec7d4d..0c1bdccdd58 100644
> --- a/compat/win32/dirent.c
> +++ b/compat/win32/dirent.c
> @@ -27,7 +27,7 @@ DIR *opendir(const char *name)
>  	DIR *dir;
>  
>  	/* convert name to UTF-16 and check length < MAX_PATH */
> -	if ((len = xutftowcs_path(pattern, name)) < 0)
> +	if ((len = xutftowcs_path(pattern, name)) < 0 || len > MAX_PATH)

We tend to avoid assignments in "if", I think before this change it
could have passed, but now that we have a more complex expression it's
worth splitting it out. So, we can just move it up to when "int" is declared:
	
	diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c
	index 52420ec7d4d..bf371cc9714 100644
	--- a/compat/win32/dirent.c
	+++ b/compat/win32/dirent.c
	@@ -23,11 +23,11 @@ DIR *opendir(const char *name)
	 	wchar_t pattern[MAX_PATH + 2]; /* + 2 for '/' '*' */
	 	WIN32_FIND_DATAW fdata;
	 	HANDLE h;
	-	int len;
	+	int len = xutftowcs_path(pattern, name);
	 	DIR *dir;
	 
	 	/* convert name to UTF-16 and check length < MAX_PATH */
	-	if ((len = xutftowcs_path(pattern, name)) < 0)
	+	if (len < 0 || len > MAX_PATH)
	 		return NULL;
	 
	 	/* append optional '/' and wildcard '*' */

But that leaves the question of whether this was just omitted from
0217569bb2d (Win32: Unicode file name support (dirent), 2012-01-14) by
mistake?

The comment above the code you're tweaking says we're checking that
"length < MAX_PATH", but as we can see 0217569bb2d dropped that
condition.

So, was that a bug? And if so why is your check for MAX_PATH different
than its check?

Shouldn't yours be (as it did):

	if (len + 2 >= MAX_PATH) {
		errno = ENAMETOOLONG;
		return NULL;
	}

?

Perhaps not, but the commit message should discuss it, i.e. why is the
MAX_PATH check now subtly different than the pre-0217569bb2d one.q

Re: [PATCH] win32: ensure len does not cause any overreads

[Phillip Wood]

On 19/12/2022 17:17, Rose via GitGitGadget wrote:
> From: Seija Kijin <doremylover123@gmail.com>
> 
> Check to make sure len is always less than MAX_PATH,
> otherwise an overread will occur, which is
> undefined behavior.
> 
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
>      win32: ensure len does not cause any overreads
>      
>      Check to make sure len is always less than MAX_PATH, otherwise an
>      overread will occur, which is undefined behavior.
>      
>      Signed-off-by: Seija Kijin doremylover123@gmail.com
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1404%2FAtariDreams%2Foverread-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AtariDreams/overread-v1
> Pull-Request: https://github.com/git/git/pull/1404
> 
>   compat/win32/dirent.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c
> index 52420ec7d4d..0c1bdccdd58 100644
> --- a/compat/win32/dirent.c
> +++ b/compat/win32/dirent.c
> @@ -27,7 +27,7 @@ DIR *opendir(const char *name)
>   	DIR *dir;
>   
>   	/* convert name to UTF-16 and check length < MAX_PATH */
> -	if ((len = xutftowcs_path(pattern, name)) < 0)
> +	if ((len = xutftowcs_path(pattern, name)) < 0 || len > MAX_PATH)

The documentation for xutftowcs_path() says

/**
  * Simplified file system specific variant of xutftowcsn, assumes output
  * buffer size is MAX_PATH wide chars and input string is \0-terminated,
  * fails with ENAMETOOLONG if input string is too long.
  */

Looking at the implementation it seems it does check the length so I 
don't think we need this change. I haven't looked into why 0217569bb2d 
(Win32: Unicode file name support (dirent), 2012-01-14) changed the 
length check from "len + 2 >= MAX_PATH" though.

Best Wishes

Phillip

>   		return NULL;
>   
>   	/* append optional '/' and wildcard '*' */
> 
> base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7