Hi James, Samuel,
I did not read all your message yet James, but I agree with Samuel's answer
when it comes about CSRF.
Jacques
Le 27/11/2019 à 09:28, Samuel Trégouët a écrit :
Hi James,
I still don't see any reason to keep checkSecureParameter in any form
because it is related to ServiceEventHandler.
Protection against csrf is a good idea but it has no thing to do with
Service. It should be done upstream in http request processing so every
type of event (ServiceEventHandler, JavaEventHandler,…) could benefit
from this protection.
Samuel
Quoting James Yong (2019-11-26 17:26:59)
Hi Jacques, all,
Haven't look into the POC yet. Please see the following updates:
1. Not a good practice to allow state-changing request via GET method without a
token to check for CSRF.
2. Not a good practice to expose CSRF token in url. See [1]. Moreover, token in
url may also be exposed via if user posts screenshots.
3. CSRF is not a concern for GET request if we avoid point 1 & 2.
4. In OFBiz, the same request handler generally can process both GET and POST
requests. The checkSecureParameter in service events would check for no query
string and as a result, state-changing operation is restricted to a POST method
when service event was used.
5. Propose to revert the removal of checkSecureParameter, but add new code to
allow exceptions to query-string check in service events. Exception can be made
by setting a new attribute, allow-query-string-for-event = true | false
(default), in security tag within request-map tag. Also extend
checkSecureParameter to other event types.
6. Propose to add new attribute, csrfToken = true | false (default), to
security tag to support anti-csrf for post request:
a) For forms: if true, will generate hidden token field value using CSRF
Guard library ( see [3] ) for every rendered form and store this token inside
session map variable. Support may be added for token name to be randomized.
b) For login form: There is a need to prevent Login CSRF. Token will also
be created using pre-session. See [2] for Login CSRF.
c) For XMLHTTPRequest: Use default value for same-origin policy. 1 token generated per user session for ajax call. Support may be added for token name to be randomized for each session.
7. The service.http.parameters.require.encrypted property that was removed,
seems related to encrypting sensitive parameter values but wasn't implemented.
May look into this at a later time.
8. For implementations that aren't using OFBiz screens, a property may be added
to disable the check for CSRF token.
Reference:
[1]
https://danielmiessler.com/blog/sensitive-information-sent-in-the-url-over-https.
[2]
https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
[3] https://www.owasp.org/index.php?title=CSRF_Guard&redirect=no
Regards,
James
