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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 19, 2022, 18:19 UTC
Message-ID: <221219.86v8m7xncc.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1404.git.git.1671470222521.gitgitgadget@gmail.com>

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

Show 32 quoted lines

> 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