Question ReadLine caught in security scan as deserialization of untrusted data

PraveenDasary

Member
Joined
May 17, 2020
Messages
6
Programming Experience
5-10
I have below code implemented in my project..

C#:
public static String LoadTextFile(String path)
    {
        StringBuilder sb = new StringBuilder();
        using (StreamReader reader = new FileInfo(path).OpenText())
        {
            try
            {
                String text = null;
                do
                {
                    text = reader.ReadLine();
                    sb.Append(text);
                } while (text != null);
            }
            catch (Exception ex)
            {
                throw ex;
            }

        }

        return sb.ToString();
    }
Calling Method:
C#:
private MessageConfig LoadConfig()
    {
        string xml = FileUtility.LoadTextFile(Environment.CurrentDirectory + @"\test.config");
        XmlSerializer ser = new XmlSerializer((typeof(MessageConfig )));
        MemoryStream ms = new MemoryStream((new UTF8Encoding()).GetBytes(xml));
        return (MessageConfig )ser.Deserialize(ms);
    }
Violation Message:

The serialized object ReadLine processed in LoadTextFile in the file Test\FileUtility.cs at line 13 is deserialized by Deserialize in the file Test\Simulator.cs at line 368

Though the XmlSerializer deserializing the memory stream to the a predefined type, ReadLine is caught in code scans with above violation. Please suggest any solution..
 

jmcilhinney

C# Forum Moderator
Staff member
Joined
Apr 23, 2011
Messages
2,860
Location
Sydney, Australia
Programming Experience
10+
Firstly, as far as I can tell, you can ditch that LoadTextFile method because it does nothing that the File.ReadAllText method doesn't already do.
C#:
private MessageConfig LoadConfig()
{
    string xml = File.ReadAllText(Path.Combine(Application.StartupPath, "test.config");
    XmlSerializer ser = new XmlSerializer((typeof(MessageConfig )));
    MemoryStream ms = new MemoryStream((new UTF8Encoding()).GetBytes(xml));
    return (MessageConfig )ser.Deserialize(ms);
}
Secondly, there's no point reading text from a file, converting that to a byte array, loading that into a MemoryStream and then passing that to the Deserialize method when you could just create a FileStream and pass that.
C#:
private MessageConfig LoadConfig()
{
    using (var fs = File.OpenRead(Path.Combine(Application.StartupPath, "test.config")))
    {
        XmlSerializer ser = new XmlSerializer((typeof(MessageConfig)));
        
        return (MessageConfig) ser.Deserialize(fs);
    }
}
You should make those changes at least and see whether you still have an issue.
 

jmcilhinney

C# Forum Moderator
Staff member
Joined
Apr 23, 2011
Messages
2,860
Location
Sydney, Australia
Programming Experience
10+
Note that I used Application.StartupPath in that code rather than Environment.CurrentDirectory, which will be correct in most cases. The former will always be the folder path from which the current EXE was run while the latter won't always be that at startup and can change while the app is running. If its a Windows GUI app and you want the path of the program folder, use Application.StartupPath.
 

PraveenDasary

Member
Joined
May 17, 2020
Messages
6
Programming Experience
5-10
Note that I used Application.StartupPath in that code rather than Environment.CurrentDirectory, which will be correct in most cases. The former will always be the folder path from which the current EXE was run while the latter won't always be that at startup and can change while the app is running. If its a Windows GUI app and you want the path of the program folder, use Application.StartupPath.
Hi Thank you for your reply and suggestions. I have made the changes you suggested and re run the scan now it says like this

The serialized object fs processed in LoadConfig in the file Test\Simulator.cs at line 368 is deserialized by Deserialize in the file Test\Simulator.cs at line 368.

Stll is there any problem .. :(

Thank you,
Praveen
 

jmcilhinney

C# Forum Moderator
Staff member
Joined
Apr 23, 2011
Messages
2,860
Location
Sydney, Australia
Programming Experience
10+
I don't really understand that message. It doesn't even really sound like an error message. Can you post the class definition for MessageConfig as well as an example of the file you're trying to deserialise? Was the file created by serialising a MessageConfig object in the first place?
 

PraveenDasary

Member
Joined
May 17, 2020
Messages
6
Programming Experience
5-10
I don't really understand that message. It doesn't even really sound like an error message. Can you post the class definition for MessageConfig as well as an example of the file you're trying to deserialise? Was the file created by serialising a MessageConfig object in the first place?
Let me explain it ... There is a MessageConfig object which is being inherited from a common object DBCommonConfig which has some common properties. So the MessageConfig object looks like below.

C#:
public class DBCommonConfig
    {
        protected string name;
        protected string className;
        protected string dllName;
        
        public DBCommonConfig()
        {
        }

        public string Name
        {
            get
            {
                return name;
            }
            set
            {
                name = value;
            }
        }

        public string ClassName
        {
            get
            {
                return className;
            }
            set
            {
                className = value;
            }
        }

        public string DllName
        {
            get
            {
                return dllName;
            }
            set
            {
                dllName = value;
            }
        }
    }

public class MDBConfig: DBCommonConfig
    {
        private string queue;
        private string channelName;
        private string direction;

        public MDBConfig()
        {
        }
        public Region Region
        {
            get;
            set;
        }

        public string Queue
        {
            get
            {
                return queue;
            }
            set
            {
                queue = value;
            }
        }
        public string ChannelName
        {
            get
            {
                return channelName;
            }
            set
            {
                channelName = value;
            }
        }
}
The config file I am loading from directory looks like below.

XML:
<MessageConfig>
  <!-- ibm mq message driven beans -->
  <MDBsConfig>
    <MDBConfig>
      <Name>SYNCRLLSS</Name>
      <ClassName>MessageDriven.OrderDataRequest.OrderDataMDB</ClassName>
      <DllName>LLM.Business</DllName>
      <Queue>SYNCRO2FSL</Queue>
    </MDBConfig>
  </MDBsConfig>
<MessageConfig>
Then I am simply calling the LoadConfig method with below code
C#:
private MessageConfig LoadConfig()
{
    using (var fs = File.OpenRead(Path.Combine(Application.StartupPath, "test.config")))
    {
        XmlSerializer ser = new XmlSerializer((typeof(MessageConfig)));
        
        return (MessageConfig) ser.Deserialize(fs);
    }
}
I am using CheckMarx for scanning the code and when I scanned, it showed below message catching fs in yellow. CheckMarx says that it is a Deserialization of untrusted data. I am not sure to I can satisfy CheckMarx scan so it will not show this high risk injection.

image 2.PNG


I am directly loading the XML from a file as a FileStream and then deserializing it to MessageConfig object. Is there any serialization needed after loading the config file into fs variable as a FileStream?
 

Skydiver

Staff member
Joined
Apr 6, 2019
Messages
1,227
Location
Virginia Beach, VA
Programming Experience
10+
Our OP is using VeraCode or some other static secure code scanner. The scanner is correctly identifying that the data from the file is being loaded into the XML deserializer and is a potential vulnerability.

My suggestion is go to the related OWASP pages and implement one or more of the mitigating controls. If your scanning software provides links to description of the vulnerability class prefer mitigations listed in those links first since the probability of the scanner detecting that you implemented the mitigating controls is much higher, and even if the the scanner didn't detect the mitigations, you have a very firm leg to stand on to show your security review team that you implementing things are recommended.

 

PraveenDasary

Member
Joined
May 17, 2020
Messages
6
Programming Experience
5-10
Let me explain it ... There is a MessageConfig object which is being inherited from a common object DBCommonConfig which has some common properties. So the MessageConfig object looks like below.

C#:
public class DBCommonConfig
    {
        protected string name;
        protected string className;
        protected string dllName;
      
        public DBCommonConfig()
        {
        }

        public string Name
        {
            get
            {
                return name;
            }
            set
            {
                name = value;
            }
        }

        public string ClassName
        {
            get
            {
                return className;
            }
            set
            {
                className = value;
            }
        }

        public string DllName
        {
            get
            {
                return dllName;
            }
            set
            {
                dllName = value;
            }
        }
    }

public class MDBConfig: DBCommonConfig
    {
        private string queue;
        private string channelName;
        private string direction;

        public MDBConfig()
        {
        }
        public Region Region
        {
            get;
            set;
        }

        public string Queue
        {
            get
            {
                return queue;
            }
            set
            {
                queue = value;
            }
        }
        public string ChannelName
        {
            get
            {
                return channelName;
            }
            set
            {
                channelName = value;
            }
        }
}
The config file I am loading from directory looks like below.

XML:
<MessageConfig>
  <!-- ibm mq message driven beans -->
  <MDBsConfig>
    <MDBConfig>
      <Name>SYNCRLLSS</Name>
      <ClassName>MessageDriven.OrderDataRequest.OrderDataMDB</ClassName>
      <DllName>LLM.Business</DllName>
      <Queue>SYNCRO2FSL</Queue>
    </MDBConfig>
  </MDBsConfig>
<MessageConfig>
Then I am simply calling the LoadConfig method with below code
C#:
private MessageConfig LoadConfig()
{
    using (var fs = File.OpenRead(Path.Combine(Application.StartupPath, "test.config")))
    {
        XmlSerializer ser = new XmlSerializer((typeof(MessageConfig)));
      
        return (MessageConfig) ser.Deserialize(fs);
    }
}
I am using CheckMarx for scanning the code and when I scanned, it showed below message catching fs in yellow. CheckMarx says that it is a Deserialization of untrusted data. I am not sure to I can satisfy CheckMarx scan so it will not show this high risk injection.

View attachment 944

I am directly loading the XML from a file as a FileStream and then deserializing it to MessageConfig object. Is there any serialization needed after loading the config file into fs variable as a FileStream?
In fact there is no error. When I run the code
Our OP is using VeraCode or some other static secure code scanner. The scanner is correctly identifying that the data from the file is being loaded into the XML deserializer and is a potential vulnerability.

My suggestion is go to the related OWASP pages and implement one or more of the mitigating controls. If your scanning software provides links to description of the vulnerability class prefer mitigations listed in those links first since the probability of the scanner detecting that you implemented the mitigating controls is much higher, and even if the the scanner didn't detect the mitigations, you have a very firm leg to stand on to show your security review team that you implementing things are recommended.

I have seen this post and there is no example in c# and also seems like I have followed the same what is been told in the post. But it could be better if I can get any solution with my c# code above. I am really unable to find a solution for this. :(
 

Skydiver

Staff member
Joined
Apr 6, 2019
Messages
1,227
Location
Virginia Beach, VA
Programming Experience
10+
The .NET Framework supports digitally signed XML. So if the provider of the XML can digitally sign the XML, you can first open the XML document, verify the signature, and only if the signature is valid, pass it on to the deserializer.

If the provider cannot digitally sign the file, then you have other options:

One is provide evidence to your security team that the "chain of custody" of the file is secure. For example, the file is installed by your installer. The installer and it's payload is digitally signed to prevent tampering of the data between the time the build finishes and the time it gets on the user's machine. And once installed on the user's machine, the file is ACL'd so that only trusted people can modify the file. Or if the file is downloaded on the fly, that the download source is trusted, secure, and no man in the middle can tamper with the file contents, and once the file is at rest on the user's machine, only your app can read or write to it. Or even better, after to download the file, you digitally sign it to prevent future tampering.

Other mitigations you can implement is parsing the XML yourself and instantiating the object yourself. Yes, that means losing the convenience of using the out of the box deserializer. The advantage is that while you are parsing the XML you can check the DLL's and Classnames against a whitelist.

Other mitigations are for the provider just to send you a predefined code word or flag and that will tell you what kind of object to instantiate.

Or you can do combinations of the above mitigations. Defense in depth is good (until it becomes a performance bottleneck).
 

jmcilhinney

C# Forum Moderator
Staff member
Joined
Apr 23, 2011
Messages
2,860
Location
Sydney, Australia
Programming Experience
10+
I am using CheckMarx for scanning the code and when I scanned, it showed below message catching fs in yellow.
It should be post #6 that we are first hearing about this. In future, please provide ALL the relevant information in the first post. It's not always possible to know what's relevant but it's hard to see how this could not be.
 

PraveenDasary

Member
Joined
May 17, 2020
Messages
6
Programming Experience
5-10
It should be post #6 that we are first hearing about this. In future, please provide ALL the relevant information in the first post. It's not always possible to know what's relevant but it's hard to see how this could not be.
Actually I have given all the information with a little changes to the naming of the objects otherwise what I have provided is everything. Somehow the CheckMarks may not be able to mitigate the issue.
 

jmcilhinney

C# Forum Moderator
Staff member
Joined
Apr 23, 2011
Messages
2,860
Location
Sydney, Australia
Programming Experience
10+
what I have provided is everything.
You should have told us where that message was coming from. If you don't, people will tend to assume that it's an error message generated by VS.
 
Top Bottom