From 4873a0c0c91f8fa2cade724391ad8826b2d90da1 Mon Sep 17 00:00:00 2001 From: Rivka Segan <15526879-rivkasegan@users.noreply.gitlab.com> Date: Mon, 28 Aug 2023 04:53:31 +0000 Subject: [PATCH] Avoid large logs of 127.0.0.1:5959 attack payloads Because veilid-server listens on 127.0.0.1 TCP port 5959, it is potentially open to attacks from websites if a user runs an ordinary web browser (e.g., Chrome or Firefox) on the same computer. Specifically, any https website can include JavaScript code that begins with let message = 'WASTE_YOUR_VEILID_SERVER_DISK_SPACE_'.repeat(1000); fetch('http://127.0.0.1:5959/' + message) and the web browser will then send many KB of data to veilid-server, where it may typically be logged to disk by this code: https://gitlab.com/veilid/veilid/-/blob/2ab51ae3e97b718067f57ce2162e142a325504df/veilid-core/src/veilid_api/serialize_helpers/serialize_json.rs#L6-L12 (Because Veilid hasn't even reached 1.0, it's very common for users to enable a large amount of logging.) The threat model is that someone creates a website that's apparently of interest to any Veilid user, but the actual purpose of the website is to leverage the user's web browser to silently tunnel an attack payload into another application that is local to the user. An attack that sends more than 1 MB of data (for each fetch API call) is feasible, and the patch in this MR tries to address that. Note that the most common web browsers always allow JavaScript on arbitrary https websites to send data to 127.0.0.1 port 5959, there is no configuration menu to shut this off, and the user is not alerted that this is occurring. Brave 1.54 (June 2023) was the first popular web browser to block this: https://brave.com/privacy-updates/27-localhost-permission/ This does not mean that an adversary can just as easily setup a website to send: {"op":"Control","args":["Shutdown"],"id":1} to 127.0.0.1 TCP port 5959 and thereby terminate a veilid-server process. A web browser using http will always send requests that begin with specific strings (such as GET or OPTIONS) on the first line, and the code at: https://gitlab.com/veilid/veilid/-/blob/2ab51ae3e97b718067f57ce2162e142a325504df/veilid-server/src/client_api.rs#L367 https://gitlab.com/veilid/veilid/-/blob/2ab51ae3e97b718067f57ce2162e142a325504df/veilid-server/src/client_api.rs#L244 https://gitlab.com/veilid/veilid/-/blob/2ab51ae3e97b718067f57ce2162e142a325504df/veilid-server/src/client_api.rs#L202 seems to work together to ensure that no JSON object results in command execution unless the first line of the input is a JSON object. (Not sure if this was a design goal, or simply how it turned out.) A web browser can do other things besides cleartext http (e.g., try to start a TLS session to 127.0.0.1 TCP port 5959), but it's perhaps unlikely that the initial bytes of the network traffic, in the context of the above threat model, would ever be a JSON object. Note that, although veilid-server is not speaking the HTTP protocol on 127.0.0.1 TCP port 5959, it is still able to read the data sent by any web browser to http://127.0.0.1:5959, send that data to a JSON parser, and write the data to the server logs. In limited testing, the HTTP client typically saw zero bytes of application layer response; however, if the HTTP client sent a huge amount of data (e.g., 16 MB), the HTTP client would sometimes receive a large response with JSON data about veilid-server's internal state. That might be a separate bug. In the context of the threat model, this may not matter because that JSON data isn't accessible by the operator of the website (that hosts the JavaScript code). There may be many ways to resolve this. First, the Veilid documentation could recommend never running a web browser on any machine that has veilid-server with 127.0.0.1 TCP port 5959 open. Second, the existence of a realistically probe-able service on 127.0.0.1 TCP port 5959 might be considered much too large an attack surface for an application of Veilid's sensitivity, and interprocess communication could be replaced with something other than unauthenticated TCP. This MR is intended to improve Veilid for an ordinary user who wants to help the project by installing veilid-server on their primary personal machine, and wants veilid-cli to remain usable, but needs to continue routine web browsing on that machine. It provides safer behavior for such a person. The MR is not intended to benefit experts who already understand localhost TCP risks, and either avoid all web browsing on the same machine or have their own countermeasures. These experts will not see any attacker-controlled traffic on port 5959, and thus the reduction in logging should be of no concern to them. Without the patch (and with logging on), data sent by a web browser is always logged by veilid-server in the form: Connection processing failure: Parse error: 'expected value at line 1 column 1' with value 'deserialize_json: --- GET / HTTP/1.1 --- to type veilid_core::veilid_api::json_api::Request' regardless of how long the attacker controlled data is. Some browsers such as Chrome start by sending OPTIONS instead of GET. With the patch, long malformed input is discarded and the log instead contains: Connection processing failure: Parse error: 'expected value at line 1 column 1' with value 'deserialize_json: --- :skipped long input that's not a JSON object --- to type veilid_core::veilid_api::json_api::Request' The patch allows logging of anything where the first non-whitespace character is a '{' - this is considered safe (at the moment) because no web browser (realistically used by a local user) can send '{' at the beginning of the first line. Also, the patch allows logging of requests smaller than 50 bytes to support two use cases. First, if a node operator is sending one of the simple JSON API requests by hand and is accidentally omitting the initial '{' from the JSON object, they'll be able to see the failure in their logs. Second, non-expert node operators may want some limited visibility into the details of adversarial activity on http://127.0.0.1:5959. Of course, this default logging policy could be made more flexible later if Veilid decides to stay with unauthenticated TCP. The patch only aims to defeat a simple DoS attack against the out-of-the-box code. --- veilid-server/src/client_api.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/veilid-server/src/client_api.rs b/veilid-server/src/client_api.rs index d4ea2541..36f47e12 100644 --- a/veilid-server/src/client_api.rs +++ b/veilid-server/src/client_api.rs @@ -15,6 +15,8 @@ use veilid_core::tools::*; use veilid_core::*; use wg::AsyncWaitGroup; +const MAX_NON_JSON_LOGGING: usize = 50; + cfg_if! { if #[cfg(feature="rt-async-std")] { use async_std::io::prelude::BufReadExt; @@ -199,12 +201,21 @@ impl ClientApi { jrp: JsonRequestProcessor, request_line: RequestLine, ) -> VeilidAPIResult> { - let line = request_line.line; + let line = request_line.line.trim_start(); + + // Avoid logging failed deserialization of large adversarial payloads from + // http://127.0.0.1:5959 by using an initial colon to force a parse error. + let sanitized_line = if line.len() > MAX_NON_JSON_LOGGING && !line.starts_with("{") { + ":skipped long input that's not a JSON object".to_string() + } else { + line.to_string() + }; + let responses_tx = request_line.responses_tx; // Unmarshal NDJSON - newline => json // (trim all whitespace around input lines just to make things more permissive for API users) - let request: json_api::Request = deserialize_json(&line)?; + let request: json_api::Request = deserialize_json(&sanitized_line)?; // See if this is a control message or a veilid-core message let response = if let json_api::RequestOp::Control { args } = request.op {