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@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel