BUG/MAJOR: log/sink: possible sink collision in sink_new_from_srv()

sink_new_from_srv() leverages sink_new_buf() with the server id as name,
sink_new_buf() then calls __sink_new() with the provided name.

Unfortunately sink_new() is designed in such a way that it will first look
up in the list of existing sinks to check if a sink already exists with
given name, in which case the existing sink is returned. While this
behavior may be error-prone, it is actually up to the caller to ensure
that the provided name is unique if it really expects a unique sink
pointer.

Due to this bug in sink_new_from_srv(), multiple tcp servers with the same
name defined in distinct log backends would end up sharing the same sink,
which means messages sent to one of the servers would also be forwarded to
all servers with the same name across all log backend sections defined in
the config, which is obviously an issue and could even raise security
concerns.

Example:

  defaults
    log backend@log-1 local0

  backend log-1
    mode log
    server s1 127.0.0.1:514
  backend log-2
    mode log
    server s1 127.0.0.1:5114

With the above config, logs sent to log-1/s1 would also end up being sent
to log-2/s1 due to server id "s1" being used for tcp servers in distinct
log backends.

To fix the issue, we now prefix the sink ame with the backend name:
back_name/srv_id combination is known to be unique (backend name
serves as a namespace)

This bug was reported by GH user @landon-lengyel under #2846.

UDP servers (with udp@ prefix before the address) are not affected as they
don't make use of the sink facility.

As a workaround, one should manually ensure that all tcp servers across
different log backends (backend with "mode log" enabled) use unique names

This bug was introduced in e58a9b4 ("MINOR: sink: add sink_new_from_srv()
function") thus it exists since the introduction of log backends in 2.9,
which means this patch should be backported up to 2.9.

diff --git a/src/sink.c b/src/sink.c
index b86c336bcdd875c3d514c4dd039c51600a69b05f..25d7a2fee6461065a2a98b391808797f436fc847 100644 (file)
--- a/src/sink.c
+++ b/src/sink.c
@@ -1285,17 +1285,28 @@ struct sink *sink_new_from_srv(struct server *srv, const char *from)
 {
        struct sink *sink = NULL;
        int bufsize = (srv->log_bufsize) ? srv->log_bufsize : BUFSIZE;
+       char *sink_name = NULL;
 
        /* prepare description for the sink */
        chunk_reset(&trash);
        chunk_printf(&trash, "created from %s declared into '%s' at line %d", from, srv->conf.file, srv->conf.line);
 
+       memprintf(&sink_name, "%s/%s", srv->proxy->id, srv->id);
+       if (!sink_name) {
+               ha_alert("memory error while creating ring buffer for server '%s/%s'.\n", srv->proxy->id, srv->id);
+               goto error;
+       }
+
        /* directly create a sink of BUF type, and use UNSPEC log format to
         * inherit from caller fmt in sink_write()
+        *
+        * sink_name must be unique to prevent existing sink from being re-used
         */
-       sink = sink_new_buf(srv->id, trash.area, LOG_FORMAT_UNSPEC, bufsize);
+       sink = sink_new_buf(sink_name, trash.area, LOG_FORMAT_UNSPEC, bufsize);
+       ha_free(&sink_name); // no longer needed
+
        if (!sink) {
-               ha_alert("unable to create a new sink buffer for server '%s'.\n", srv->id);
+               ha_alert("unable to create a new sink buffer for server '%s/%s'.\n", srv->proxy->id, srv->id);
                goto error;
        }