# [PATCH] win32: ensure len does not cause any overreads 4 messages from 2022-12-19 to 2024-12-18. Participants: Rose via GitGitGadget, Ævar Arnfjörð Bjarmason, Phillip Wood, AreaZR via GitGitGadget. Thread: https://gitlist.dev/t/58982 ## Rose via GitGitGadget, 2022-12-19 17:17 Subject: [PATCH] win32: ensure len does not cause any overreads Message-ID: URL: https://gitlist.dev/e/pull.1404.git.git.1671470222521.gitgitgadget%40gmail.com ``` From: Seija Kijin 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 --- 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 ``` ## Ævar Arnfjörð Bjarmason, 2022-12-19 18:19 Subject: Re: [PATCH] win32: ensure len does not cause any overreads Message-ID: <221219.86v8m7xncc.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/221219.86v8m7xncc.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Mon, Dec 19 2022, Rose via GitGitGadget wrote: > From: Seija Kijin > > 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 > --- > 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 ``` ## Phillip Wood, 2022-12-19 20:37 Subject: Re: [PATCH] win32: ensure len does not cause any overreads Message-ID: URL: https://gitlist.dev/e/dd47cbd0-35a6-1009-26e8-6a281224436d%40dunelm.org.uk In-Reply-To: ``` On 19/12/2022 17:17, Rose via GitGitGadget wrote: > From: Seija Kijin > > 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 > --- > 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 ``` ## AreaZR via GitGitGadget, 2024-12-18 00:29 Subject: [PATCH v2] win32: ensure len does not cause any overreads Message-ID: URL: https://gitlist.dev/e/pull.1404.v2.git.git.1734481790015.gitgitgadget%40gmail.com In-Reply-To: ``` From: Seija Kijin Check to make sure len is always two less than MAX_PATH, otherwise an overread will occur, which is undefined behavior. Signed-off-by: Seija Kijin --- 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%2FAreaZR%2Foverread-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AreaZR/overread-v2 Pull-Request: https://github.com/git/git/pull/1404 Range-diff vs v1: 1: f9ec5429d01 ! 1: dfc34fb4c1a win32: ensure len does not cause any overreads @@ Metadata ## Commit message ## win32: ensure len does not cause any overreads - Check to make sure len is always less than MAX_PATH, + Check to make sure len is always two less than MAX_PATH, otherwise an overread will occur, which is undefined behavior. @@ Commit message ## compat/win32/dirent.c ## @@ compat/win32/dirent.c: 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) + if ((len = xutftowcs_path(pattern, name)) < 0) return NULL; ++ if (len + 2 >= MAX_PATH) ++ return NULL; ++ /* append optional '/' and wildcard '*' */ + if (len && !is_dir_sep(pattern[len - 1])) + pattern[len++] = '/'; compat/win32/dirent.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c index 52420ec7d4d..fb63d1adbc5 100644 --- a/compat/win32/dirent.c +++ b/compat/win32/dirent.c @@ -30,6 +30,9 @@ DIR *opendir(const char *name) if ((len = xutftowcs_path(pattern, name)) < 0) return NULL; + if (len + 2 >= MAX_PATH) + return NULL; + /* append optional '/' and wildcard '*' */ if (len && !is_dir_sep(pattern[len - 1])) pattern[len++] = '/'; base-commit: 2ccc89b0c16c51561da90d21cfbb4b58cc877bf6 -- gitgitgadget ```