[
https://issues.apache.org/jira/browse/THRIFT-5932?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Pengpeng Hou updated THRIFT-5932:
---------------------------------
Description:
Hi,
I rechecked current upstream head and this Windows fallback still looks unsafe.
In `compiler/cpp/src/thrift/main.cc`, `saferealpath()` uses
`GetFullPathNameA()` on Windows and then falls back to copying the original
path into `resolved_path` with:
if (len == 0 || len > MAX_PATH - 1)
{ strcpy(resolved_path, path); }
else
{ strcpy(resolved_path, buf); }
The fallback does not know the actual size of `resolved_path`, and the callers
typically pass a fixed stack buffer such as `THRIFT_PATH_MAX` in the compiler
front end. If the input `.thrift` path is too long, or if `GetFullPathNameA()`
fails, this code copies the raw path into the caller buffer without any local
bound check.
I rechecked the current code and did not find any capacity-aware copy or
rejection of overlong paths before the `strcpy()`.
*How to reproduce*
1. Run the Thrift C++ compiler on Windows.
2. Pass a very long `.thrift` file path so that `GetFullPathNameA()` returns 0
or a length greater than `MAX_PATH - 1`.
3. The fallback path calls `strcpy(resolved_path, path)`.
4. If the caller's `resolved_path` buffer is the usual fixed stack buffer, the
copy can overflow it.
was:
Hi,
I rechecked current upstream head and this Windows fallback still looks unsafe.
In `compiler/cpp/src/thrift/main.cc`, `saferealpath()` uses
`GetFullPathNameA()` on Windows and then falls back to copying the original
path into `resolved_path` with:
if (len == 0 || len > MAX_PATH - 1) {
strcpy(resolved_path, path);
} else {
strcpy(resolved_path, buf);
}
The fallback does not know the actual size of `resolved_path`, and the callers
typically pass a fixed stack buffer such as `THRIFT_PATH_MAX` in the compiler
front end. If the input `.thrift` path is too long, or if `GetFullPathNameA()`
fails, this code copies the raw path into the caller buffer without any local
bound check.
I rechecked the current code and did not find any capacity-aware copy or
rejection of overlong paths before the `strcpy()`.
*How to reproduce*
1. Run the Thrift C++ compiler on Windows.
2. Pass a very long `.thrift` file path so that `GetFullPathNameA()` returns 0
or a length greater than `MAX_PATH - 1`.
3. The fallback path calls `strcpy(resolved_path, path)`.
4. If the caller's `resolved_path` buffer is the usual fixed stack buffer, the
copy can overflow it.
This is easiest to observe with ASan or a debugger, but the bug is in the
fallback logic itself.
> saferealpath() on Windows can strcpy() an arbitrarily long path into caller
> storage
> -----------------------------------------------------------------------------------
>
> Key: THRIFT-5932
> URL: https://issues.apache.org/jira/browse/THRIFT-5932
> Project: Thrift
> Issue Type: Bug
> Reporter: Pengpeng Hou
> Priority: Major
>
> Hi,
> I rechecked current upstream head and this Windows fallback still looks
> unsafe.
> In `compiler/cpp/src/thrift/main.cc`, `saferealpath()` uses
> `GetFullPathNameA()` on Windows and then falls back to copying the original
> path into `resolved_path` with:
> if (len == 0 || len > MAX_PATH - 1)
> { strcpy(resolved_path, path); }
> else
> { strcpy(resolved_path, buf); }
> The fallback does not know the actual size of `resolved_path`, and the
> callers typically pass a fixed stack buffer such as `THRIFT_PATH_MAX` in the
> compiler front end. If the input `.thrift` path is too long, or if
> `GetFullPathNameA()` fails, this code copies the raw path into the caller
> buffer without any local bound check.
> I rechecked the current code and did not find any capacity-aware copy or
> rejection of overlong paths before the `strcpy()`.
>
> *How to reproduce*
> 1. Run the Thrift C++ compiler on Windows.
> 2. Pass a very long `.thrift` file path so that `GetFullPathNameA()` returns
> 0 or a length greater than `MAX_PATH - 1`.
> 3. The fallback path calls `strcpy(resolved_path, path)`.
> 4. If the caller's `resolved_path` buffer is the usual fixed stack buffer,
> the copy can overflow it.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
