Identity Server Security Bug: User-controlled bypass of sensitive method

[alexhiggins732]

User-controlled bypass of sensitive method

The original Identity Server 4 code base has several security bugs detected by CodeQL scanning violating this rule.

Description:

Many C# constructs enable code statements to be executed conditionally, for example, if statements and for statements. If the statements contain important authentication or login code, and user-controlled data determines whether or not the code is executed, an attacker may be able to bypass security systems.

Examples

Tool	Rule ID	Source
CodeQL	cs/user-controlled-bypass	/host/Quickstart/Account/AccountController.cs#L95-L9

if (button != "login")
{
	if (context != null)
	{
		// if the user cancels, send a result back into IdentityServer as if they 
		// denied the consent (even if this client does not require consent).
		// this will send back an access denied OIDC error response to the client.
		await _interaction.DenyAuthorizationAsync(context, AuthorizationError.AccessDenied);

		// we can trust model.ReturnUrl since GetAuthorizationContextAsync returned non-null
		if (context.IsNativeClient())
		{
			// The client is native, so this change in how to
			// return the response is for better UX for the end user.
			return this.LoadingPage("Redirect", model.ReturnUrl);
		}

		return Redirect(model.ReturnUrl);
	}
	else
	{
		// since we don't have a valid context, then we just go back to the home page
		return Redirect("~/");
	}
}

if (ModelState.IsValid)
{
	// validate username/password against in-memory store
	if (_users.ValidateCredentials(model.Username, model.Password))
	{
		var user = _users.FindByUsername(model.Username);
		await _events.RaiseAsync(new UserLoginSuccessEvent(user.Username, user.SubjectId, user.Username, clientId: context?.Client.ClientId));

		// only set explicit expiration here if user chooses "remember me". 
		// otherwise we rely upon expiration configured in cookie middleware.
		AuthenticationProperties props = null;
		if (AccountOptions.AllowRememberLogin && model.RememberLogin)
		{
			props = new AuthenticationProperties
			{
				IsPersistent = true,
				ExpiresUtc = DateTimeOffset.UtcNow.Add(AccountOptions.RememberMeLoginDuration)
			};
		};

		// issue authentication cookie with subject ID and username
		var isuser = new IdentityServerUser(user.SubjectId)
		{
			DisplayName = user.Username
		};

		await HttpContext.SignInAsync(isuser, props);

		if (context != null)
		{
			if (context.IsNativeClient())
			{
				// The client is native, so this change in how to
				// return the response is for better UX for the end user.
				return this.LoadingPage("Redirect", model.ReturnUrl);
			}

			// we can trust model.ReturnUrl since GetAuthorizationContextAsync returned non-null
			return Redirect(model.ReturnUrl);
		}

		// request for a local page
		if (Url.IsLocalUrl(model.ReturnUrl))
		{
			return Redirect(model.ReturnUrl);
		}
		else if (string.IsNullOrEmpty(model.ReturnUrl))
		{
			return Redirect("~/");
		}
		else
		{
			// user might have clicked on a malicious link - should be logged
			throw new Exception("invalid return URL");
		}
	}

	await _events.RaiseAsync(new UserLoginFailureEvent(model.Username, "invalid credentials", clientId:context?.Client.ClientId));
	ModelState.AddModelError(string.Empty, AccountOptions.InvalidCredentialsErrorMessage);
}

Issues

• Issue 206 - https://github.com/alexhiggins732/IdentityServer8/security/code-scanning/206
• Issue 207 - https://github.com/alexhiggins732/IdentityServer8/security/code-scanning/207

Recommendation

Never decide whether to authenticate a user based on data that may be controlled by that user. If necessary, ensure that the data is validated extensively when it is input before any authentication checks are performed or authentication flows are performed.

Explicit form actions should be supplied for each user input and the code flow should not depend on the a user-injectable input value.

It is still possible to have a system that "remembers" users, thus not requiring the user to login on every interaction. For example, personalization settings can be applied without authentication because this is not sensitive information. However, users should be allowed to take sensitive actions only when they have been fully authenticated.

Example

This example shows two ways of deciding whether to authenticate a user. The first way shows a decision that is based on the value of a cookie. Cookies can be easily controlled by the user, and so this allows a user to become authenticated without providing valid credentials. The second, more secure way shows a decision that is based on looking up the user in a security database.

public boolean doLogin(HttpCookie adminCookie, String user, String password)
{

    // BAD: login is executed only if the value of 'adminCookie' is 'false',
    // but 'adminCookie' is controlled by the user
    if (adminCookie.Value == "false")
        return login(user, password);

    return true;
}

public boolean doLogin(HttpCookie adminCookie, String user, String password)
{
    // GOOD: use server-side information based on the credentials to decide
    // whether user has privileges
    bool isAdmin = queryDbForAdminStatus(user, password);
    if (!isAdmin)
        return login(user, password);

    return true;
}

This example shows explicit code flows based on the event fired.

 <form asp-route="Login">
     <input type="hidden" asp-for="ReturnUrl" />

     <div class="form-group">
         <label asp-for="Username"></label>
         <input class="form-control" placeholder="Username" asp-for="Username" autofocus>
     </div>
     <div class="form-group">
         <label asp-for="Password"></label>
         <input type="password" class="form-control" placeholder="Password" asp-for="Password" autocomplete="off">
     </div>
     @if (Model.AllowRememberLogin)
     {
         <div class="form-group">
             <div class="form-check">
                 <input class="form-check-input" asp-for="RememberLogin">
                 <label class="form-check-label" asp-for="RememberLogin">
                     Remember My Login
                 </label>
             </div>
         </div>
     }
     <button class="btn btn-primary" name="button" value="login">Login</button>
     <button class="btn btn-secondary" name="button" value="cancel" formaction="/Account/LoginCancel">Cancel</button>
 </form>

References

• Common Weakness Enumeration: CWE-807.
• Common Weakness Enumeration: CWE-247.
• Common Weakness Enumeration: CWE-350.