Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Public archives not available with sympa-6.2.38 #527

Closed
pascalmaes opened this issue Jan 12, 2019 · 15 comments
Closed

Public archives not available with sympa-6.2.38 #527

pascalmaes opened this issue Jan 12, 2019 · 15 comments
Labels

Comments

@pascalmaes
Copy link

pascalmaes commented Jan 12, 2019

A user not logged in can't see the archive of a list with a public web access

ERROR (confirm_action) - Authorization rejected. Maybe you forgot to log in?

Version

6.2.38

Installation method

From source on a virgin system (the previous installation of sympa has been deleted).
Everything as default.

Expected behavior

The archive should be readable

Actual behavior

ERROR (confirm_action) - Authorization rejected. Maybe you forgot to log in?

In sympa.log

Jan 12 11:20:04 sympa-test wwsympa[30577]: info main::do_arc(2019-01, /) [robot listes] [session 95681996162673] [client y] [list test-4]
Jan 12 11:20:04 sympa-test wwsympa[30577]: info main::check_action_parameters() [robot listes] [session 95681996162673] [client y] [list test-4] CSRF token mismatch: in="" session="aaad607574282c8ea3a865514c3ba433"

In sympa.err

Jan 12 11:20:04 sympa-test wwsympa[30577]: err main::#1557 [robot listes] [session 95681996162673] [client y] Missing required parameters for action "confirm_action"

Additional information

When logged as listmaster, I can't see the scenario source of "access right (web_access)"

INTERNAL SERVER ERROR (dump_scenario) -

In sympa.err

Jan 12 11:15:32 sympa-test.sipr.ucl.ac.be wwsympa[30577]: err main::#1572 > main::do_dump_scenario#15942 > Sympa::Scenario::new#77 Missing parameter

@ikedas
Copy link
Member

ikedas commented Jan 12, 2019

Hi @pascalmaes,

Could you please tell us these things?

  • detailed operation you performed (launched browser, went to URL xxx, clicked xxx, ...).
  • web browser(s) you tried.

Thanks.

@pascalmaes
Copy link
Author

I have try with Safari, Firefox, Google Chrome on macOS and with Firefox on Windows

I know the URL : https://sympa-test.sipr.ucl.ac.be/sympa/arc/test-4/2019-01/

@ikedas ikedas added the bug label Jan 12, 2019
@ikedas
Copy link
Member

ikedas commented Jan 12, 2019

@pascalmaes, I think it's a bug.

Can you do either one of below?

  • Add this line to sympa.conf (after doing this, service have to be reloaded)
    web_archive_spam_protection javascript
    
  • Change config of corresponding list to set "Archives" / "email address protection method" to "javascript".

@pascalmaes
Copy link
Author

The line web_archive_spam_protection has been added to /etc/sympa/sympa.conf
The services has been restarted.

I take a look to the config file and I see

web_access HASH(0x7322d80)

and I have an error message in sympa.log

Jan 12 13:35:18 sympa-test.sipr.ucl.ac.be task_manager[1108]: info Sympa::List::_load_list_config_file() Bad entry "web_access HASH(0x7322d80)" in /var/sympa/list_data/listes/test-4/config

I modified the access to private and then again to public. Now I have

archive
web_access public
mail_access owner

and the access is OK for anybody

For email address protection method (web_archive_spam_protection)(default), I have

use Javascript (javascript)

I'm still unable to see the source scenario of the archive web access

The buttons "source scenario" are working in "List Definition", "Sending/receiving setup", "Privileges" except for the shared_doc part ; not working in "Archives" and "Bounces".

and the error messages are

Jan 12 13:51:49 sympa-test wwsympa[1170]: info main::do_dump_scenario(test-4, tracking) [robot listes] [session 95681996162673] [client y] [user pascal.maes@live.be] [list test-4]
Jan 12 13:51:49 sympa-test wwsympa[1170]: notice Sympa::WWW::Report::reject_report_web() Unable to notify listmaster for error: "cannot_open_file": (no robot)
Jan 12 13:51:49 sympa-test wwsympa[1170]: info main::do_dump_scenario() [robot listes] [session 95681996162673] [client y] [user listmaster] [list test-4] Failed to load scenario

@ikedas
Copy link
Member

ikedas commented Jan 12, 2019

Broken web_access line and error on "source scenario" page seem related to issue #520. On these problems, please continue at that issue page.

I'll investigate problem on web_archive_spam_protection and will write on progress at this page in the next week.

Thanks for reporting bug in detail!

@racke
Copy link
Contributor

racke commented Jan 12, 2019

I tested this with 6.2.38 and I can reproduce the problem with web_archive_spam_protection set to cookie.
With javascript it works fine.

@mpkut
Copy link
Contributor

mpkut commented Jan 13, 2019

My apologies, folks. We observed this in our logs after our 6.2.38 upgrade just today. I just love running into really pretty obvious cases that were not tested before rolling out into production. This one I believe is my doing.

The sequence seems to be

  • user visits /arc/LISTNAME with a GET request
  • archive protection code triggers an internal confirm_actionaction to generate the I am not a spammer click-through form
  • confirm_action is in the %require_csrftoken list, but no CSRF token was provided due to the use of a GET request
  • check_action_parameters forbids access with the error message noted above, because no CSRF token was provided

Our tests this afternoon show that removing confirm_action from the %require_csrf array allows unauthenticated archive access to work while still applying CSRF prevention to the requests still on the list.

*** /usr/libexec/sympa/wwsympa.fcgi.save	2019-01-09 19:59:20.000000000 -0600
--- /usr/libexec/sympa/wwsympa.fcgi	2019-01-12 20:39:00.964946622 -0600
***************
*** 730,736 ****
  
  our %require_csrftoken = (
      'add'            => 1,
-     'confirm_action' => 1,
      'del'            => 1,
      'move_user'      => 1,
      'savefile'       => 1,
--- 730,735 ----

Now, confirm_action action is in the %require_csrftoken list in the first place based on the idea that requests could be forged through this action otherwise. However, the existence of GET requests that trigger this action means that we really can't have it in the list. So the quickest workaround for the archive access issue would be to just remove or comment it out per the diff above.

Looking at Session.pm further it seems that you can only successfully confirm an action if the session data contains a hash of the arguments and other action-specific data. That would seem to inherently act against CSRF on its own, since the session will not usually be primed with the right data. So the removal of confirm_action from the list may be acceptable as a long term solution.

@ikedas
Copy link
Member

ikedas commented Jan 13, 2019

This also broken:

@mpkut
Copy link
Contributor

mpkut commented Jan 13, 2019

Seems like the same mechanism: GET request -> trigger confirm_action > fails due to lack of CSRF token.

I put confirm_action back in the %require_csrftoken array in our test environment and the auto_signoff request broke as described; removing it again resulted in seeing the expected click-through dialogue.

@ikedas
Copy link
Member

ikedas commented Jan 13, 2019

@mpkut, sorry for crossing comment.

I think the measure with POST request should be left, like:

diff --git a/src/cgi/wwsympa.fcgi.in b/src/cgi/wwsympa.fcgi.in
index 22bd44a..9699cba 100644
--- a/src/cgi/wwsympa.fcgi.in
+++ b/src/cgi/wwsympa.fcgi.in
@@ -2315,7 +2315,8 @@ sub check_action_parameters {
     }

     ## Validate CSRF token when one is required
-    if (defined($require_csrftoken{$param->{'action'}})) {
+    if (defined($require_csrftoken{$param->{'action'}})
+        and $ENV{'REQUEST_METHOD'} ne 'GET') {
         wwslog('debug', 'Action %s: CSRF token required', $param->{'action'});

         unless (defined($in{'csrftoken'})

@mpkut
Copy link
Contributor

mpkut commented Jan 13, 2019

Hello @ikedas,

That would certainly allow all GET-based requests to operate as in previous versions. Is there any risk of a GET-based way to supply the necessary action arguments to do something that the POST-based CSRF code would prevent?

@ikedas
Copy link
Member

ikedas commented Jan 13, 2019

Indeed, attackers can throw anything in by GET requests. What we can do at the next step is to prevent unneccessary GET requests.

@racke
Copy link
Contributor

racke commented Jan 13, 2019

GET requests should not be used to change data in the system etc. So if these exists, I suggest to turn them into POST requests.

Further reading: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Resources_that_need_to_be_protected_from_CSRF_vulnerability

@mpkut
Copy link
Contributor

mpkut commented Jan 15, 2019

Further review suggests that the simple patch of removing the confirm_action action from the %require_csrftoken hash is workable enough, so I have submitted #529 to that effect.

@ikedas
Copy link
Member

ikedas commented Jan 18, 2019

New release closing this issue is come in this Saturday. Thanks for reporting, investigating and fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants