FileManager: avoid a TOCTOU issue in computing CWD #1035
Conversation
|
@swift-ci please test |
Sources/FoundationEssentials/FileManager/FileManager+Directories.swift
Outdated
Show resolved
Hide resolved
Sources/FoundationEssentials/FileManager/FileManager+Directories.swift
Outdated
Show resolved
Hide resolved
Sources/FoundationEssentials/FileManager/FileManager+Directories.swift
Outdated
Show resolved
Hide resolved
|
@swift-ci please test |
On Windows, we could potentially return a `nil` for the current working
directory in the rare case that the current working directory was
changed during the computation:
```swift
let dwLength: DWORD = GetCurrentDirectoryW(0, nil) // 1
return withUnsafeTemporaryAllocation(of: WCHAR.self, capacity: Int(dwLength)) {
if GetCurrentDirectoryW(dwLength, $0.baseAddress) == dwLength - 1 { // 2
return String(decodingCString: $0.baseAddress!, as: UTF16.self)
}
return nil // 3
}
```
Consider the case where at step 1, we receive $n$. We then are
interrupted, the CWD changed. We then perform step 2, where we receive
$m$ (st $m != n$). We would then proceed to point 3, where we return
`nil`. Avoid this TOCTOU issue by repeating this operation to a fixed
point.
Because we are guaranteed a current directory on Windows (unless the
initial query for the buffer size fails), we will eventually succeed. In
order to avoid a DoS attack vector, limit the attempt to quiescence to a
fixed number.
|
@swift-ci please test |
| } | ||
| return nil | ||
| #else | ||
| withUnsafeTemporaryAllocation(of: CChar.self, capacity: FileManager.MAX_PATH_SIZE) { buffer in | ||
| guard getcwd(buffer.baseAddress!, FileManager.MAX_PATH_SIZE) != nil else { |
There was a problem hiding this comment.
One quick question - on non-Windows we just allocate a buffer of MAX_PATH_SIZE (1024 bytes IIRC) and assume that the path will fit (returning nil if for some reason it doesn't). Just confirming - I assume there's no equivalent for Windows that we should use either instead of requesting the buffer size or using in the case of the size check failing?
There was a problem hiding this comment.
Yeah, there is no real equivalent for Windows. The majority of the API surface on Windows is designed to assume that you will query the required sizes. In the case of the file system, the only place there is a small buffer size guarantee is the Win32 API surface and in Foundation we will bypass that layer entirely when working with paths to ensure that long paths work.
|
Is this an established pattern of calling this API? Or: how do other libraries handle this race? |
|
@kperryua the established pattern is to call it in a loop until it succeeds. However, this does open up a possible DoS attack vector where the attacker has access to your address space and is able to inject a detour for The actual race condition that exists with the loop variant is only possible in a multithreaded scenario where a thread is constantly changing the working directory to a monotonically increasing size (which will obviously be bound as you cannot grow ad infinitum) and you are forced to yield control in between the calls. I'm not sure how likely that is to be hit in practice though. |
|
@jmschonfeld oaky to merge? |
On Windows, we could potentially return a `nil` for the current working
directory in the rare case that the current working directory was
changed during the computation:
```swift
let dwLength: DWORD = GetCurrentDirectoryW(0, nil) // 1
return withUnsafeTemporaryAllocation(of: WCHAR.self, capacity: Int(dwLength)) {
if GetCurrentDirectoryW(dwLength, $0.baseAddress) == dwLength - 1 { // 2
return String(decodingCString: $0.baseAddress!, as: UTF16.self)
}
return nil // 3
}
```
Consider the case where at step 1, we receive $n$. We then are
interrupted, the CWD changed. We then perform step 2, where we receive
$m$ (st $m != n$). We would then proceed to point 3, where we return
`nil`. Avoid this TOCTOU issue by repeating this operation to a fixed
point.
Because we are guaranteed a current directory on Windows (unless the
initial query for the buffer size fails), we will eventually succeed. In
order to avoid a DoS attack vector, limit the attempt to quiescence to a
fixed number.
On Windows, we could potentially return a
nilfor the current working directory in the rare case that the current working directory was changed during the computation:Consider the case where at step 1, we receive$n$ . We then are interrupted, the CWD changed. We then perform step 2, where we receive $m$ (st $m != n$ ). We would then proceed to point 3, where we return
nil. Avoid this TOCTOU issue by repeating this operation to a fixed point.Because we are guaranteed a current directory on Windows (unless the initial query for the buffer size fails), we will eventually succeed. In order to avoid a DoS attack vector, limit the attempt to quiescence to a fixed number.
Fixes: #1034