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

@@ -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<ApplicationUser> 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);
+        var model = new EnableAuthenticatorViewModel();
-        if (string.IsNullOrEmpty(unformattedKey))
+        await LoadSharedKeyAndQrCodeUriAsync(user, model);
-        {
-            await _userManager.ResetAuthenticatorKeyAsync(user);
-            unformattedKey = await _userManager.GetAuthenticatorKeyAsync(user);
-        }
-
-        var model = new EnableAuthenticatorViewModel
-        {
-            SharedKey = FormatKey(unformattedKey),
-            AuthenticatorUri = GenerateQrCodeUri(user.Email, unformattedKey)
-        };
 
         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<IActionResult> 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<IActionResult> 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<IActionResult> 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);
+    }
+
 }

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; }
 }

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; }
 }
+

src/Web/Views/Manage/EnableAuthenticator.cshtml

@@ -23,7 +23,7 @@
             <p>Scan the QR Code or enter this key <kbd>@Model.SharedKey</kbd> into your two factor authenticator app. Spaces and casing do not matter.</p>
             <div class="alert alert-info">To enable QR code generation please read our <a href="https://go.microsoft.com/fwlink/?Linkid=852423">documentation</a>.</div>
             <div id="qrCode"></div>
-            <div id="qrCodeData" data-url="@Html.Raw(Model.AuthenticatorUri)"></div>
+            <div id="qrCodeData" data-url="@Model.AuthenticatorUri"></div>
         </li>
         <li>
             <p>

src/Web/Views/Manage/GenerateRecoveryCodes.cshtml

@@ -1,24 +1,26 @@
-@model GenerateRecoveryCodesViewModel
+@{
-@{
+    ViewData["Title"] = "Generate two-factor authentication (2FA) recovery codes";
-    ViewData["Title"] = "Recovery codes";
     ViewData.AddActivePage(ManageNavPages.TwoFactorAuthentication);
 }
 
-<h4>@ViewData["Title"]</h4>
+<h2>@ViewData["Title"]</h2>
+
 <div class="alert alert-warning" role="alert">
     <p>
         <span class="glyphicon glyphicon-warning-sign"></span>
-        <strong>Put these codes in a safe place.</strong>
+        <strong>This action generates new recovery codes.</strong>
     </p>
     <p>
         If you lose your device and don't have the recovery codes you will lose access to your account.
     </p>
+    <p>
+        Generating new recovery codes does not change the keys used in authenticator apps. If you wish to change the key
+        used in an authenticator app you should <a asp-action="ResetAuthenticatorWarning">reset your authenticator keys.</a>
+    </p>
 </div>
-<div class="row">
+
-    <div class="col-md-12">
+<div>
-        @for (var row = 0; row < Model.RecoveryCodes.Count(); row += 2)
+    <form asp-action="GenerateRecoveryCodes" method="post" class="form-group">
-        {
+        <button class="btn btn-danger" type="submit">Generate Recovery Codes</button>
-            <code>@Model.RecoveryCodes[row]</code><text>&nbsp;</text><code>@Model.RecoveryCodes[row + 1]</code><br />
+    </form>
-        }
-    </div>
 </div>

src/Web/Views/Manage/ShowRecoverCodes.cshtml

@@ -0,0 +1,25 @@
+@model ShowRecoveryCodesViewModel
+@{
+    ViewData["Title"] = "Recovery codes";
+    ViewData.AddActivePage(ManageNavPages.TwoFactorAuthentication);
+}
+
+<h4>@ViewData["Title"]</h4>
+<div class="alert alert-warning" role="alert">
+    <p>
+        <span class="glyphicon glyphicon-warning-sign"></span>
+        <strong>Put these codes in a safe place.</strong>
+    </p>
+    <p>
+        If you lose your device and don't have the recovery codes you will lose access to your account.
+    </p>
+</div>
+<div class="row">
+    <div class="col-md-12">
+        @for (var row = 0; row < Model.RecoveryCodes.Length; row += 2)
+        {
+            <code>@Model.RecoveryCodes[row]</code><text>&nbsp;</text><code>@Model.RecoveryCodes[row + 1]</code><br />
+        }
+    </div>
+</div>
+© 2021 GitHub, Inc.

src/Web/Views/Manage/TwoFactorAuthentication.cshtml

@@ -30,7 +30,7 @@
     }
 
     <a asp-action="Disable2faWarning" class="btn btn-default">Disable 2FA</a>
-    <a asp-action="GenerateRecoveryCodes" class="btn btn-default">Reset recovery codes</a>
+    <a asp-action="GenerateRecoveryCodesWarning" class="btn btn-default">Reset recovery codes</a>
 }
 
 <h5>Authenticator app</h5>