Share via

Need to make class Serializable - vulnerability found

Question

Friday, December 8, 2017 3:52 PM

Hello folks, I have a situation. I'm trying to fix a problem with my code not being serializable. According to Fortify SSC,

"
ASP.NET Bad Practices
Non-Serializable Object Stored in Session.
Time and State, Structural
The method x() in ADGroupsToUserscs stores a non-serializable object as an HttpSessionState attribute on line 39, which can damage application reliability.
"

    [Serializable]
    public class ADGroupsToUsers
    {
        private static Hashtable searchedGroups = null;

        static public ArrayList x(string strGroupName)
        {
            ArrayList groupMembers = new ArrayList();
            searchedGroups = new Hashtable();

            // find group
            DirectorySearcher search = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            search.Filter = String.Format("(&(objectCategory=group)(cn={0}))", strGroupName);
            search.PropertiesToLoad.Add("distinguishedName");
            search.PropertiesToLoad.Add("GivenName");
            search.PropertiesToLoad.Add("sn");
            SearchResult sru = null;
            DirectoryEntry group;

            try
            {
                sru = search.FindOne();
            }
            catch (Exception ex)
            {
                throw ex;
            }
            group = sru.GetDirectoryEntry();

            groupMembers = getUsersInGroup(group.Properties["distinguishedName"].Value.ToString());
            HttpContext.Current.Session["ADUserReturn"] = groupMembers; // Fortify complains about this line
            return groupMembers;
        }

All replies (13)

Monday, December 11, 2017 1:43 PM ✅Answered

As you are still using ArrayList it is still unclear what is stored in Session["ADUserReturn"];

Do you still store AD objects? Once again the idea would be to just store plain simple .NET objects with just a copy of the information you need (just the group names to which the user belong ?). In this case ADUserReturn could be perhaps just a list of strings. Don't you use only the group name ?

Once done other enhancements could be to hide that behind an API such as https://msdn.microsoft.com/en-us/library/dn613290(v=vs.108).aspx and maybe even later to really use an ASP.NET Identity implementation on top of AD.

Friday, December 8, 2017 5:26 PM

You'll need to take a look at the getUsersInGroup method as this method is the source of the warning.

getUsersInGroup(group.Properties["distinguishedName"].Value.ToString())

Basically, the message means you tied yourself to Session and cannot move away from Session until object is made serializable.  At this point, there is not much anyone can do you provide assistance since we cannot see the code that creates the object.  Also consider moving away from an ArrayList and use a typed collection like a List.

Also Fortify SSC is probably a better place to get assistance as they can better explain what you need to do in order to get the C# code to pass.

Friday, December 8, 2017 5:27 PM

Hi,

My guess is that get code as you are using ArrayList which should be preferred only when storing objects whose type can vary from one to the next. Else you could use List<MyType> instead.

How to fixx this dépends on what you are doing with that later. Rather than keeping in memory AD objects you could just store the exact information(s) you need.

Friday, December 8, 2017 5:36 PM

You are correct, this is returning a list of AD Users. The code pulls them from the AD and then displays them in a grid view.

This is the entirety of the helper class code. Let me know if you need to see that page as well. Happy to post it.

using System;
using System.Collections;
using System.Configuration;
using System.DirectoryServices;
using System.Web;

namespace ADListing.App_Code
{
    [Serializable]
    public class ADGroupsToUserNames
    {
        private static Hashtable searchedGroups = null;

        static public ArrayList x(string strGroupName)
        {
            ArrayList groupMembers = new ArrayList();
            searchedGroups = new Hashtable();

            // find group
            DirectorySearcher search = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            search.Filter = String.Format("(&(objectCategory=group)(cn={0}))", strGroupName);
            search.PropertiesToLoad.Add("distinguishedName");
            search.PropertiesToLoad.Add("GivenName");
            search.PropertiesToLoad.Add("sn");
            SearchResult sru = null;
            DirectoryEntry group;

            try
            {
                sru = search.FindOne();
            }
            catch (Exception ex)
            {
                throw ex;
            }
            group = sru.GetDirectoryEntry();

            groupMembers = getUsersInGroup(group.Properties["distinguishedName"].Value.ToString());
            HttpContext.Current.Session["ADUserReturn"] = groupMembers;
            return groupMembers;
        }

        private static ArrayList getUsersInGroup(string strGroupDN)
        {
            ArrayList groupMembers = new ArrayList();
            searchedGroups.Add(strGroupDN, strGroupDN);

            // find all users in this group
            DirectorySearcher ds = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            ds.Filter = String.Format
                        ("(&(memberOf={0})(objectClass=person))", strGroupDN);

            ds.PropertiesToLoad.Add("distinguishedName");
            ds.PropertiesToLoad.Add("givenname");
            ds.PropertiesToLoad.Add("samaccountname");
            ds.PropertiesToLoad.Add("sn");

            foreach (SearchResult sr in ds.FindAll())
            {
                groupMembers.Add(sr.Properties["samaccountname"][0].ToString());
                groupMembers.Add(sr.Properties["givenname"][0].ToString());
                groupMembers.Add(sr.Properties["sn"][0].ToString());
            }

            // get nested groups
            ArrayList al = getNestedGroups(strGroupDN);
            foreach (object g in al)
            {
                // only if we haven't searched this group before - avoid endless loops
                if (!searchedGroups.ContainsKey(g))
                {
                    // get members in nested group
                    ArrayList ml = getUsersInGroup(g as string);
                    // add them to result list
                    foreach (object s in ml)
                    {
                        groupMembers.Add(s as string);
                    }
                }
            }

            return groupMembers;
        }

        private static ArrayList getNestedGroups(string strGroupDN)
        {
            ArrayList groupMembers = new ArrayList();

            // find all nested groups in this group
            DirectorySearcher ds = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            ds.Filter = String.Format
                        ("(&(memberOf={0})(objectClass=group))", strGroupDN);

            ds.PropertiesToLoad.Add("distinguishedName");

            foreach (SearchResult sr in ds.FindAll())
            {
                groupMembers.Add(sr.Properties["distinguishedName"][0].ToString());
            }
            return groupMembers;
        }
    }
}

Friday, December 8, 2017 5:50 PM

You'll need to create an object model using POCOs to store the AD information rather than ArrayLists and HashTables.  Most likely that's going to be a lot of work as the change will ripple through the app.  I have the same issue in an app I support.  It was created a long time ago and uses HashTables and ArrayLists for everything which was fine at the time it was created.

Friday, December 8, 2017 6:54 PM

Ok, I am working up the POCO. Here is what I have so far:

using System;
using System.Web.SessionState;

namespace ADLookups.App_Code
{
    [Serializable]
    public class SerializeADGroups
    {
        private string DistName = string.Empty;
        private string DistValue = string.Empty;

        public void AddToSession(HttpSessionState session)
        {
            session["DistName"] = DistName.ToString();
            session["DistValue"] = DistValue.ToString();
        }
    }
}

Now, my big question is how do I get my array list which contains the users in those AD groups to work with this POCO I have wired up? I've not used this before so I'm a little out of my element. Of course I may be going about this completely wrong; that's always possible. A little help please :)

Friday, December 8, 2017 7:21 PM

POCO is Plain Old CLR Object and is just a basic class with public (usually public) properties.

    [Serializable]
    public class SerializeADGroups
    {
        public string DistName { get; set; }
        public string DistValue { get; set; }
    }

You do not want Session in your POCOs as that ties the POCO to a web app that implements Session.  IMHO, Session is generally a bad idea in modern web app anyway.

To create a collection of POCOs it is common to use a generic list.

public List<SerializeADGroups> adGroups = new List<SerializeADGroups>();

Then added the list (above) to Session in the web app is just.

session["adGroups"] = adGroups

You can learn about Generics and POCOs in the C# programming guide.

/en-us/dotnet/csharp/programming-guide/generics/

Friday, December 8, 2017 8:45 PM

So, using this architecture, how do I go about using this code to perform this comparison operation? I'm trying to make sure the username is in the list of users in an AD Group, like so -

...

   ADGroupsToUsers.x("AllMarketing");
   ArrayList ADUsers = new ArrayList();
   ADUsers = (ArrayList)Session["ADUsersReturn"];
   string MyUser = User.Identity.Name.ToString();
   string strUser = MyUser.Substring(3); // to strip off domain context
   Boolean isStringExists = ADUsers.Contains(strUser.ToString());

ADGroupsToUsers.cs

    public class ADGroupsToUsers
    {
        private static Hashtable searchedGroups = null;
        public List<SerializeADGroups> adGroups = new List<SerializeADGroups>();

        static public ArrayList x(string strGroupName)
        {
            ArrayList groupMembers = new ArrayList();
            searchedGroups = new Hashtable();

            // find group
            DirectorySearcher search = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            search.Filter = String.Format("(&(objectCategory=group)(cn={0}))", strGroupName);
            search.PropertiesToLoad.Add("distinguishedName");
            search.PropertiesToLoad.Add("GivenName");
            search.PropertiesToLoad.Add("sn");
            SearchResult sru = null;
            DirectoryEntry group;

            try
            {
                sru = search.FindOne();
            }
            catch (Exception ex)
            {
                throw ex;
            }
            group = sru.GetDirectoryEntry();
            groupMembers = getUsersInGroup(group.Properties["distinguishedName"].Value.ToString());
            // HttpContext.Current.Session["adGroups"] = groupMembers;
            HttpContext.Current.Session["ADUsersReturn"] = groupMembers; 

// Note:
// When I the line as HttpContext.Current.Session["ADGroups"] = groupMembers it said
// the session was empty

            return groupMembers;
        }

        private static ArrayList getUsersInGroup(string strGroupDN)
        {
            ArrayList groupMembers = new ArrayList();
            searchedGroups.Add(strGroupDN, strGroupDN);

            // find all users in this group
            DirectorySearcher ds = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            ds.Filter = String.Format
                        ("(&(memberOf={0})(objectClass=person))", strGroupDN);

            ds.PropertiesToLoad.Add("distinguishedName");
            ds.PropertiesToLoad.Add("givenname");
            ds.PropertiesToLoad.Add("samaccountname");
            ds.PropertiesToLoad.Add("sn");

            foreach (SearchResult sr in ds.FindAll())
            {
                groupMembers.Add(sr.Properties["samaccountname"][0].ToString());
                groupMembers.Add(sr.Properties["givenname"][0].ToString());
                groupMembers.Add(sr.Properties["sn"][0].ToString());
            }

            // get nested groups
            ArrayList al = getNestedGroups(strGroupDN);
            foreach (object g in al)
            {
                // only if we haven't searched this group before - avoid endless loops
                if (!searchedGroups.ContainsKey(g))
                {
                    // get members in nested group
                    ArrayList ml = getUsersInGroup(g as string);
                    // add them to result list
                    foreach (object s in ml)
                    {
                        groupMembers.Add(s as string);
                    }
                }
            }

            return groupMembers;
        }

        private static ArrayList getNestedGroups(string strGroupDN)
        {
            ArrayList groupMembers = new ArrayList();

            // find all nested groups in this group
            DirectorySearcher ds = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            ds.Filter = String.Format
                        ("(&(memberOf={0})(objectClass=group))", strGroupDN);

            ds.PropertiesToLoad.Add("distinguishedName");

            foreach (SearchResult sr in ds.FindAll())
            {
                groupMembers.Add(sr.Properties["distinguishedName"][0].ToString());
            }
            return groupMembers;
        }
    }

Saturday, December 9, 2017 12:08 AM

So, using this architecture, how do I go about using this code to perform this comparison operation? I'm trying to make sure the username is in the list of users in an AD Group, like so -

It's the same "architecture" that you're currently using except taking full advantage of the strongly typed nature of .NET.

var adUser = ADUsers.Where(m => m.username == strUser.ToString());

/en-us/dotnet/csharp/programming-guide/concepts/linq/getting-started-with-linq

/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/lambda-expressions

Saturday, December 9, 2017 1:02 AM

I'll give it a shot on Monday, thanks!

Monday, December 11, 2017 1:07 PM

Ok, keeping this code below in mind, how should this be re-written to accommodate the Linq query you posted? I'm still having trouble. Thanks!

    ADGroupsToUsers.x("Marketing");
    ArrayList ADUsers = new ArrayList();
    ADUsers = (ArrayList)Session["ADUserReturn"];
    string MyUser = User.Identity.Name.ToString();
    string strUser = MyUser.Substring(3);
    var adUser = ADUsers.Where(m => m.ADUsers == strUser.ToString());
    Boolean isStringExists = ADUsers.Contains(strUser.ToString());

Monday, December 11, 2017 3:51 PM

The way the class works is it takes the primary group name, breaks that out into the subgroups. Then it determines the users in those groups and subgroups and returns those as an Array List. This is the helper class, maybe you can help me to rewrite it so it is not storing the AD Objects?

using System;
using System.Collections;
using System.Collections.Generic;
using System.Configuration;
using System.DirectoryServices;
using System.Web;

namespace ADGroupLookup.App_Code
{
    public class ADGroupsToUsers
    {
        private static Hashtable searchedGroups = null;
        public List<SerializeADGroups> adGroups = new List<SerializeADGroups>();

        static public ArrayList x(string strGroupName)
        {
            ArrayList groupMembers = new ArrayList();
            searchedGroups = new Hashtable();

            // find group
            DirectorySearcher search = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            search.Filter = String.Format("(&(objectCategory=group)(cn={0}))", strGroupName);
            search.PropertiesToLoad.Add("distinguishedName");
            search.PropertiesToLoad.Add("GivenName");
            search.PropertiesToLoad.Add("sn");
            SearchResult sru = null;
            DirectoryEntry group;

            try
            {
                sru = search.FindOne();
            }
            catch (Exception ex)
            {
                throw ex;
            }
            group = sru.GetDirectoryEntry();
            groupMembers = getUsersInGroup(group.Properties["distinguishedName"].Value.ToString());
            HttpContext.Current.Session["ADUserReturn"] = groupMembers;
            return groupMembers;
        }

        private static ArrayList getUsersInGroup(string strGroupDN)
        {
            ArrayList groupMembers = new ArrayList();
            searchedGroups.Add(strGroupDN, strGroupDN);

            // find all Users in this group
            DirectorySearcher ds = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            ds.Filter = String.Format
                        ("(&(memberOf={0})(objectClass=person))", strGroupDN);

            ds.PropertiesToLoad.Add("distinguishedName");
            ds.PropertiesToLoad.Add("givenname");
            ds.PropertiesToLoad.Add("samaccountname");
            ds.PropertiesToLoad.Add("sn");

            foreach (SearchResult sr in ds.FindAll())
            {
                groupMembers.Add(sr.Properties["samaccountname"][0].ToString());
                groupMembers.Add(sr.Properties["givenname"][0].ToString());
                groupMembers.Add(sr.Properties["sn"][0].ToString());
            }

            // get nested groups
            ArrayList al = getNestedGroups(strGroupDN);
            foreach (object g in al)
            {
                // only if we haven't searched this group before - avoid endless loops
                if (!searchedGroups.ContainsKey(g))
                {
                    // get members in nested group
                    ArrayList ml = getUsersInGroup(g as string);
                    // add them to result list
                    foreach (object s in ml)
                    {
                        groupMembers.Add(s as string);
                    }
                }
            }

            return groupMembers;
        }

        private static ArrayList getNestedGroups(string strGroupDN)
        {
            ArrayList groupMembers = new ArrayList();

            // find all nested groups in this group
            DirectorySearcher ds = new DirectorySearcher(ConfigurationManager.AppSettings["ADPath"]);
            ds.Filter = String.Format
                        ("(&(memberOf={0})(objectClass=group))", strGroupDN);

            ds.PropertiesToLoad.Add("distinguishedName");

            foreach (SearchResult sr in ds.FindAll())
            {
                groupMembers.Add(sr.Properties["distinguishedName"][0].ToString());
            }
            return groupMembers;
        }
    }
}

Monday, December 11, 2017 3:53 PM

Incidentally, this is the StackOverflow thread that helped me figure this out: https://stackoverflow.com/questions/14234774/how-i-can-find-a-user-in-active-directory-group-with-subgroups