When looking at the REST documentation the small section 'Preventing CSRF Attacks' only lists the X-REQUESTED-WITH header as required. But it seems the Origin header is also mandatory, at least on updates. The config option "csrf_enforce_header_origin" has no influence on this.

When recently upgrading a tracker at a customer, one client can no longer run their scripts (which do an update) because they get "Required Header Missing". Unfortunately it doesn't even tell *which* header is missing.

Two things should be done:
- Mention all required and optional headers in the REST-API CSRF section, in particular the headers that are needed even if turned off in config.ini
- Maybe mention *which* header is missing in the error message returned to the user. I'm not sure if this would constitute a security issue but I think not.

Hi Ralf:

In message <1732543729.53.0.967849195543.issue2551372@roundup.psfhosted.org>,
Ralf Schlatterbeck writes:
>When looking at the REST documentation the small section 'Preventing
>CSRF Attacks' only lists the X-REQUESTED-WITH header as required. But
>it seems the Origin header is also mandatory, at least on
>updates. The config option "csrf_enforce_header_origin" has no
>influence on this.

I think Origin is the only other one because it has to be checked
against allowed_api_origins if the request is not a GET. (I guess your
update is a PUT/PATCH/POST.)

Technically the Origin header is not a CSRF header in this role. It's
an API authorization header. The allowed_api_origins setting is
documented in the CSRF attacks section with:

  If you want to allow Roundup's api to be accessed by an application
  that is not hosted at the same origin as Roundup, you must permit
  the origin using the ``allowed_api_origins`` setting in
  ``config.ini``.

How about adding this sentence:

  If you access the REST interface with a method other than GET, you
  must also supply an origin header with a value that is permitted by
  ``allowed_api_origins``.

Does this provide you with the info?

>When recently upgrading a tracker at a customer, one client can no
>longer run their scripts (which do an update) because they get
>"Required Header Missing". Unfortunately it doesn't even tell *which*
>header is missing.

It should be logged at error level. It is not displayed to the client
intentionally.

However, this code in handle_rest:

        # verify Origin is allowed on all requests including GET.
        # If a GET, missing origin is allowed  (i.e. same site GET request)
        if not self.is_origin_header_ok(api=True):
            if 'HTTP_ORIGIN' not in self.env:
                msg = self._("Required Header Missing") (*)
            else:
                msg = self._("Client is not allowed to use Rest Interface.")

is missing code after (*) like:

  logger.error("Origin header missing or unauthorized for REST request.")

This might be more useful if it also logs the method (get, post ...)
and the authenticated user.

>Two things should be done:
>- Mention all required and optional headers in the REST-API CSRF
>  section, in particular the headers that are needed even if turned off
>  in config.ini

Origin is the only one I can think of. IIRC the X-REQUESTED-WITH
header check can be turned off. The allowed_api_origins can't be
turned off.  If empty it has the value of the origin from the
tracker.web setting (i.e. self).

>- Maybe mention *which* header is missing in the error message
>  returned to the user. I'm not sure if this would constitute a
>  security issue but I think not.

The missing headers should be logged on the server. Recommendations on
reporting the missing header to the client was mixed when the code was
originally written. More sources were against reporting the missing
header to the attacker who could use a non-browser originated REST
request to probe the app.

OSWAP also comes down on the do not report side as well.

https://cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html#error-handling

  * Respond with generic error messages - avoid revealing details of
    the failure unnecessarily.

details like the header name.

Thoughts?

On Mon, Nov 25, 2024 at 04:26:43PM +0000, John Rouillard wrote:
> I think Origin is the only other one because it has to be checked
> against allowed_api_origins if the request is not a GET. (I guess your
> update is a PUT/PATCH/POST.)
> 
> Technically the Origin header is not a CSRF header in this role. It's
> an API authorization header. The allowed_api_origins setting is
> documented in the CSRF attacks section with:
> 
>   If you want to allow Roundup's api to be accessed by an application
>   that is not hosted at the same origin as Roundup, you must permit
>   the origin using the ``allowed_api_origins`` setting in
>   ``config.ini``.
> 
> How about adding this sentence:
> 
>   If you access the REST interface with a method other than GET, you
>   must also supply an origin header with a value that is permitted by
>   ``allowed_api_origins``.

This should include the default origin, e.g.

... with a value that is either the default origin (the URL of the
tracker without the path component set in the config file as ``web``
in section ``[tracker]``) or one that is permitted by ``allowed_api_origins``.

> >When recently upgrading a tracker at a customer, one client can no
> >longer run their scripts (which do an update) because they get
> >"Required Header Missing". Unfortunately it doesn't even tell *which*
> >header is missing.
> 
> It should be logged at error level. It is not displayed to the client
> intentionally.

I thought so. Do you think an attacker gets any new insights when this
would be displayed? The things are documented (we're just working on
making the docs better :-) and it would help tremendously in debugging
if it is shown to the user.

> However, this code in handle_rest:
> 
>         # verify Origin is allowed on all requests including GET.
>         # If a GET, missing origin is allowed  (i.e. same site GET request)
>         if not self.is_origin_header_ok(api=True):
>             if 'HTTP_ORIGIN' not in self.env:
>                 msg = self._("Required Header Missing") (*)
>             else:
>                 msg = self._("Client is not allowed to use Rest Interface.")
> 
> is missing code after (*) like:
> 
>   logger.error("Origin header missing or unauthorized for REST request.")

Ah. That's why I didn't find anything in the log. But I grepped for
'csrf'. Maybe that should be included in the log message even if it is
not only for csrf protection. At least the log messages for missing
headers should have something in common we can grep for. Not necessarily
the string 'csrf' although all the other log messages on headers seem to
have that string.

> This might be more useful if it also logs the method (get, post ...)
> and the authenticated user.

Yes, that would be fine.
I'll try to get my changes on the feature branch merged next, then I can
put some time into this.

> The missing headers should be logged on the server. Recommendations on
> reporting the missing header to the client was mixed when the code was
> originally written. More sources were against reporting the missing
> header to the attacker who could use a non-browser originated REST
> request to probe the app.
> 
> OSWAP also comes down on the do not report side as well.
> 
> https://cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html#error-handling
> 
>   * Respond with generic error messages - avoid revealing details of
>     the failure unnecessarily.
> 
> details like the header name.
> 
> Thoughts?

Hmm, I'm still in favour of reporting the name of the missing header.
This would be useful to me, too when a user reports something.
As mentioned above: When an attacker knows what they're attacking they
also know which headers would be required because we are now working on
documenting this...

Thanks for looking into this so quickly!
Kind regards
Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

Hi Ralf:

In message <20241125170046.njam6esqtet4qbhj@runtux.com>,
Ralf Schlatterbeck writes:
>On Mon, Nov 25, 2024 at 04:26:43PM +0000, John Rouillard wrote:
>> Technically the Origin header is not a CSRF header in this role. It's
>> an API authorization header. The allowed_api_origins setting is
>> documented in the CSRF attacks section with:
>> 
>>   If you want to allow Roundup's api to be accessed by an application
>>   that is not hosted at the same origin as Roundup, you must permit
>>   the origin using the ``allowed_api_origins`` setting in
>>   ``config.ini``.
>> 
>> How about adding this sentence:
>> 
>>   If you access the REST interface with a method other than GET, you
>>   must also supply an origin header with a value that is permitted by
>>   ``allowed_api_origins``.
>
>This should include the default origin, e.g.
>
>... with a value that is either the default origin (the URL of the
>tracker without the path component set in the config file as ``web``
>in section ``[tracker]``) or one that is permitted by ``allowed_api_origins``.

Yup that works.

>> It should be logged at error level. It is not displayed to the client
>> intentionally.
>
>I thought so. Do you think an attacker gets any new insights when this
>would be displayed? The things are documented (we're just working on
>making the docs better :-) and it would help tremendously in debugging
>if it is shown to the user.

I would stick with OSWAP recommendations there and keep information
about invalid requests and possible attack strategies away from the
client.  I guess we could add a config.ini flag to enable extended
error output exposing the internal settings. This would be similar
to web.debug that displays tracebacks in the web interface.

>> However, this code in handle_rest:
>> 
>>         # verify Origin is allowed on all requests including GET.
>>         # If a GET, missing origin is allowed  (i.e. same site GET request)
>>         if not self.is_origin_header_ok(api=True):
>>             if 'HTTP_ORIGIN' not in self.env:
>>                 msg = self._("Required Header Missing") (*)
>>             else:
>>                 msg = self._("Client is not allowed to use Rest Interface.")
>> 
>> is missing code after (*) like:
>> 
>>   logger.error("Origin header missing or unauthorized for REST request.")
>
>Ah. That's why I didn't find anything in the log. But I grepped for
>'csrf'. Maybe that should be included in the log message even if it is
>not only for csrf protection. At least the log messages for missing
>headers should have something in common we can grep for. Not necessarily
>the string 'csrf' although all the other log messages on headers seem to
>have that string.

I think all of the header checks report as an error when enabled. So
searching for ERROR should have turned it up (if the logger.error code
had existed). But I have no issue with adding a unique tag in the log
message. Maybe 'HEADER_ERROR:' or something so it makes sens when
hedrs are checked in anti-CSRF or Authorization contexts.

>Hmm, I'm still in favour of reporting the name of the missing header.
>This would be useful to me, too when a user reports something.

Well you can just check the logs. If the log include the username, it
would make it easier to search/query.

>As mentioned above: When an attacker knows what they're attacking they
>also know which headers would be required because we are now working on
>documenting this...

CSRF headers can be turned on/off. So telling the attacker which
headers are required seems to make their job easier.

I've now fixed (and pushed) this, both adding the logging when the origin header is missing as well as updating the docs in the CSRF section of the rest documentation. Let me know if you think we can close this.
Regarding grep for missing headers: It seems all log message contain the word 'header' it's just a matter of knowing what to grep for :-)

Hi Ralf:

My only concern is that it could be used to fill the logs with errors.
But that's an issue we have elsewhere in the code. I don't have a good
solution for it or how to make it easier to respond to.

Logging the username might be useful in tracking down the cause.
But, I think this code is accessible from an anonymous user if
anonymous is granted rest access and the username is useless in
this case.

Maybe logging the IP address to allow firewalling in case of DOS?
I don't think the Client object has that. It would also need to be
proxy aware (see rev 627c5d6a0551 for changes to roundup-server) which
would require a setting to enable/disable.

If none of these seem worthwhile/feasible, I say close it.

Hi John,

On Wed, Dec 04, 2024 at 07:17:15PM +0000, John Rouillard wrote:
> 
> Hi Ralf:
> 
> My only concern is that it could be used to fill the logs with errors.
> But that's an issue we have elsewhere in the code. I don't have a good
> solution for it or how to make it easier to respond to.

Yes, we could make it configurable. But I have several customers where
I'm very happy when I can diagnose things with the right amount of
logging. And seeing when there are REST calls with missing headers is
certainly one of them.

> Logging the username might be useful in tracking down the cause.
> But, I think this code is accessible from an anonymous user if
> anonymous is granted rest access and the username is useless in
> this case.

Yes, I do not have any trackers where anonymous has REST access.

> Maybe logging the IP address to allow firewalling in case of DOS?

Yes, makes sense.
I have a use-case where a sync job is using the API, finding all
log entries where something goes wrong is a use-case for me (the sync
job comes from a fixed IP), I'll look into this.

> I don't think the Client object has that. It would also need to be
> proxy aware (see rev 627c5d6a0551 for changes to roundup-server) which
> would require a setting to enable/disable.

We could use the already-introduced -P option for that if I understand
this correctly? Do we really need that option? I guess a X-Forwarded-For
header would only be present in the proxy-case so logging the IP from
there if the header is present wouldn't require a -P option?

Note that I'm running roundup in the meantime always behind apache using
uwsgi. This *is* sort-of a proxy setup. I'm not sure what logging would
do in that case, have you experience with uwsgi concerning logging of
IPs?

Thanks
Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

In message <20241205064231.fhotuss6d4hoggh6@runtux.com>,
Ralf Schlatterbeck writes:
>
>
>Ralf Schlatterbeck added the comment:
>
>Hi John,
>
>On Wed, Dec 04, 2024 at 07:17:15PM +0000, John Rouillard wrote:
>> 
>> Hi Ralf:
>> 
>> My only concern is that it could be used to fill the logs with errors.
>> But that's an issue we have elsewhere in the code. I don't have a good
>> solution for it or how to make it easier to respond to.
>
>Yes, we could make it configurable. But I have several customers where
>I'm very happy when I can diagnose things with the right amount of
>logging. And seeing when there are REST calls with missing headers is
>certainly one of them.

I think logging these by default is a good idea. I just don't know how
to handle it being abused. Rate limiting could work, but seems
overkill.  YAGNI seems to apply here for now if we can add info so the
admin can use other tools to address abuse.

[...]
>> Maybe logging the IP address to allow firewalling in case of DOS?
>
>Yes, makes sense.
>I have a use-case where a sync job is using the API, finding all
>log entries where something goes wrong is a use-case for me (the sync
>job comes from a fixed IP), I'll look into this.
>
>> I don't think the Client object has that. It would also need to be
>> proxy aware (see rev 627c5d6a0551 for changes to roundup-server) which
>> would require a setting to enable/disable.
>
>We could use the already-introduced -P option for that if I understand
>this correctly?

-P only works for the standalone roundup-server. AFAIK, only
roundup-server logs an http log format string. None of the other ways
of running Roundup (cgi, wsgi, zope) do. But all of the other ways
should have access to all the CGI environment variables. Only
roundup-server filters that environment.

>Do we really need that option? I guess a X-Forwarded-For
>header would only be present in the proxy-case so logging the IP from
>there if the header is present wouldn't require a -P option?

X-Forwarded-For can be sent by the client. It should be used when
Roundup is accessible only via reverse proxy. Then the reverse proxy
will set that header correctly.

>Note that I'm running roundup in the meantime always behind apache using
>uwsgi. This *is* sort-of a proxy setup. I'm not sure what logging would
>do in that case, have you experience with uwsgi concerning logging of
>IPs?

I am not running uwsgi anymore. AFAIK, only roundup-server logs
connection info currently.