runtime: blacklist /dev, /proc and /sys from watch dirs.#3165
runtime: blacklist /dev, /proc and /sys from watch dirs.#3165htuch merged 5 commits intoenvoyproxy:masterfrom
Conversation
server_test proto fuzzing resulted in OOM because of /dev traversal. It seems bad to allow runtime paths to point in these places. Risk Level: Low Testing: New unit tests. Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Seems OK though it still feels a bit platform specific to me. I had some comments on some additional follow ups we might want to do.
|
|
||
| void DiskLayer::walkDirectory(const std::string& path, const std::string& prefix) { | ||
| ENVOY_LOG(debug, "walking directory: {}", path); | ||
| // Check if this is an obviously bad path. |
There was a problem hiding this comment.
Per offline convo, I still think the correct solution here is a max recursion check. Can we TODO that? Additionally, It seems like we should potentially do this check in any place we attempt to load a file (TLS certs, etc.)? Should we issue/TODO that also?
There was a problem hiding this comment.
Agree. Added TODO for recursion depth and filed #3170.
Signed-off-by: Harvey Tuch <htuch@google.com>
| // TODO(htuch): When we are using C++17, switch to std::filesystem::canonical. | ||
| char* resolved_path = ::realpath(path.c_str(), nullptr); | ||
| if (resolved_path == nullptr) { | ||
| throw EnvoyException(fmt::format("Unable to determine canonical path for {}", path)); |
There was a problem hiding this comment.
Can you cover this case? Perhaps if you just pass realpath() a bogus path? (I'm not sure what the OSX issue was).
There was a problem hiding this comment.
This is covered in FilesystemImpl.CanonicalPathFail.
Signed-off-by: Harvey Tuch <htuch@google.com>
source/common/runtime/runtime_impl.h
Outdated
| const std::string path_; | ||
| Api::OsSysCalls& os_sys_calls_; | ||
| // Maximum recursion depth for walkDirectory(). | ||
| const uint32_t MaxWalkDepth = 4; |
There was a problem hiding this comment.
I would go higher here. This might even be about where we are with our normal topology. Maybe 20?
Signed-off-by: Harvey Tuch <htuch@google.com>
…3165) server_test proto fuzzing resulted in OOM because of /dev traversal. It seems bad to allow runtime paths to point in these places. Risk Level: Low Testing: New unit tests. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Rama <rama.rao@salesforce.com>
server_test proto fuzzing resulted in OOM because of /dev traversal. It
seems bad to allow runtime paths to point in these places.
Risk Level: Low
Testing: New unit tests.
Signed-off-by: Harvey Tuch htuch@google.com