Skip to content

Commit

Permalink
Merge pull request from GHSA-56fm-hfp3-x3w3
Browse files Browse the repository at this point in the history 
Fix CSRF Vulnerability on 2FA endpoints
  • Loading branch information
@j0k3r
j0k3r committed Oct 2, 2023
2 parents 9ec351e + aa06e83 commit 0cfdddc
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 70 deletions.
51 changes: 39 additions & 12 deletions src/Wallabag/CoreBundle/Controller/ConfigController.php
View file
Expand Up @@ -254,10 +254,14 @@ public function indexAction(Request $request, Config $craueConfig, TaggingRuleRe
/**
* Disable 2FA using email.
*
* @Route("/config/otp/email/disable", name="disable_otp_email")
* @Route("/config/otp/email/disable", name="disable_otp_email", methods={"POST"})
*/
public function disableOtpEmailAction()
public function disableOtpEmailAction(Request $request)
{
if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
throw $this->createAccessDeniedException('Bad CSRF token.');
}

$user = $this->getUser();
$user->setEmailTwoFactor(false);

Expand All @@ -274,10 +278,14 @@ public function disableOtpEmailAction()
/**
* Enable 2FA using email.
*
* @Route("/config/otp/email", name="config_otp_email")
* @Route("/config/otp/email", name="config_otp_email", methods={"POST"})
*/
public function otpEmailAction()
public function otpEmailAction(Request $request)
{
if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
throw $this->createAccessDeniedException('Bad CSRF token.');
}

$user = $this->getUser();

$user->setGoogleAuthenticatorSecret(null);
Expand All @@ -297,10 +305,14 @@ public function otpEmailAction()
/**
* Disable 2FA using OTP app.
*
* @Route("/config/otp/app/disable", name="disable_otp_app")
* @Route("/config/otp/app/disable", name="disable_otp_app", methods={"POST"})
*/
public function disableOtpAppAction()
public function disableOtpAppAction(Request $request)
{
if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
throw $this->createAccessDeniedException('Bad CSRF token.');
}

$user = $this->getUser();

$user->setGoogleAuthenticatorSecret('');
Expand All @@ -319,10 +331,14 @@ public function disableOtpAppAction()
/**
* Enable 2FA using OTP app, user will need to confirm the generated code from the app.
*
* @Route("/config/otp/app", name="config_otp_app")
* @Route("/config/otp/app", name="config_otp_app", methods={"POST"})
*/
public function otpAppAction(GoogleAuthenticatorInterface $googleAuthenticator)
public function otpAppAction(Request $request, GoogleAuthenticatorInterface $googleAuthenticator)
{
if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
throw $this->createAccessDeniedException('Bad CSRF token.');
}

$user = $this->getUser();
$secret = $googleAuthenticator->generateSecret();

Expand Down Expand Up @@ -357,8 +373,10 @@ function ($backupCode) {
* Cancelling 2FA using OTP app.
*
* @Route("/config/otp/app/cancel", name="config_otp_app_cancel")
*
* XXX: commented until we rewrite 2fa with a real two-steps activation
*/
public function otpAppCancelAction()
/*public function otpAppCancelAction()
{
$user = $this->getUser();
$user->setGoogleAuthenticatorSecret(null);
Expand All @@ -367,15 +385,19 @@ public function otpAppCancelAction()
$this->userManager->updateUser($user, true);
return $this->redirect($this->generateUrl('config') . '#set3');
}
}*/

/**
* Validate OTP code.
*
* @Route("/config/otp/app/check", name="config_otp_app_check")
* @Route("/config/otp/app/check", name="config_otp_app_check", methods={"POST"})
*/
public function otpAppCheckAction(Request $request, GoogleAuthenticatorInterface $googleAuthenticator)
{
if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
throw $this->createAccessDeniedException('Bad CSRF token.');
}

$isValid = $googleAuthenticator->checkCode(
$this->getUser(),
$request->get('_auth_code')
Expand All @@ -395,7 +417,12 @@ public function otpAppCheckAction(Request $request, GoogleAuthenticatorInterface
'scheb_two_factor.code_invalid'
);

return $this->redirect($this->generateUrl('config_otp_app'));
$this->addFlash(
'notice',
'scheb_two_factor.code_invalid'
);

return $this->redirect($this->generateUrl('config') . '#set3');
}

/**
Expand Down
9 changes: 0 additions & 9 deletions src/Wallabag/CoreBundle/Form/Type/UserInformationType.php
View file
Expand Up @@ -23,15 +23,6 @@ public function buildForm(FormBuilderInterface $builder, array $options)
->add('email', EmailType::class, [
'label' => 'config.form_user.email_label',
])
->add('emailTwoFactor', CheckboxType::class, [
'required' => false,
'label' => 'config.form_user.emailTwoFactor_label',
])
->add('googleTwoFactor', CheckboxType::class, [
'required' => false,
'label' => 'config.form_user.googleTwoFactor_label',
'mapped' => false,
])
->add('save', SubmitType::class, [
'label' => 'config.form.save',
])
Expand Down
36 changes: 32 additions & 4 deletions src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig
View file
Expand Up @@ -209,6 +209,10 @@

{{ form_widget(form.user.save, {'attr': {'class': 'btn waves-effect waves-light'}}) }}

{{ form_widget(form.user._token) }}

{{ form_end(form.user) }}

<br/>
<br/>
<div class="row">
Expand All @@ -229,18 +233,42 @@
<tr>
<td>{{ 'config.form_user.two_factor.emailTwoFactor_label'|trans }}</td>
<td>{% if app.user.isEmailTwoFactor %}<b>{{ 'config.form_user.two_factor.state_enabled'|trans }}</b>{% else %}{{ 'config.form_user.two_factor.state_disabled'|trans }}{% endif %}</td>
<td><a href="{{ path('config_otp_email') }}" class="waves-effect waves-light btn{% if app.user.isEmailTwoFactor %} disabled{% endif %}">{{ 'config.form_user.two_factor.action_email'|trans }}</a> {% if app.user.isEmailTwoFactor %}<a href="{{ path('disable_otp_email') }}" class="waves-effect waves-light btn red">Disable</a>{% endif %}</td>
<td>
<form action="{{ path('config_otp_email') }}" method="post" name="config_otp_email">
<input type="hidden" name="token" value="{{ csrf_token('otp') }}" />

<button class="waves-effect waves-light btn{% if app.user.isEmailTwoFactor %} disabled{% endif %}" type="submit">{{ 'config.form_user.two_factor.action_email'|trans }}</button>
</form>
{% if app.user.isEmailTwoFactor %}
<form action="{{ path('disable_otp_email') }}" method="post" name="disable_otp_email">
<input type="hidden" name="token" value="{{ csrf_token('otp') }}" />

<button class="waves-effect waves-light btn red" type="submit">Disable</button>
</form>
{% endif %}
</td>
</tr>
<tr>
<td>{{ 'config.form_user.two_factor.googleTwoFactor_label'|trans }}</td>
<td>{% if app.user.isGoogleTwoFactor %}<b>{{ 'config.form_user.two_factor.state_enabled'|trans }}</b>{% else %}{{ 'config.form_user.two_factor.state_disabled'|trans }}{% endif %}</td>
<td><a href="{{ path('config_otp_app') }}" class="waves-effect waves-light btn{% if app.user.isGoogleTwoFactor %} disabled{% endif %}">{{ 'config.form_user.two_factor.action_app'|trans }}</a> {% if app.user.isGoogleTwoFactor %}<a href="{{ path('disable_otp_app') }}" class="waves-effect waves-light btn red">Disable</a>{% endif %}</td>
<td>
<form action="{{ path('config_otp_app') }}" method="post" name="config_otp_app">
<input type="hidden" name="token" value="{{ csrf_token('otp') }}" />

<button class="waves-effect waves-light btn{% if app.user.isGoogleTwoFactor %} disabled{% endif %}" type="submit">{{ 'config.form_user.two_factor.action_app'|trans }}</button>
</form>
{% if app.user.isGoogleTwoFactor %}
<form action="{{ path('disable_otp_app') }}" method="post" name="disable_otp_app">
<input type="hidden" name="token" value="{{ csrf_token('otp') }}" />

<button class="waves-effect waves-light btn red" type="submit">Disable</button>
</form>
{% endif %}
</td>
</tr>
</tbody>
</table>
</div>
{{ form_widget(form.user._token) }}
</form>
</div>

<div id="set4" class="col s12">
Expand Down
4 changes: 1 addition & 3 deletions src/Wallabag/CoreBundle/Resources/views/Config/otp_app.html.twig
View file
Expand Up @@ -40,6 +40,7 @@
{% endfor %}

<form class="form" action="{{ path("config_otp_app_check") }}" method="post">
<input type="hidden" name="token" value="{{ csrf_token('otp') }}" />
<div class="card-content">
<div class="row">
<div class="input-field col s12">
Expand All @@ -49,9 +50,6 @@
</div>
</div>
<div class="card-action">
<a href="{{ path('config_otp_app_cancel') }}" class="waves-effect waves-light grey btn">
{{ 'config.otp.app.cancel'|trans }}
</a>
<button class="btn waves-effect waves-light" type="submit" name="send">
{{ 'config.otp.app.enable'|trans }}
<i class="material-icons right">send</i>
Expand Down
74 changes: 32 additions & 42 deletions tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php
View file
Expand Up @@ -1174,14 +1174,13 @@ public function testUserEnable2faEmail()
$this->logInAs('admin');
$client = $this->getTestClient();

$crawler = $client->request('GET', '/config/otp/email');

$this->assertSame(302, $client->getResponse()->getStatusCode());
$crawler = $client->request('GET', '/config');

$crawler = $client->followRedirect();
$form = $crawler->filter('form[name=config_otp_email]')->form();
$client->submit($form);

$this->assertGreaterThan(1, $alert = $crawler->filter('body')->extract(['_text']));
$this->assertStringContainsString('flashes.config.notice.otp_enabled', $alert[0]);
$this->assertSame(302, $client->getResponse()->getStatusCode());
$this->assertStringContainsString('flashes.config.notice.otp_enabled', $client->getContainer()->get(SessionInterface::class)->getFlashBag()->get('notice')[0]);

// restore user
$em = $this->getEntityManager();
Expand All @@ -1201,14 +1200,23 @@ public function testUserDisable2faEmail()
$this->logInAs('admin');
$client = $this->getTestClient();

$crawler = $client->request('GET', '/config/otp/email/disable');
$em = $this->getEntityManager();
$user = $em
->getRepository(User::class)
->findOneByUsername('admin');

$this->assertSame(302, $client->getResponse()->getStatusCode());
$user->setEmailTwoFactor(true);
$em->persist($user);
$em->flush();

$crawler = $client->followRedirect();
$crawler = $client->request('GET', '/config');

$this->assertGreaterThan(1, $alert = $crawler->filter('body')->extract(['_text']));
$this->assertStringContainsString('flashes.config.notice.otp_disabled', $alert[0]);
$form = $crawler->filter('form[name=disable_otp_email]')->form();
$client->submit($form);

$this->assertSame(302, $client->getResponse()->getStatusCode());

$this->assertStringContainsString('flashes.config.notice.otp_disabled', $client->getContainer()->get(SessionInterface::class)->getFlashBag()->get('notice')[0]);

// restore user
$em = $this->getEntityManager();
Expand All @@ -1224,7 +1232,10 @@ public function testUserEnable2faGoogle()
$this->logInAs('admin');
$client = $this->getTestClient();

$crawler = $client->request('GET', '/config/otp/app');
$crawler = $client->request('GET', '/config');

$form = $crawler->filter('form[name=config_otp_app]')->form();
$client->submit($form);

$this->assertSame(200, $client->getResponse()->getStatusCode());

Expand All @@ -1243,49 +1254,28 @@ public function testUserEnable2faGoogle()
$em->flush();
}

public function testUserEnable2faGoogleCancel()
public function testUserDisable2faGoogle()
{
$this->logInAs('admin');
$client = $this->getTestClient();

$crawler = $client->request('GET', '/config/otp/app');

$this->assertSame(200, $client->getResponse()->getStatusCode());

// restore user
$em = $this->getEntityManager();
$user = $em
->getRepository(User::class)
->findOneByUsername('admin');

$this->assertTrue($user->isGoogleTwoFactor());
$this->assertGreaterThan(0, $user->getBackupCodes());

$crawler = $client->request('GET', '/config/otp/app/cancel');

$this->assertSame(302, $client->getResponse()->getStatusCode());

$user = $em
->getRepository(User::class)
->findOneByUsername('admin');

$this->assertFalse($user->isGoogleTwoFactor());
$this->assertEmpty($user->getBackupCodes());
}
$user->setGoogleAuthenticatorSecret("Google2FA");
$em->persist($user);
$em->flush();

public function testUserDisable2faGoogle()
{
$this->logInAs('admin');
$client = $this->getTestClient();
$crawler = $client->request('GET', '/config');

$crawler = $client->request('GET', '/config/otp/app/disable');
$form = $crawler->filter('form[name=disable_otp_app]')->form();
$client->submit($form);

$this->assertSame(302, $client->getResponse()->getStatusCode());

$crawler = $client->followRedirect();

$this->assertGreaterThan(1, $alert = $crawler->filter('body')->extract(['_text']));
$this->assertStringContainsString('flashes.config.notice.otp_disabled', $alert[0]);

$this->assertStringContainsString('flashes.config.notice.otp_disabled', $client->getContainer()->get(SessionInterface::class)->getFlashBag()->get('notice')[0]);

// restore user
$em = $this->getEntityManager();
Expand Down

0 comments on commit 0cfdddc

Please sign in to comment.