Http: protect prefix variable when add variable

[Jim T]

Hi Maxim,

Thanks for your reply.  In fact I want to discuss is that I think nginx
should help user to check the configuration and ensure the configuration
will follow user's expectations.

When user set config like auth_request_set "$email
$upstream_http_x_auth_request_email;" will effect on all server, I think
user can't image this. Avoid doing mistakes like this by users themselves I
think maybe is difficult?

So I think if can interrupt these kind of variable, or we should raise a
warn.  Or we should miracle these variables only work for specific
context?  Could you share more advice for this case?  Thanks.

Best Regards,
Jinhua

<nginx-devel-request at nginx.org> 于 2021年1月21日周四 20:01写道：

> Date: Wed, 20 Jan 2021 19:50:02 +0300
> From: Maxim Dounin <mdounin at mdounin.ru>
> To: nginx-devel at nginx.org
> Subject: Re: Http: protect prefix variable when add variable
> Message-ID: <20210120165002.GW1147 at mdounin.ru>
> Content-Type: text/plain; charset=us-ascii
>
> 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/
>
>
> ------------------------------
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20210123/cee0d0a6/attachment.htm>