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.