Security Risk in User.Migrate

[nethrelm]

If a user with an active session manually goes to any of the login pages (while already being logged in) they can log in again with another account and this will cause the second account to become associated with any groups of the first account.

Example:
Say there are two user accounts: user@domain.xyz, and admin@domain.xyz
- user@domain.xyz is in groups: "_Default_" and "Group A"
- admin@domain.xyz is in groups: "_Default_" and "Admin"
- User logs in as user@domain.xyz
- User manually types "[StoreUrl]/Admin/Login.aspx" in browser
- User logs in as admin@domain.xyz
- admin@domain.xyz is now in groups: "_Default_", "Admin", and "Group A" (I don't want this to happen!)

I think that group migration should only occur when an anonymous user logs in. This way the functionality is retained for anonymous visitors that get automatically placed into groups from custom code, and those groups carry over if the user creates a new account or logs in to an existing account. However, it will prevent cross contamination of groups for existing users.

Suggested fix: Line 793-799 in CommerceBuilder/Users/User.cs should be changed to:
if (oldUser.IsAnonymous)
{
//Lines 793-799
}

I should also point out that this is a potential security risk if an admin user logs in to a non-admin account without logging out, the non-admin account will be placed in the Admin group. Scary!

[jmestep]

Wow, I just tested and saw the same behavior. I'll report a bug.

[Naveed]

Thanks for reporting, Ideally a logout should be called for first user if one already logged in (automatically by code) before attempting logging in the 2nd user.

[jguengerich]

Both the admin login (/Admin/Login.aspx.cs) and the storefront login (/Conlib/LoginDialog.aspx.cs) call migrate. If the source code for migration is changed as nethrelm suggests, then the logout Naveed mentions still should happen in both places. Those that don't have source code and want to implement a fix can put code similar to nethrlem's suggestion, in addition to the logout, in the two .cs files I just mentioned.

Also, I'm wondering should it check for oldUser.IsAnonymous or oldUser.IsAnonymousOrGuest?

[nethrelm]

There are actually several other places where User.Migrate is called that can lead to this issue. If you want to address them all (and you should), here is a list:

Admin/Login.aspx.cs
Checkout/Default.aspx.cs
ConLib/CheckoutLoginDialog.ascx.cs
ConLib/LoginDialog.ascx.cs
Mobile/Checkout/Default.aspx.cs
Mobile/PasswordHelp.aspx.cs
Mobile/UserControls/LoginDialog.ascx.cs
PasswordHelp.aspx.cs

And yes, looking at it again, I believe the best thing to do until a patch is issued is to only call User.Migrate if the migrating user IsAnonymousOrGuest.

[Katie]

Background history:

This issue was introduced into Gold R10. We made a change to fix an issue reported here - viewtopic.php?f=65&t=18058&start=0&hilit=migrate

Someone was customizing Able, and found an issue while trying to add users to special groups if they came in from a referring URL. When the issue was reported, we were hesitant to fix it. Read the developer's note from Issue ID AC8-2628 below:

I am not sure if we need to consider it bug since in order to reproduce it one must need to do some custom code. We don't do any roles assignments to anonymous users out of the box hence there was no code to take care of role migration. But it seems like it would be better if just migrate if there are any roles.

We decided to go ahead and fix the problem for this customer, but testing was limited because we didn't have any customization in place and there wasn't a set of steps to reproduce the original issue. I'm not trying to point fingers. We are 100% responsible for our code changes, and we just didn't test for this particular scenario. In any case, we have reverted the code change and produced a patch for R10 builds. You can download a new CommerceBuilder.dll here -

ftp://ftp.ablecommerce.com/patches/Patc ... ID2836.zip

Thanks for letting us know of the issue. Please get the patch installed asap!

Katie

PS. If you have the source code and wish to fix this in your custom DLL, the code that needs to be removed is from the CommerceBuilder.Users.User.cs files' Migrate method -

Code: Select all

// MIGRATE USER GROUPS IF ANY
                foreach (UserGroup userGroup in oldUser.UserGroups)
                {
                    if (!newUser.IsInGroup(userGroup.GroupId))
                    {
                        newUser.UserGroups.Add(new UserGroup(newUser.Id, userGroup.GroupId));
                    }
                }
                newUser.Save();

[nethrelm]

Yes, I made the original suggestion. However, the migrate method can also transfer affiliates, baskets, wishlists, page views, and order data. The potential data corruption that can result from the migrate method goes well beyond user groups (and hence well beyond the scope of my suggestion). Restricting migration to IsAnonymousOrGuest should prevent erroneous data re-assignments, but further testing should be done to make sure that the solution is sound. I would strongly suggest that the testing team take a look at what occurs following the steps I outlined. I do not think the behavior works as intended, but that is for you to determine.

[Katie]

We completed testing and compared AC7 to Gold. The behavior is almost the same in both. We are not migrating any data (page views, orders, wishlists, affiliates, addresses, basket contents etc.) when a normal user is logged-in while the admin user has an active session.