locked
Need to make class Serializable - vulnerability found RRS feed

  • Question

  • User-718146471 posted

    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;
            }

    Friday, December 8, 2017 3:52 PM

Answers

  • User753101303 posted

    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.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Monday, December 11, 2017 1:43 PM

All replies

  • User475983607 posted

    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:26 PM
  • User753101303 posted

    Hi,

    My guess is that getUsersInGroup returns a list of AD objects ? We don't see this for sure in the posted 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:27 PM
  • User-718146471 posted

    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:36 PM
  • User475983607 posted

    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 5:50 PM
  • User-718146471 posted

    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 6:54 PM
  • User475983607 posted

    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.

    https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/generics/

    Friday, December 8, 2017 7:21 PM
  • User-718146471 posted

    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;
            }
        }
    
    Friday, December 8, 2017 8:45 PM
  • User475983607 posted

    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());

    https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/linq/getting-started-with-linq

    https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/lambda-expressions

    Saturday, December 9, 2017 12:08 AM
  • User-718146471 posted
    I'll give it a shot on Monday, thanks!
    Saturday, December 9, 2017 1:02 AM
  • User-718146471 posted

    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 1:07 PM
  • User753101303 posted

    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.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Monday, December 11, 2017 1:43 PM
  • User-718146471 posted

    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:51 PM
  • User-718146471 posted

    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

    Monday, December 11, 2017 3:53 PM