Is it ok to use .Replace(“ ”, “+”) when reading hashes in a querystring?

Question

I am letting users change their password after a reset, by following a link containing hashes of the password and user name (e-mail address). The link can look like this:

www.example.domain/login?h1=8tGecrXvKJBOhtzvyDmJNjpLF5RF3Ed+QSkimxlJaFo=&h2=Bv2WmO4uzgrDIRj9scKtMz0Ek0KpyJ3M00wJrMU7oeA=

Note the presence of a "+" in one of the hashes. This will be translated to " ", so in the login-method below, I'm replacing " " with "+":

public async Task<IActionResult> Login(string h1, string h2, Uri returnUrl = null)
{
    // h1 = password
    // h2 = e-mail address
    if (h1 != null && h2 != null)
    {
        h1 = h1.Replace(" ", "+");
        h2 = h2.Replace(" ", "+");
        AdminUser au = await db.AdminUsers
            .Include(p => p.Person)
            .Where(u => 
                u.PasswordHash == h1 &&
                HashPassword(u.Person.Email1, Convert.FromBase64String(u.PasswordSalt)) == h2)
            .FirstOrDefaultAsync().ConfigureAwait(false);
        if (au != null)
        {
            return RedirectToAction("ChangePassword", "Admin", new { id = au.PersonId, userType = "au" });
        }
        // Some unfinished business here (if (au == null))
    }
    LoginFormViewModel vm = new LoginFormViewModel
    {
        ReturnUrl = returnUrl
    };
    return View(vm);
}

This has worked so far in my tests, but I don't know if I might run into problems because of the .Replace() in some scenario. If a hash contains "/" or "=", there is no problem.

Answer 1

When passing arguments in an URL, they should be appropriately encoded

Answer 2

This has obviously nothing to do with hashes - it has everything to do with queries that might contain a "+" character. For example a search for "Joe's + Jim's Coffee Shop" or often for any base-64 encoded data. So think of a general solution. The code for this should absolutely not be in your "login" method, but you should have a separate method that does its best to return the contents of a query string correctly.

In my experience, there is quite a bit of variation how the data may arrive at your code. There may be an original space character that was for some reason not encoded and arrives at your code as a space character. Or it might arrive as a "+" character. A "+" character might arrive percent encoded or as a "+" character. If you're really unlucky "+" character might arrive as a twice percent encoded plus character. You want code that handles correct encodings correctly, and that handles incorrect encodings correctly if it can figure out what it is, and if it can't figure it out, you can only hope for the best.

So you write this decoding function. In your case you expect base-64 encoded data. Which means there is no percent, no space, but there may be "+" characters, in the correctly decoded data. Your incoming data should only contain space characters and percent-encoded data, but you may accept "+" characters. You should probably log anything that looks dodgy, like having both "+" and space characters, and investigate it.

There are other situations. If you expect data that could legally contain both "+" and space characters, then your incoming data should contain percent-encode "+" characters (which become "+" characters), and plain "+" characters (which become space characters). If you have neither or both, everything is most most likely fine. If you have only "+" characters you can only pray that the query arrived fine at your app. If there are reasons why a space or plus character should not appear at some point, you'll have to decide. For example, phone numbers rarely start with a space but often contain space characters; they often start with a "+" put rarely contain "+" characters.

(I have encountered especially e-mail clients that did absolutely stupid things with percent and plus encoding, so log anything that is suspicious).