abpframework / abp

Open Source Web Application Framework for ASP.NET Core. Offers an opinionated architecture to build enterprise software solutions with best practices on top of the .NET and the ASP.NET Core platforms. Provides the fundamental infrastructure, production-ready startup templates, application modules, UI themes, tooling, guides and documentation.
https://abp.io
GNU Lesser General Public License v3.0
12.31k stars 3.32k forks source link

Default "Lockedout" error description makes it susceptible to user enumeration attacks #17803

Closed tinonetic closed 7 months ago

tinonetic commented 7 months ago
  1. ABP 7.3
  2. Angular
  3. EF Core
  4. Response Message: { "result":4, "description": "Lockedout" }

Steps needed to reproduce the problem. 1) Register users through /account/login or /account/register 2) Try logging in with the wrong password until you reach the maximum number of tries 3) You will get the message: { "result":4, "description": "Lockedout" }

We had a penetration tester who did a security audit for our public facing APIs.

They flagged the issue above as a vulnerability to a user enumeration attack. If a malicious actor is enumerating through a possible user list, the message allows them to identify and confirm an existing account. When account is unlocked, attacker can now focus on the next step, which is guessing the passwords of confirmed user accounts

What I have looked at:

  1. Found the Account Resource file. It has a UserLockedOutMessage resource string, but it's default value is "The user account has been locked out due to invalid login attempts. Please wait a while and try again." image

  2. This is used by Login.chtml.cs

  3. Found a call to return LoginResultType

  4. Found that LockedOutType also exists in both TypeScript and C# code

Can't seem to locate where I can customize this value.

Only workaround I can think of would be overriding the exception that generates the response and returning a different response type

https://github.com/abpframework/abp/issues/16893#issuecomment-1617948830

Is there a better way of intercepting/customizing this response?

If not, would you consider it as a new feature?

maliming commented 7 months ago

hi

We follow the behavior of ASP NET Core Identity.

https://github.com/dotnet/aspnetcore/blob/release/8.0-rc1/src/Identity/Core/src/IdentityApiEndpointRouteBuilderExtensions.cs#L117 https://github.com/dotnet/aspnetcore/blob/release/8.0-rc1/src/Identity/UI/src/Areas/Identity/Pages/V4/Account/Login.cshtml.cs#L143 https://github.com/dotnet/aspnetcore/blob/release/8.0-rc1/src/Identity/UI/src/Areas/Identity/Pages/V4/Account/Lockout.cshtml#L9

tinonetic commented 7 months ago

Is there a better way of intercepting/customizing this response?

If not, would you consider it as a new feature?

maliming commented 7 months ago

hi

This is a good way.

https://github.com/abpframework/abp/issues/16893#issuecomment-1617948830

I don't think there is any need to change.

tinonetic commented 7 months ago

hi

This is a good way.

#16893 (comment)

I don't think there is any need to change.

It turns out the response is not a result of an exception.

The Login response is sent by the AccountContoller:

So what has worked is overriding the AccountController using the documented way and @hikalkan 's example

So final, working code, placed in the MyApp.HttpApi project, in the Controllers folder is


    [Dependency(ReplaceServices = true)]
    [ExposeServices(typeof(AccountController))]
    public class MyAppAccountController : AccountController
    {
        public MyAppAccountController(SignInManager<Volo.Abp.Identity.IdentityUser> signInManager, IdentityUserManager userManager, ISettingProvider settingProvider, IdentitySecurityLogManager identitySecurityLogManager, IOptions<IdentityOptions> identityOptions) : base(signInManager, userManager, settingProvider, identitySecurityLogManager, identityOptions)
        {
        }

        public override async Task<AbpLoginResult> Login(Volo.Abp.Account.Web.Areas.Account.Controllers.Models.UserLoginInfo login)
        {
            var loginResult = await base.Login(login);
            if (loginResult.Result == LoginResultType.LockedOut)
            {
                return new AbpLoginResult(LoginResultType.InvalidUserNameOrPassword);
            }
            return loginResult;
        }
    }
tinonetic commented 7 months ago

hi This is a good way. #16893 (comment) I don't think there is any need to change.

It turns out the response is not a result of an exception.

The Login response is sent by the AccountContoller:

So what has worked is overriding the AccountController using the documented way and @hikalkan 's example

So final, working code, placed in the MyApp.HttpApi project, in the Controllers folder is

    [Dependency(ReplaceServices = true)]
    [ExposeServices(typeof(AccountController))]
    public class MyAppAccountController : AccountController
    {
        public MyAppAccountController(SignInManager<Volo.Abp.Identity.IdentityUser> signInManager, IdentityUserManager userManager, ISettingProvider settingProvider, IdentitySecurityLogManager identitySecurityLogManager, IOptions<IdentityOptions> identityOptions) : base(signInManager, userManager, settingProvider, identitySecurityLogManager, identityOptions)
        {
        }

        public override async Task<AbpLoginResult> Login(Volo.Abp.Account.Web.Areas.Account.Controllers.Models.UserLoginInfo login)
        {
            var loginResult = await base.Login(login);
            if (loginResult.Result == LoginResultType.LockedOut)
            {
                return new AbpLoginResult(LoginResultType.InvalidUserNameOrPassword);
            }
            return loginResult;
        }
    }

Unfortunately, this isn't working. I added the class to the MyApp.HttpApi project in the Controllers folder

Any reason why it wouldn't work?

maliming commented 7 months ago

Is the Login method of MyAppAccountController hit?

tinonetic commented 7 months ago

It's not hit. I added a Log message and a break point and it's not hitting.

Do I need to add something in the module class?

maliming commented 7 months ago

You can share a template project with your code on GitHub.

tinonetic commented 7 months ago

Ok. Let me generate it

tinonetic commented 7 months ago

@maliming , the repo for the sample is here: https://github.com/tinonetic/abp-controller-override

The custom AccountController is: https://github.com/tinonetic/abp-controller-override/blob/main/src/MyApp.HttpApi/Controllers/MyAppAccountController.cs

maliming commented 7 months ago
[Dependency(ReplaceServices = true)]
[ExposeServices(typeof(AccountController), IncludeSelf = true)]
public class MyAppAccountController : AccountController
{
    public MyAppAccountController(SignInManager<Volo.Abp.Identity.IdentityUser> signInManager, IdentityUserManager userManager, ISettingProvider settingProvider, IdentitySecurityLogManager identitySecurityLogManager, IOptions<IdentityOptions> identityOptions)
        : base(signInManager, userManager, settingProvider, identitySecurityLogManager, identityOptions)
    {
    }

    public override async Task<AbpLoginResult> Login(Volo.Abp.Account.Web.Areas.Account.Controllers.Models.UserLoginInfo login)
    {
        Logger.LogInformation("#### Working");
        var loginResult = await base.Login(login);
        if (loginResult.Result == LoginResultType.LockedOut)
        {
            return new AbpLoginResult(LoginResultType.InvalidUserNameOrPassword);
        }
        return loginResult;
    }
}
tinonetic commented 7 months ago
[Dependency(ReplaceServices = true)]
[ExposeServices(typeof(AccountController), IncludeSelf = true)]
public class MyAppAccountController : AccountController
{
    public MyAppAccountController(SignInManager<Volo.Abp.Identity.IdentityUser> signInManager, IdentityUserManager userManager, ISettingProvider settingProvider, IdentitySecurityLogManager identitySecurityLogManager, IOptions<IdentityOptions> identityOptions)
        : base(signInManager, userManager, settingProvider, identitySecurityLogManager, identityOptions)
    {
    }

    public override async Task<AbpLoginResult> Login(Volo.Abp.Account.Web.Areas.Account.Controllers.Models.UserLoginInfo login)
    {
        Logger.LogInformation("#### Working");
        var loginResult = await base.Login(login);
        if (loginResult.Result == LoginResultType.LockedOut)
        {
            return new AbpLoginResult(LoginResultType.InvalidUserNameOrPassword);
        }
        return loginResult;
    }
}

Thanks, but it's not working. I tried adding a breakpoint, it doesn't hit it. Also the log entry doesn't show up in the log file.

What else did you do to get it to work

maliming commented 7 months ago
image
tinonetic commented 7 months ago

Oh. You are right.

But it looks like I was mistaken and I thought this would fix the issue for the UI as well.

What do I modify to intercept the response that goes to the UI?

image

maliming commented 7 months ago

The page in your screenshot is https://github.com/abpframework/abp/blob/dev/modules/account/src/Volo.Abp.Account.Web/Pages/Account/Login.cshtml.cs#L84

tinonetic commented 7 months ago

The page in your screenshot is https://github.com/abpframework/abp/blob/dev/modules/account/src/Volo.Abp.Account.Web/Pages/Account/Login.cshtml.cs#L84

Thank-you. Trying it now

tinonetic commented 7 months ago

Just tried replacing the LoginModel: https://github.com/tinonetic/abp-controller-override/blob/main/src/MyApp.HttpApi/Models/MyAppLoginModel.cs

Then tried replacing it in the Module: https://github.com/tinonetic/abp-controller-override/blob/fac1a5cc155dd7ae7794032d3eed111e31629433/src/MyApp.HttpApi/MyAppHttpApiModule.cs#L36C6-L36C6

It's not hit.

I would also still need to find out how to replace this JSON response: { "result":4, "description": "Lockedout" }

I feel like I'm going round in circles... no fault of yours.

maliming commented 7 months ago

hi

Please move MyAppLoginModel to MyApp.HttpApi.Host project.

using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Identity;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Options;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Volo.Abp.Account.Web;
using Volo.Abp.Account.Web.Pages.Account;
using Volo.Abp.DependencyInjection;
using Volo.Abp.OpenIddict;

namespace MyApp.Models;

[Dependency(ReplaceServices = true)]
[ExposeServices(typeof (LoginModel), typeof(OpenIddictSupportedLoginModel))]
public class MyAppLoginModel : OpenIddictSupportedLoginModel
{
    public MyAppLoginModel(IAuthenticationSchemeProvider schemeProvider, IOptions<AbpAccountOptions> accountOptions,
        IOptions<IdentityOptions> identityOptions, AbpOpenIddictRequestHelper openIddictRequestHelper) : base(
        schemeProvider, accountOptions, identityOptions, openIddictRequestHelper)
    {
    }

    public override async Task<IActionResult> OnPostAsync(string action)
    {
        var result = await base.OnPostAsync(action);
        if (Alerts.Any(a => a.Text == L["UserLockedOutMessage"]))
        {
            Alerts.Clear();
            Alerts.Danger(L["InvalidUserNameOrPassword"]);
        }

        return result;
    }
}
tinonetic commented 7 months ago

hi

Please move MyAppLoginModel to MyApp.HttpApi.Host project.

using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Identity;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Options;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Volo.Abp.Account.Web;
using Volo.Abp.Account.Web.Pages.Account;
using Volo.Abp.DependencyInjection;
using Volo.Abp.OpenIddict;

namespace MyApp.Models;

[Dependency(ReplaceServices = true)]
[ExposeServices(typeof (LoginModel), typeof(OpenIddictSupportedLoginModel))]
public class MyAppLoginModel : OpenIddictSupportedLoginModel
{
    public MyAppLoginModel(IAuthenticationSchemeProvider schemeProvider, IOptions<AbpAccountOptions> accountOptions,
        IOptions<IdentityOptions> identityOptions, AbpOpenIddictRequestHelper openIddictRequestHelper) : base(
        schemeProvider, accountOptions, identityOptions, openIddictRequestHelper)
    {
    }

    public override async Task<IActionResult> OnPostAsync(string action)
    {
        var result = await base.OnPostAsync(action);
        if (Alerts.Any(a => a.Text == L["UserLockedOutMessage"]))
        {
            Alerts.Clear();
            Alerts.Danger(L["InvalidUserNameOrPassword"]);
        }

        return result;
    }
}

Thank-you!

This did it!