Defect #35417
closed
User sessions not reset after 2FA activation
Added by Holger Just almost 3 years ago. Updated almost 2 years ago.
0%
Description
Felix Schäfer reports via email to security@redmine.org
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
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.
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.
Updated by Go MAEDA almost 3 years ago
- Status changed from New to Resolved
- Assignee set to Go MAEDA
Committed the patch. Thank you.
Updated by Holger Just almost 3 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.
Updated by Holger Just almost 3 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.
Updated by Marius BĂLTEANU almost 3 years ago
We can cut a release this weekend, is it ok?
Updated by Marius BĂLTEANU almost 3 years ago
- Related to Feature #1237: Add support for two-factor authentication added
Updated by Marius BĂLTEANU almost 3 years ago
- File test_for_35417.patch test_for_35417.patch added
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.
Updated by Holger Just almost 3 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?
Updated by Marius BĂLTEANU almost 3 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.
Updated by Marius BĂLTEANU over 2 years ago
- Status changed from Reopened to Closed
- Assignee set to Go MAEDA
Test committed.
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.
Updated by Marius BĂLTEANU over 2 years ago
- Tracker changed from Patch to Defect
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?
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 :(
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.
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.
Updated by Marius BĂLTEANU almost 2 years ago
- Project changed from 2 to Redmine
- Category set to Accounts / authentication
- Resolution set to Fixed