Project

General

Profile

Actions

Defect #35417

closed

User sessions not reset after 2FA activation

Added by Holger Just almost 3 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Accounts / authentication
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:

Description

Felix Schäfer reports via email to

Hello,

Currently a user signed up on multiple browsers/machines can activate 2FA in one session but still continue using the other sessions. This presents a security risk if an attacker has or gets control of one of those other sessions.

The attached patch resets all the session, autologin and recovery keys of a user when 2FA is set up. Maybe a warning could also be added to the 2FA set up screen about this so that users with multiple active sessions are not surprised about getting logged out from the other sessions.

Thank you,

Felix Schäfer


Files

2fa-session-reset.patch (1.24 KB) 2fa-session-reset.patch Holger Just, 2021-06-14 11:18
test_for_35417.patch (1.18 KB) test_for_35417.patch Marius BĂLTEANU, 2021-07-27 23:15

Related issues

Related to Redmine - Feature #1237: Add support for two-factor authenticationClosedGo MAEDA2008-05-14

Actions
Actions #1

Updated by Go MAEDA almost 3 years ago

  • Target version set to 4.2.2

Setting the target version to 4.2.2.

Maybe a warning could also be added to the 2FA set up screen about this so that users with multiple active sessions are not surprised about getting logged out from the other sessions.

Although it would be more user-friendly to display such a message, I think the patch can be merged as-is because Redmine currently don't show such warning when resseting session by timeout or some reasons.

Actions #2

Updated by Holger Just almost 3 years ago

Go MAEDA wrote:

Although it would be more user-friendly to display such a message, I think the patch can be merged as-is because Redmine currently don't show such warning when resseting session by timeout or some reasons.

I agree. We also destroy / logout other sessions on password change without any further warning. I think this can be merged as is.

Actions #3

Updated by Go MAEDA over 2 years ago

  • Status changed from New to Resolved
  • Assignee set to Go MAEDA

Committed the patch. Thank you.

Actions #4

Updated by Go MAEDA over 2 years ago

  • Status changed from Resolved to Closed
Actions #5

Updated by Holger Just over 2 years ago

  • Status changed from Closed to Reopened
  • Assignee changed from Go MAEDA to Holger Just

Thank you Maeda-san.

As this is now comitted, I would go ahead and reserve a CVE-ID for this issue. The CVE-ID will be marked as reserved until we have a release containing this fix.

Actions #6

Updated by Holger Just over 2 years ago

CVE-2021-37156 was assigned for this.

The CVE entry is not yet public. Once we have a release with this fix, we have to update the CVE entry on https://cveform.mitre.org/ and add a reference to the news and the updated /Security_Advisories page.

Actions #7

Updated by Marius BĂLTEANU over 2 years ago

We can cut a release this weekend, is it ok?

Actions #8

Updated by Holger Just over 2 years ago

Sure!

Actions #9

Updated by Marius BĂLTEANU over 2 years ago

  • Related to Feature #1237: Add support for two-factor authentication added
Actions #10

Updated by Marius BĂLTEANU over 2 years ago

I've added a test for this case, Holger, can you take a look, please?

As Mischa mentioned in #35611, we should have this covered by tests.

Actions #11

Updated by Holger Just over 2 years ago

  • Assignee deleted (Holger Just)

Thank you! I tested the test (heh) and it appears to work correctly (fails without the original patch, works with both in place). I think this one can be merged.

The second part of the patch is however still untested, namely that concurrent active sessions are destroyed if 2fa is activated. I'm not sure how we can properly test the handling of concurrent sessions though...

In any case, could you (Marius and Maeda-san) coordinate to merge this in time before the release?

Actions #12

Updated by Marius BĂLTEANU over 2 years ago

Holger Just wrote:

Thank you! I tested the test (heh) and it appears to work correctly (fails without the original patch, works with both in place). I think this one can be merged.

The second part of the patch is however still untested, namely that concurrent active sessions are destroyed if 2fa is activated. I'm not sure how we can properly test the handling of concurrent sessions though...

In any case, could you (Marius and Maeda-san) coordinate to merge this in time before the release?

Yes, I can do it later today.

Actions #13

Updated by Marius BĂLTEANU over 2 years ago

  • Status changed from Reopened to Closed
  • Assignee set to Go MAEDA

Test committed.

Actions #14

Updated by Holger Just over 2 years ago

For the security scanner, I would then use the following definition after the release:

  register(
    id:        '2021-08-01',
    category:  :privilege_escalation,
    severity:  :low,
    details:   'User sessions not reset after activation of two-factor authentication',
    ticket_id: '35417',
    cves:      ['CVE-2021-37156'],
    fixed_in:  ['< 4.2.0', '>= 4.2.2']
  )

with the following title:

User sessions not reset after activation of two-factor authentication

and description:

When enabling two-factor authentication for a user's account, Redmine allows existing user sessions to continue, resulting in increased risk if an attacker has or gets control of one of those other sessions.

Actions #15

Updated by Marius BĂLTEANU over 2 years ago

  • Tracker changed from Patch to Defect
Actions #16

Updated by Marius BĂLTEANU over 2 years ago

Holger Just wrote:

For the security scanner, I would then use the following definition after the release:

[...]

with the following title:

User sessions not reset after activation of two-factor authentication

and description:

When enabling two-factor authentication for a user's account, Redmine allows existing user sessions to continue, resulting in increased risk if an attacker has or gets control of one of those other sessions.

Thanks Holger for your feedback and also for your updates made today to the Wiki page. I saw that you updated the severity to Medium for this issue and then you reverted without any comment. Is it ok or you reverted by mistake?

Actions #17

Updated by Holger Just over 2 years ago

When updating the security scanner yesterday, I thought a bit more about the possible impact of this. After comparing it with other issues we had tagged as "low" in the past, I came to the conclusion that this probably deserved a medium. I initially tagged it in the security scanner as such.

Only after having updated the severity in the wiki page however, I saw that you mentioned the severity in the release news article as low. Unwilling to change it there too and convinced that consistency is more important than my hunch of severity, I reverted it everywhere back to low. Sorry for this back-and-forth :(

Actions #18

Updated by Marius BĂLTEANU over 2 years ago

Holger Just wrote:

When updating the security scanner yesterday, I thought a bit more about the possible impact of this. After comparing it with other issues we had tagged as "low" in the past, I came to the conclusion that this probably deserved a medium. I initially tagged it in the security scanner as such.

Only after having updated the severity in the wiki page however, I saw that you mentioned the severity in the release news article as low. Unwilling to change it there too and convinced that consistency is more important than my hunch of severity, I reverted it everywhere back to low. Sorry for this back-and-forth :(

Don't worry, I just wanted to double check this with you. In the future, I will avoid posting the severity in the news post.

Actions #19

Updated by Holger Just over 2 years ago

I have requested an update for CVE-2021-37156 on https://cveform.mitre.org/ with the following references:

The CVE should be published in the next few hours with this new information.

Actions #20

Updated by Marius BĂLTEANU almost 2 years ago

  • Project changed from 2 to Redmine
  • Category set to Accounts / authentication
  • Resolution set to Fixed
Actions

Also available in: Atom PDF