From 36066ee7f33ecf155979ae3554e832e307166b2e Mon Sep 17 00:00:00 2001 From: Steve Smith Date: Sun, 12 Dec 2021 18:25:39 -0500 Subject: [PATCH] Fix Security Issues (#644) * Apply fix for Microsoft Security Advisory CVE-2018-0784 * Apply fix for Microsoft Security Advisory CVE-2018-0785 --- src/Web/Controllers/ManageController.cs | 88 ++++++++++++++----- .../Manage/EnableAuthenticatorViewModel.cs | 4 +- ...Model.cs => ShowRecoveryCodesViewModel.cs} | 3 +- .../Views/Manage/EnableAuthenticator.cshtml | 2 +- .../Views/Manage/GenerateRecoveryCodes.cshtml | 26 +++--- src/Web/Views/Manage/ShowRecoverCodes.cshtml | 25 ++++++ .../Manage/TwoFactorAuthentication.cshtml | 2 +- 7 files changed, 111 insertions(+), 39 deletions(-) rename src/Web/ViewModels/Manage/{GenerateRecoveryCodesViewModel.cs => ShowRecoveryCodesViewModel.cs} (71%) create mode 100644 src/Web/Views/Manage/ShowRecoverCodes.cshtml diff --git a/src/Web/Controllers/ManageController.cs b/src/Web/Controllers/ManageController.cs index f9d4cb1..90be6a9 100644 --- a/src/Web/Controllers/ManageController.cs +++ b/src/Web/Controllers/ManageController.cs @@ -26,6 +26,7 @@ public class ManageController : Controller private readonly UrlEncoder _urlEncoder; private const string AuthenticatorUriFormat = "otpauth://totp/{0}:{1}?secret={2}&issuer={0}&digits=6"; + private const string RecoveryCodesKey = nameof(RecoveryCodesKey); public ManageController( UserManager userManager, @@ -370,37 +371,42 @@ public class ManageController : Controller throw new ApplicationException($"Unable to load user with ID '{_userManager.GetUserId(User)}'."); } - var unformattedKey = await _userManager.GetAuthenticatorKeyAsync(user); - if (string.IsNullOrEmpty(unformattedKey)) - { - await _userManager.ResetAuthenticatorKeyAsync(user); - unformattedKey = await _userManager.GetAuthenticatorKeyAsync(user); - } - - var model = new EnableAuthenticatorViewModel - { - SharedKey = FormatKey(unformattedKey), - AuthenticatorUri = GenerateQrCodeUri(user.Email, unformattedKey) - }; + var model = new EnableAuthenticatorViewModel(); + await LoadSharedKeyAndQrCodeUriAsync(user, model); return View(model); } + [HttpGet] + public IActionResult ShowRecoveryCodes() + { + var recoveryCodes = (string[])TempData[RecoveryCodesKey]; + if (recoveryCodes == null) + { + return RedirectToAction(nameof(TwoFactorAuthentication)); + } + + var model = new ShowRecoveryCodesViewModel { RecoveryCodes = recoveryCodes }; + return View(model); + } + + [HttpPost] [ValidateAntiForgeryToken] public async Task EnableAuthenticator(EnableAuthenticatorViewModel model) { - if (!ModelState.IsValid) - { - return View(model); - } - var user = await _userManager.GetUserAsync(User); if (user == null) { throw new ApplicationException($"Unable to load user with ID '{_userManager.GetUserId(User)}'."); } + if (!ModelState.IsValid) + { + await LoadSharedKeyAndQrCodeUriAsync(user, model); + return View(model); + } + // Strip spaces and hypens var verificationCode = model.Code.Replace(" ", string.Empty).Replace("-", string.Empty); @@ -409,13 +415,17 @@ public class ManageController : Controller if (!is2faTokenValid) { - ModelState.AddModelError("model.TwoFactorCode", "Verification code is invalid."); + ModelState.AddModelError("Code", "Verification code is invalid."); + await LoadSharedKeyAndQrCodeUriAsync(user, model); return View(model); } await _userManager.SetTwoFactorEnabledAsync(user, true); _logger.LogInformation("User with ID {UserId} has enabled 2FA with an authenticator app.", user.Id); - return RedirectToAction(nameof(GenerateRecoveryCodes)); + var recoveryCodes = await _userManager.GenerateNewTwoFactorRecoveryCodesAsync(user, 10); + TempData[RecoveryCodesKey] = recoveryCodes.ToArray(); + + return RedirectToAction(nameof(ShowRecoveryCodes)); } [HttpGet] @@ -441,7 +451,8 @@ public class ManageController : Controller return RedirectToAction(nameof(EnableAuthenticator)); } - [HttpGet] + [HttpPost] + [ValidateAntiForgeryToken] public async Task GenerateRecoveryCodes() { var user = await _userManager.GetUserAsync(User); @@ -456,11 +467,28 @@ public class ManageController : Controller } var recoveryCodes = await _userManager.GenerateNewTwoFactorRecoveryCodesAsync(user, 10); - var model = new GenerateRecoveryCodesViewModel { RecoveryCodes = recoveryCodes.ToArray() }; - _logger.LogInformation("User with ID {UserId} has generated new 2FA recovery codes.", user.Id); - return View(model); + var model = new ShowRecoveryCodesViewModel { RecoveryCodes = recoveryCodes.ToArray() }; + + return View(nameof(ShowRecoveryCodes), model); + } + + [HttpGet] + public async Task GenerateRecoveryCodesWarning() + { + var user = await _userManager.GetUserAsync(User); + if (user == null) + { + throw new ApplicationException($"Unable to load user with ID '{_userManager.GetUserId(User)}'."); + } + + if (!user.TwoFactorEnabled) + { + throw new ApplicationException($"Cannot generate recovery codes for user with ID '{user.Id}' because they do not have 2FA enabled."); + } + + return View(nameof(GenerateRecoveryCodesWarning)); } private void AddErrors(IdentityResult result) @@ -496,4 +524,18 @@ public class ManageController : Controller _urlEncoder.Encode(email), unformattedKey); } + + private async Task LoadSharedKeyAndQrCodeUriAsync(ApplicationUser user, EnableAuthenticatorViewModel model) + { + var unformattedKey = await _userManager.GetAuthenticatorKeyAsync(user); + if (string.IsNullOrEmpty(unformattedKey)) + { + await _userManager.ResetAuthenticatorKeyAsync(user); + unformattedKey = await _userManager.GetAuthenticatorKeyAsync(user); + } + + model.SharedKey = FormatKey(unformattedKey); + model.AuthenticatorUri = GenerateQrCodeUri(user.Email, unformattedKey); + } + } diff --git a/src/Web/ViewModels/Manage/EnableAuthenticatorViewModel.cs b/src/Web/ViewModels/Manage/EnableAuthenticatorViewModel.cs index 2475048..f59f364 100644 --- a/src/Web/ViewModels/Manage/EnableAuthenticatorViewModel.cs +++ b/src/Web/ViewModels/Manage/EnableAuthenticatorViewModel.cs @@ -1,5 +1,6 @@ using System.ComponentModel; using System.ComponentModel.DataAnnotations; +using Microsoft.AspNetCore.Mvc.ModelBinding; namespace Microsoft.eShopWeb.Web.ViewModels.Manage; @@ -11,8 +12,9 @@ public class EnableAuthenticatorViewModel [Display(Name = "Verification Code")] public string Code { get; set; } - [ReadOnly(true)] + [BindNever] public string SharedKey { get; set; } + [BindNever] public string AuthenticatorUri { get; set; } } diff --git a/src/Web/ViewModels/Manage/GenerateRecoveryCodesViewModel.cs b/src/Web/ViewModels/Manage/ShowRecoveryCodesViewModel.cs similarity index 71% rename from src/Web/ViewModels/Manage/GenerateRecoveryCodesViewModel.cs rename to src/Web/ViewModels/Manage/ShowRecoveryCodesViewModel.cs index b130a25..6a2b561 100644 --- a/src/Web/ViewModels/Manage/GenerateRecoveryCodesViewModel.cs +++ b/src/Web/ViewModels/Manage/ShowRecoveryCodesViewModel.cs @@ -1,6 +1,7 @@ namespace Microsoft.eShopWeb.Web.ViewModels.Manage; -public class GenerateRecoveryCodesViewModel +public class ShowRecoveryCodesViewModel { public string[] RecoveryCodes { get; set; } } + diff --git a/src/Web/Views/Manage/EnableAuthenticator.cshtml b/src/Web/Views/Manage/EnableAuthenticator.cshtml index 9c17ea9..f6203a2 100644 --- a/src/Web/Views/Manage/EnableAuthenticator.cshtml +++ b/src/Web/Views/Manage/EnableAuthenticator.cshtml @@ -23,7 +23,7 @@

Scan the QR Code or enter this key @Model.SharedKey into your two factor authenticator app. Spaces and casing do not matter.

To enable QR code generation please read our documentation.
-
+

  • diff --git a/src/Web/Views/Manage/GenerateRecoveryCodes.cshtml b/src/Web/Views/Manage/GenerateRecoveryCodes.cshtml index 669d13e..a4234e9 100644 --- a/src/Web/Views/Manage/GenerateRecoveryCodes.cshtml +++ b/src/Web/Views/Manage/GenerateRecoveryCodes.cshtml @@ -1,24 +1,26 @@ -@model GenerateRecoveryCodesViewModel -@{ - ViewData["Title"] = "Recovery codes"; +@{ + ViewData["Title"] = "Generate two-factor authentication (2FA) recovery codes"; ViewData.AddActivePage(ManageNavPages.TwoFactorAuthentication); } -

    @ViewData["Title"]

    +

    @ViewData["Title"]

    + -
    -
    - @for (var row = 0; row < Model.RecoveryCodes.Count(); row += 2) - { - @Model.RecoveryCodes[row] @Model.RecoveryCodes[row + 1]
    - } -
    + +
    +
    + +
    \ No newline at end of file diff --git a/src/Web/Views/Manage/ShowRecoverCodes.cshtml b/src/Web/Views/Manage/ShowRecoverCodes.cshtml new file mode 100644 index 0000000..ed6bc95 --- /dev/null +++ b/src/Web/Views/Manage/ShowRecoverCodes.cshtml @@ -0,0 +1,25 @@ +@model ShowRecoveryCodesViewModel +@{ + ViewData["Title"] = "Recovery codes"; + ViewData.AddActivePage(ManageNavPages.TwoFactorAuthentication); +} + +

    @ViewData["Title"]

    + +
    +
    + @for (var row = 0; row < Model.RecoveryCodes.Length; row += 2) + { + @Model.RecoveryCodes[row] @Model.RecoveryCodes[row + 1]
    + } +
    +
    +© 2021 GitHub, Inc. \ No newline at end of file diff --git a/src/Web/Views/Manage/TwoFactorAuthentication.cshtml b/src/Web/Views/Manage/TwoFactorAuthentication.cshtml index 8e75bab..2fdd95e 100644 --- a/src/Web/Views/Manage/TwoFactorAuthentication.cshtml +++ b/src/Web/Views/Manage/TwoFactorAuthentication.cshtml @@ -30,7 +30,7 @@ } Disable 2FA - Reset recovery codes + Reset recovery codes }
    Authenticator app