BUG/MEDIUM: stats: subject "stats admin" accesses to "stats scope" filtering

It was reported by Red Hat in partnership with AISLE Research that
proxies updated via a stats page in "stats admin" mode were not subject
to the filtering of "stats scope".

It is totally true and was never meant to be when "stats scope" was
added in 1.2 in 2006 while "stats admin" arrived 5 years later in 1.4,
nor was it even implied by the doc, but it is easy to see how someone
who would combine the two could expect them to work together:

- "stats scope" only limits which proxies are shown, the purpose being
  to avoid polluting the page with a long list of auxiliary proxies that
  take ages to render (e.g. those dedictated to stick-tables etc). A
  common use case is to only show known frontends so that numerous
  backends are not listed, it's most often used on public access.

- "stats admin" is used to permit to change a proxy or a server's
  state and is protected using HTTP Basic auth, cleartext passwords
  by default, and is mostly meant to be used from trusted sources,
  hence private access.

The two are almost never seen together (no single occurrence found
in github issues for 92 occurrences of "stats admin" and 5 of
"stats scope"), but admittedly, if someone were to combine the two,
seeing buttons on the listed proxies only, they would surely expect
to only be able to act on those.

So in order to clear this void, this commit does the following upon
a POST:
  - when a scope is defined, the requested proxy must exist and match
    the list of permitted ones, otherwise a DENY is returned. This
    ensures that the POST cannot be abused either to infer the existence
    of a given proxy name.
  - when no scope is defined, a non-existing proxy continues to return
    an "unknown proxy" error.

The doc was updated to explain this new behavior. This commit should
be backported to stable versions, along with the previous patch it
depends on:

   MINOR: stats: factor the proxy vs scope check into its own function

The stats code was split between 2.9 and 3.0, so the changes must all
be performed in stats.c before 3.0. Also, ctx->http_px does not exist
earlier, but http_px is simply the calling stream's backend (s->be at
some places).

Thanks to Red Hat and AISLE Research for reporting this.

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 2acf2763f202835af4e7ee17fd1a587f30d7dd0e..b76c7541feb8a3162f38f9914beec488dd28cc21 100644 (file)
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -12758,7 +12758,9 @@ stats admin { if | unless } <cond>
   matched.
 
   The admin level allows to enable/disable servers from the web interface. By
-  default, statistics page is read-only for security reasons.
+  default, statistics page is read-only for security reasons. If "stats scope"
+  directives are set in the section, then only proxies designated by these
+  directives will accept state changes; access to other ones will be denied.
 
   Currently, the POST request is limited to the buffer size minus the reserved
   buffer space, which means that if the list of servers is too long, the
@@ -12793,8 +12795,8 @@ stats admin { if | unless } <cond>
         stats http-request auth unless AUTH
         stats admin if AUTH_ADMIN
 
-  See also : "stats enable", "stats auth", "stats http-request", section 12.2
-             about userlists and section 7 about ACL usage.
+  See also : "stats enable", "stats auth", "stats http-request", "stats scope",
+             section 12.2 about userlists and section 7 about ACL usage.
 
 ssl-f-use [<sslbindconf> ...]*
   Assignate a certificate to the current frontend.
@@ -13122,11 +13124,11 @@ stats scope { <name> | "." }
               section in which the statement appears.
 
   When this statement is specified, only the sections enumerated with this
-  statement will appear in the report. All other ones will be hidden. This
-  statement may appear as many times as needed if multiple sections need to be
-  reported. Please note that the name checking is performed as simple string
-  comparisons, and that it is never checked that a give section name really
-  exists.
+  statement will appear in the report. All other ones will be hidden, and
+  attempts to change their state in admin mode will be rejected. This statement
+  may appear as many times as needed if multiple sections need to be reported.
+  Please note that the name checking is performed as simple string comparisons,
+  and that it is never checked that a give section name really exists.
 
   Though this statement alone is enough to enable statistics reporting, it is
   recommended to set all other settings in order to avoid relying on default
@@ -13150,7 +13152,8 @@ stats scope { <name> | "." }
         stats uri     /admin?stats
         stats refresh 5s
 
-  See also : "stats auth", "stats enable", "stats realm", "stats uri"
+  See also : "stats auth", "stats enable", "stats realm", "stats uri" and
+             "stats admin"
 
 
 stats show-desc [ <desc> ]

diff --git a/src/stats-html.c b/src/stats-html.c
index 68790fbb9a716af6be66f635ac424a8aa0e04c6a..469ca6ac1c838111e9b0a66d2f3ba608c619cc94 100644 (file)
--- a/src/stats-html.c
+++ b/src/stats-html.c
@@ -19,6 +19,7 @@
 #include <haproxy/pipe.h>
 #include <haproxy/proxy.h>
 #include <haproxy/stats.h>
+#include <haproxy/stats-proxy.h>
 #include <haproxy/stconn.h>
 #include <haproxy/server.h>
 #include <haproxy/task.h>
@@ -1734,7 +1735,29 @@ static int stats_process_http_post(struct stconn *sc)
 
                        /* Now we can check the key to see what to do */
                        if (!px && (strcmp(key, "b") == 0)) {
-                               if ((px = proxy_be_by_name(value)) == NULL) {
+                               /* we need to lookup the proxy because its name
+                                * may be a real name ("pub") or numeric ("#4").
+                                */
+                               px = proxy_be_by_name(value);
+
+                               /* now we have two possibilities:
+                                *   - if the are "stats scope" rules, a non-existing
+                                *     proxy or a forbidden one are the same, we return
+                                *     a deny because a non-existing name cannot match
+                                *     a scope rule and we don't want to disclose the
+                                *     existence of a given name.
+                                *   - without "stats scope" rules, we just return
+                                *     not found.
+                                */
+                               if (ctx->http_px->uri_auth && ctx->http_px->uri_auth->scope) {
+                                       if (!px || !stats_proxy_in_scope(px, ctx->http_px->uri_auth, ctx->http_px)) {
+                                               /* "stats scope" rules do not permit to access this proxy name */
+                                               ctx->st_code = STAT_STATUS_DENY;
+                                               goto out;
+                                       }
+                               }
+
+                               if (!px) {
                                        /* the backend name is unknown or ambiguous (duplicate names) */
                                        ctx->st_code = STAT_STATUS_ERRP;
                                        goto out;