Http: protect prefix variable when add variable

[Maxim Dounin]

Hello!

On Wed, Jan 20, 2021 at 07:59:50PM +0800, Jim T wrote:

> Hello!
> 
> There is a incident occur in our team, when we use auth_request_set like
> this in many server, and print $upstream_http_x_auth_request_email in log:
> 
> server {
>     listen       8080 reuseport;
>     server_name  test.io;
>     location / {
>         auth_request            /oauth2/auth;
>         auth_request_set     $email $upstream_http_x_auth_request_email;
>     }
> }
> 
> But when we add a bad auth_request_set like below:
> server {
>     listen       8080 reuseport;
>     server_name  test2.io;
>     location / {
>         auth_request            /oauth2/auth;
>         auth_request_set      $upstream_http_x_auth_request_email $email;
>     }
> }
> 
> We will lost all $upstream_http_x_auth_request_email even the server
> haven't use, because there is a new variable
> $upstream_http_x_auth_request_email, and the prefix variable can't be read
> any more.
> 
> So I think we can fix it like this, to avoid the wrong configuration:

Thank you for your suggestion and patch.
See comments below.

> # HG changeset patch
> # User Jinhua Tan <312841925 at qq.com>
> # Date 1611143620 -28800
> #      Wed Jan 20 19:53:40 2021 +0800
> # Node ID fd7e9432a59abcfcf380ddedb1e892098a54a845
> # Parent  61d0df8fcc7c630da35e832ba8e983db0061a3be
> Http: protect prefix variable when add variable
> 
> diff -r 61d0df8fcc7c -r fd7e9432a59a src/http/ngx_http_variables.c
> --- a/src/http/ngx_http_variables.c Tue Jan 19 20:35:17 2021 +0300
> +++ b/src/http/ngx_http_variables.c Wed Jan 20 19:53:40 2021 +0800
> @@ -393,6 +393,20 @@
>  };
> 
> 
> +static ngx_str_t  ngx_http_protect_variables_prefix[] = {
> +    ngx_string("arg_"),
> +    ngx_string("http_"),
> +    ngx_string("sent_http_"),
> +    ngx_string("sent_trailer_"),
> +    ngx_string("cookie_"),
> +    ngx_string("arg_"),
> +    ngx_string("upstream_http_"),
> +    ngx_string("upstream_trailer_"),
> +    ngx_string("upstream_cookie_"),
> +    ngx_null_string
> +};

Using a static list of prefixes is certainly wrong: there can be 
arbitrary prefix variables added by various modules, and limiting 
checks to a predefied list is not going to work correctly.

> +
> +
>  ngx_http_variable_value_t  ngx_http_variable_null_value =
>      ngx_http_variable("");
>  ngx_http_variable_value_t  ngx_http_variable_true_value =
> @@ -410,6 +424,7 @@
>      ngx_hash_key_t             *key;
>      ngx_http_variable_t        *v;
>      ngx_http_core_main_conf_t  *cmcf;
> +    ngx_str_t                  *p;
> 
>      if (name->len == 0) {
>          ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> @@ -421,6 +436,18 @@
>          return ngx_http_add_prefix_variable(cf, name, flags);
>      }
> 
> +    if (flags & NGX_HTTP_VAR_CHANGEABLE) {
> +        for (p = ngx_http_protect_variables_prefix; p->len; p++) {
> +            if (name->len >= p.len
> +                && ngx_strncasecmp(name->data, p->data, p->len) == 0)
> +            {
> +                ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                                   "similar to prefix variable \"%V\"",
> *p);
> +                return NULL;
> +            }
> +        }
> +    }
> +
>      cmcf = ngx_http_conf_get_module_main_conf(cf, ngx_http_core_module);
> 
>      key = cmcf->variables_keys->keys.elts;

Prefix variables are intentionally implemented in a way which 
allows one to overwrite them: for example, this is used in nginx 
itself to provide custom handler for some $http_* variables, such 
as $http_host (which can be effectively retrieved, since nginx has 
a pointer to the Host header explicitly stored).  That is, it is 
quite normal that the ngx_http_add_variable() function you modify 
is called with a variable which is more specific than a registered 
prefix variable.  And using the NGX_HTTP_VAR_CHANGEABLE flag to 
distinguish when to fail looks wrong, as this is an unrelated 
flag.  You probably mean to detect user-added variables, such as 
introduced by "set" or "auth_request_set".  There is no way to 
detect such variables except in a particular directive used to 
define the variable.

Further, I tend to think there are valid use cases when one may 
want to actually redefine a particular variable.

Overall, the patch certainly needs more work, and I very much 
doubt we want to introduce such checks at all and hence this work 
needs to be done.  A better solution might be to avoid doing 
mistakes like the one you've described above.

-- 
Maxim Dounin
http://mdounin.ru/