GenerateChangePhoneNumberTokenAsync is not generating an SMS-friendly token in ASP.NET Core 2.0

[Tratcher]

From @leodip on August 22, 2017 14:53

In ASP.NET Core 1.1, if you call:

var code = _userManager.GenerateChangePhoneNumberTokenAsync(user, "(21) 92345-7154");

... you get back a token that is SMS-friendly, for example: "293688"

In ASP.NET Core 2.0, the same call returns a token that looks like:

"CfDJ8NlUtDo9xLxKnOBUmUICyLsPm6IFT3Xdp9OIBPwggxd2XdFiJxW+3x4DZf4sGlHhyXC4oPMgMVLzq0la6Di+cfWIwopZvzfMFkwx0ThBEOF4xxTVab2xwHSeW54GMxvjiEv9XFDCqjegpw/5y7iR7WiqtB6UNoIk0Hc6VGNkMxwROOuiEDFHy97e7flTDlVwTm9CiovkN3JQC+UDtoOCG+NXnsJ7l+aQ6mzF50aYsWqt8eT2GXhP1sKpD6P9RKuhKiu4Y0m7BBYW8jfE5EKgH4I="

You can't ask the user to type the above token to verify his phone number, because it's too long and complicated.

Is this a known bug?

Thanks

Copied from original issue: dotnet/aspnetcore#2158

[HaoK]

Yeah this looks like an unintended regression caused in the refactoring:

1.1.2 code: https://github.com/aspnet/Identity/blob/rel/1.1.2/src/Microsoft.AspNetCore.Identity/UserManager.cs#L1569

2.0 code that no longer uses the Rfc service which generates the nice ints:
https://github.com/aspnet/Identity/blob/dev/src/Microsoft.Extensions.Identity.Core/UserManager.cs#L1582

The current tests weren't checking the format of the codes, only that they are able to be consumed

@ajcvickers @blowdart this is something we should consider for 2.0.1 as this pretty much will break all existing SMS apps

[HaoK]

Workarounds aren't great since https://github.com/aspnet/Identity/blob/dev/src/Microsoft.Extensions.Identity.Core/Rfc6238AuthenticationService.cs is internal, the best option would probably be for these SMS apps to register a derived UserManager and copy the 1.1.2 implementation and Rfc6238AuthenticationService.cs to override VerifyChangePhoneNumberTokenAsync/GenerateChangePhoneNumberTokenAsync to do what they did in 1.1.2

[jkotalik]

I don't see any tests for the Rfc6238AuthenticationService class (direct unit tests), maybe we should add some in to as it may have caught some of these issues.

[HaoK]

There was no regression in Rfc6238, its the user manager methods no longer use that service, its these tests that are missing one that validates that the token is an integer:

https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNetCore.Identity.Specification.Tests/UserManagerSpecificationTests.cs#L1517

Basically there is no test: "VerifyGenerateChangePhoneNumberTokenIsInteger"

[blowdart]

@Eilon We need to consider this for a patch release.

[ajcvickers]

@HaoK Can you write the Justification/Risk statements needed to get approval for the patch?

[HaoK]

Justification: The Generate/ValidateChangePhoneNumberToken methods generated integer codes in 1.x as their purpose is to be used for SMS codes. These were unintentionally changed to generate long dataprotection strings in 2.0, which are totally not appropriate for SMS two factor codes. Any apps that were using these APIs for SMS code generation would be affected.

Risk: Low, this fix will just returns the code for these two methods back to what it was in 1.x

[Eilon]

@HaoK this bug is marked as 2.0.1, but the PR was to dev. Should this bug be in 2.1.0, and we need to log a separate bug for 2.0.1?

[Eilon]

(Re-opening just to make sure this doesn't get lost.)

[HaoK]

Yeah this was checked into dev, so yeah fixed in 2.1, and I'll file a port issue for 2.0.1

[amirjalali1]

any workaround or news on this issue?
we just moved to 2.0 and encountered this issue.
is there any plan for releasing the patch?
a workaround would be nice.

[Eilon]

@1amirjalai maybe @HaoK can help if there's a possible workaround. We'll have a feed with the patch packages on them for people to test hopefully this week. I'll provide details here when that's available.

[leodip]

Here is what I did for a workaround.

In Startup.cs, where it has:

services.AddIdentity<ApplicationUser, IdentityRole>(o => { ... });

I added:

o.Tokens.ProviderMap.Add("PhoneNumber", new TokenProviderDescriptor(typeof(PhoneNumberTokenProvider<ApplicationUser>)));

In ManageController (method AddPhoneNumber), I generate the token like this:

var token = new VerifyPhoneToken()
            {
                Token = _random.Next().ToString(),
                PhoneNumber = model.PhoneNumber,
                CreatedUTC = DateTime.UtcNow
            };

HttpContext.Session.Set("AddPhoneNumberToken", token);

var code = await _userManager.GenerateUserTokenAsync(user, "PhoneNumber", token.Token + ":" + model.PhoneNumber);  

I also modified this method (for security):

[HttpGet]
        public async Task<IActionResult> VerifyPhoneNumber(string phoneNumber)
        {
            var user = await GetCurrentUserAsync();
            if (user == null)
            {
                return View("Error");
            }

            if (string.IsNullOrWhiteSpace(phoneNumber))
            {
                return View("Error");
            }

            var token = HttpContext.Session.Get<VerifyPhoneToken>("AddPhoneNumberToken");
            if (token == null || token.PhoneNumber != phoneNumber)
            {
                token = new VerifyPhoneToken()
                {
                    Token = _random.Next().ToString(),
                    PhoneNumber = phoneNumber,
                    CreatedUTC = DateTime.UtcNow
                };
                HttpContext.Session.Set("AddPhoneNumberToken", token);
                var code = await _userManager.GenerateUserTokenAsync(user, "PhoneNumber", token.Token + ":" + phoneNumber);
            }

            return View(new VerifyPhoneNumberViewModel { PhoneNumber = phoneNumber });
        }

When it's time to verify, also in ManageController, I do:

[HttpPost]
        [ValidateAntiForgeryToken]
        public async Task<IActionResult> VerifyPhoneNumber(VerifyPhoneNumberViewModel model)
        {
            if (!ModelState.IsValid)
            {
                return View(model);
            }
            var user = await GetCurrentUserAsync();
            if (user != null)
            {
                var token = HttpContext.Session.Get<VerifyPhoneToken>("AddPhoneNumberToken");
                if(token != null && token.CreatedUTC > DateTime.UtcNow.AddMinutes(-10))
                {                    
                    var result = await _userManager.VerifyUserTokenAsync(user, "PhoneNumber", token.Token + ":" + model.PhoneNumber, model.Code);
                    if (result)
                    {
                        // Verification successful  (do whatever you need here)
                        return RedirectToAction(nameof(Index), new { Message = ManageMessageId.AddPhoneSuccess });
                    }
                }
            }
            // If we got this far, something failed, redisplay the form
            ModelState.AddModelError(string.Empty, "Verification failed.");
            return View(model);
        }

There's probably room for improvement, but I can say it works fine, and appears to be secure.

[ptrstpp950]

The workaround is quite simple. Just one line of code is needed

services.AddIdentity<ApplicationUser, IdentityRole>(o =>
{
    //other stuff
    o.Tokens.ChangePhoneNumberTokenProvider = "Phone";
});

[alikuli]

I am using NINJECT and MVC 5. (Microsoft.AspNet.Identity.Core v 2.2.1) I am beginning to get a strange error from GenerateChangePhoneNumberToken (UserId, PhoneNumber) --- Note, the signature is different to the one you have pointed out earlier in this thread.

I get the following error:

Actual Method where error happened 'GenerateChangePhoneNumberToken' --- System.ArgumentNullException: String reference not set to an instance of a String. Parameter name: s at System.Text.Encoding.GetBytes(String s) at Microsoft.AspNet.Identity.UserManager`2.d__ee.MoveNext()

I cant seem to get rid of this error.

Funny thing is, the signature "GenerateEmailConfirmationToken(userId)" is working fine.

[Eilon]

@chadwackerman2 we're working on getting a build of the 2.0.1 patch out to our MyGet feed but we're having some infrastructure issues. I will update all the 2.0.1 issues with package feed details when we have it ready.