chenBright opened a new pull request, #3523:
URL: https://github.com/apache/brpc/pull/3523
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
Two builtin services let whoever reaches them do more than the surrounding
configuration intends.
#### `/flags` can repoint the bvar dump files.
The bvar dump flags carry an accept-anything validator whose only job is
to
wake the dumping thread:
```cpp
// src/bvar/variable.cpp
static bool wakeup_dumping_thread(const char*, const std::string&) {
pthread_cond_signal(&dump_cond);
return true;
}
```
The dumping thread re-reads those flags every round and ends up in
`FileDumper::dump_impl()`:
```cpp
butil::CreateDirectoryAndGetError(dir, &error); // recursive mkdir
_fp = fopen(_filename.c_str(), "w"); // truncate
```
So two requests against a server with default options:
```
GET /flags/bvar_dump_file?setvalue=/victim/dir/target
GET /flags/bvar_dump?setvalue=true
```
create a directory chain anywhere and truncate a file inside it, with the
privileges of the server. `setvalue` is a GET, so this is reachable from a
browser as well.
The primitive is bounded but real. `bvar_dump_file` and `bvar_dump_tabs`
get a
`.data` suffix forced on them by `FilePath::AddExtension()`, and the bytes
written are bvar names and values, not attacker input. `mbvar_dump_file` is
used verbatim with no suffix. `bvar_dump_tabs` names files too: its tab
names
go through the same `AddExtension()`, which appends them to the path with
no
sanitization, so `../` in a tab name escapes the configured directory. The
recursive mkdir and the truncation are unrestricted in every case.
#### `/pprof` ignores `FLAGS_max_profiling_seconds`, which `/hotspots`
enforces.
```cpp
// src/brpc/builtin/pprof_service.cpp, ReadSeconds(), before this PR
return seconds; // no upper
bound
```
```cpp
// src/brpc/builtin/hotspots_service.cpp:233
seconds = std::min(seconds, FLAGS_max_profiling_seconds);
```
`?seconds=2147483647` reaches `bthread_usleep(sleep_sec * 1000000L)`,
roughly
68 years. The profilers are process wide -- gperftools' `ProfilerStart` and
bthread's `g_cp` are each a single global, so this is not merely one hung
RPC: it holds the only CPU or contention profiler for as long as it asked
for,
and `/hotspots/cpu` and `/hotspots/contention` answer 503 the whole time.
The
300 second limit was enforced on only one of the two paths to the same
resource, so the path without it decides the limit for both.
### What is changed and the side effects?
Changed:
- drop the validator registration for -bvar_dump_file`, `-bvar_dump_tabs`
and
`-mbvar_dump_file`.
- `src/brpc/builtin/pprof_service.cpp`: clamp `ReadSeconds()` to
`-max_profiling_seconds`.
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of
Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
