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