ubeddulla opened a new pull request, #3589:
URL: https://github.com/apache/brpc/pull/3589
### What problem does this PR solve?
Issue Number: resolve N/A
Problem Summary:
1. `StartProfiling` writes the `view` and `base` query parameters straight
into the inline `<script>` of the
`/hotspots/{cpu,heap,growth,contention,iobuf}` page, as part of the url of the
`$.ajax` call it makes to `*_non_responsive`. `GET /hotspots/cpu?view=" +
alert(1) + "</script><script>alert(2)</script>` therefore closes the js string
and the script element and runs attacker-chosen script in the origin that also
serves `/flags?setvalue=` and the rest of the builtin admin endpoints.
2. `DoProfiling`, the handler that same ajax call lands on, already rejects
both parameters with `ValidProfilePath()`, and the comment on that helper says
it has to be applied to every parameter that comes from the url. The page
handler just never calls it, so applying it there too closes the injection and
makes the two handlers agree on what they accept.
3. `/rpcz` is the only other builtin page that echoes a query parameter, and
it prints `time_str` (a `const std::string*`) rather than `*time_str`, so an
unparsable value renders a heap address instead: ``Invalid time=`0x10a9e9168'``.
### What is changed and the side effects?
Changed:
`ValidProfilePath()` on `view` and `base` in `StartProfiling`, failing with
the same `EINVAL` text the sibling handler uses, and `WebEscape()` on the html
branch of the `/rpcz` message. Profile paths the page itself generates
(`./rpc_data/profiling/<checksum>/<timestamp>.<type>`) pass the check, so links
from the page keep working.
Side effects:
- Performance effects: none, one validation of two short query values per
`/hotspots/*` page load.
- Breaking backward compatibility: none. A `view`/`base` that the check
rejects is one `*_non_responsive` already refused, it now fails on the page
instead of in the ajax response. Plain text `/rpcz` output is unchanged.
---
### Check List:
- Added `BuiltinServiceTest.hotspots_query_escaping` and
`BuiltinServiceTest.rpcz_query_escaping` in
`test/brpc_builtin_service_unittest.cpp`. Both fail before the fix: the first
shows `url:
"/hotspots/cpu_non_responsive?console=1&seconds=10&display_type=dot&view=" +
alert(1) + "</script><script>alert(2)</script>",` in the rendered page, the
second shows the leaked pointer. Both pass after.
- `brpc_builtin_service_unittest` is green (21/22) via cmake with
`-DBUILD_UNIT_TESTS=ON`. The one failure is `BuiltinServiceTest.dir`, which
expects `/usr/include/sys/errno.h` and fails on this macOS box before the
change as well.
- Compiles clean with `-Wall -W` in both the debug and the release
configuration.
--
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]
