Skip to content

Commit e01a672

Browse files
benhillisBen Hillis
andauthored
Fix 4 code bugs: substr off-by-one, HANDLE* cast, TOCTOU GetLastError, sun_path overflow (#14297)
Bug 1 - LxssHttpProxy.cpp: IPv6 substr extraction used wrong length calculation. substr(openBracket+1, closeBracket-1) is incorrect when openBracket > 0; fixed to substr(openBracket+1, closeBracket-openBracket-1). Also fixed empty-address guard to check closeBracket (not closeBracket-1). Bug 2 - LxssUserSession.cpp: Two instances of reinterpret_cast<HANDLE*> in ScopedMultiRelay construction should be reinterpret_cast<HANDLE> (without the pointer). Other identical callsites in the same file already use the correct cast. Bug 3 - LxssUserSession.cpp: GetLastError() was called unconditionally after CreateFileW, even on success. A stale ERROR_SHARING_VIOLATION from a prior API call could cause a false throw. Fixed to only check GetLastError() when CreateFileW fails (!vhd). Bug 4 - plan9.cpp: sun_path bounds check used > instead of >= leaving no room for null terminator. Also added a post-split check to ensure the child name fits after splitting parent/child for long paths. Co-authored-by: Ben Hillis <benhill@ntdev.microsoft.com>
1 parent 55e04d5 commit e01a672

File tree

3 files changed

+16
-9
lines changed

3 files changed

+16
-9
lines changed

src/linux/init/plan9.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,9 @@ wil::unique_fd CreateUnixServerSocket(const char* path)
5151
}
5252
});
5353

54-
// Check if the path will fit in a sockaddr_un.
54+
// Check if the path will fit in a sockaddr_un (with room for null terminator).
5555
std::string_view pathView{path};
56-
if (pathView.length() > sizeof(sockaddr_un::sun_path))
56+
if (pathView.length() >= sizeof(sockaddr_un::sun_path))
5757
{
5858
// It won't, so split the parent path and child name.
5959
auto index = pathView.find_last_of('/');
@@ -64,6 +64,9 @@ wil::unique_fd CreateUnixServerSocket(const char* path)
6464
const std::string parent{pathView.substr(0, index)};
6565
pathView = pathView.substr(index + 1);
6666

67+
// Ensure the child name fits in sun_path (with null terminator).
68+
THROW_ERRNO_IF(ENAMETOOLONG, pathView.length() >= sizeof(sockaddr_un::sun_path));
69+
6770
// Get the current working directory to restore it later, and change to the socket's parent
6871
// path.
6972
oldCwd = getcwd(oldCwdBuffer, sizeof(oldCwdBuffer));

src/windows/service/exe/LxssHttpProxy.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -327,12 +327,12 @@ try
327327
if (openBracket != ::std::wstring::npos)
328328
{
329329
const auto closeBracket = portRemoved.find_first_of(L"]");
330-
if (closeBracket == ::std::wstring::npos || (openBracket + 1 >= closeBracket - 1))
330+
if (closeBracket == ::std::wstring::npos || (openBracket + 1 >= closeBracket))
331331
{
332332
// no other of below checks can contain brackets
333333
return UnsupportedProxyReason::Supported;
334334
}
335-
portRemoved = portRemoved.substr(openBracket + 1, closeBracket - 1);
335+
portRemoved = portRemoved.substr(openBracket + 1, closeBracket - openBracket - 1);
336336
}
337337

338338
in6_addr addrV6{};

src/windows/service/exe/LxssUserSession.cpp

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1737,11 +1737,15 @@ try
17371737
RETURN_HR_IF(WSL_E_DISTRO_NOT_STOPPED, m_runningInstances.contains(*DistroGuid));
17381738

17391739
const wil::unique_hfile vhd{::CreateFileW(configuration.VhdFilePath.c_str(), GENERIC_WRITE, 0, nullptr, OPEN_EXISTING, 0, nullptr)};
1740-
if (const DWORD err = GetLastError(); err == ERROR_SHARING_VIOLATION)
1740+
if (!vhd)
17411741
{
1742-
THROW_HR_WITH_USER_ERROR(HRESULT_FROM_WIN32(err), wsl::shared::Localization::MessageVhdInUse());
1742+
const DWORD err = GetLastError();
1743+
if (err == ERROR_SHARING_VIOLATION)
1744+
{
1745+
THROW_HR_WITH_USER_ERROR(HRESULT_FROM_WIN32(err), wsl::shared::Localization::MessageVhdInUse());
1746+
}
1747+
THROW_WIN32(err);
17431748
}
1744-
THROW_LAST_ERROR_IF(!vhd);
17451749

17461750
FILE_SET_SPARSE_BUFFER buffer{
17471751
.SetSparse = Sparse,
@@ -1949,7 +1953,7 @@ HRESULT LxssUserSessionImpl::SetVersion(_In_ LPCGUID DistroGuid, _In_ ULONG Vers
19491953
auto wsl1Pipe = wsl::windows::common::wslutil::OpenAnonymousPipe(LX_RELAY_BUFFER_SIZE, true, true);
19501954

19511955
wsl::windows::common::relay::ScopedMultiRelay stdErrRelay(
1952-
std::vector<HANDLE>{wsl1Pipe.first.get(), reinterpret_cast<HANDLE*>(vmContext.errorSocket.get())}, onTarOutput);
1956+
std::vector<HANDLE>{wsl1Pipe.first.get(), reinterpret_cast<HANDLE>(vmContext.errorSocket.get())}, onTarOutput);
19531957

19541958
// Add mounts for the rootfs and tools.
19551959
auto mounts = _CreateSetupMounts(configuration);
@@ -2014,7 +2018,7 @@ HRESULT LxssUserSessionImpl::SetVersion(_In_ LPCGUID DistroGuid, _In_ ULONG Vers
20142018
auto wsl1Pipe = wsl::windows::common::wslutil::OpenAnonymousPipe(LX_RELAY_BUFFER_SIZE, true, true);
20152019

20162020
wsl::windows::common::relay::ScopedMultiRelay stdErrRelay(
2017-
std::vector<HANDLE>{wsl1Pipe.first.get(), reinterpret_cast<HANDLE*>(vmContext.errorSocket.get())}, onTarOutput);
2021+
std::vector<HANDLE>{wsl1Pipe.first.get(), reinterpret_cast<HANDLE>(vmContext.errorSocket.get())}, onTarOutput);
20182022

20192023
// Add mounts for the rootfs and tools.
20202024
auto mounts = _CreateSetupMounts(configuration);

0 commit comments

Comments
 (0)