Skip to content

Commit

Permalink
Prohibit Xml Dtd processing (#1370)
Browse files Browse the repository at this point in the history 
- enable CA3075 as warning (on windows)
- Dtd processing is by default not prohibited, only limited to 10000000 chars.
- ensure to only use safe defaults for all xml processing
  • Loading branch information
@mregen
mregen committed Apr 20, 2021
1 parent c996e79 commit 77a3da8
Show file tree
Hide file tree
Showing 18 changed files with 323 additions and 247 deletions.
3 changes: 3 additions & 0 deletions .editorconfig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,6 @@ dotnet_naming_symbols.all_members.applicable_kinds =

dotnet_naming_style.pascal_case_style.capitalization = pascal_case

# CA3075: Insecure DTD processing in XML
dotnet_diagnostic.CA3075.severity = warning

41 changes: 18 additions & 23 deletions Stack/Opc.Ua.Core/Schema/UANodeSetHelpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,11 @@ public UANodeSet()
/// <returns>The set of nodes</returns>
public static UANodeSet Read(Stream istrm)
{
StreamReader reader = new StreamReader(istrm);

try
using (StreamReader reader = new StreamReader(istrm))
using (XmlReader xmlReader = XmlReader.Create(reader, Utils.DefaultXmlReaderSettings()))
{
XmlSerializer serializer = new XmlSerializer(typeof(UANodeSet));
return serializer.Deserialize(reader) as UANodeSet;
}
finally
{
reader.Dispose();
return serializer.Deserialize(xmlReader) as UANodeSet;
}
}

Expand Down Expand Up @@ -103,7 +98,7 @@ public void AddAlias(ISystemContext context, string alias, Opc.Ua.NodeId nodeId)
Array.Copy(this.Aliases, aliases, this.Aliases.Length);
}

aliases[count-1] = new NodeIdAlias() { Alias = alias, Value = Export(nodeId, context.NamespaceUris) };
aliases[count - 1] = new NodeIdAlias() { Alias = alias, Value = Export(nodeId, context.NamespaceUris) };
this.Aliases = aliases;
}

Expand All @@ -123,7 +118,7 @@ public void Import(ISystemContext context, NodeStateCollection nodes)
/// <summary>
/// Adds a node to the set.
/// </summary>
public void Export(ISystemContext context, NodeState node, bool outputRedundantNames =true)
public void Export(ISystemContext context, NodeState node, bool outputRedundantNames = true)
{
if (node == null) throw new ArgumentNullException(nameof(node));

Expand Down Expand Up @@ -175,7 +170,7 @@ public void Export(ISystemContext context, NodeState node, bool outputRedundantN
encoder.WriteVariantContents(variant.Value, variant.TypeInfo);

XmlDocument document = new XmlDocument();
document.InnerXml = encoder.Close();
document.LoadInnerXml(encoder.Close());
value.Value = document.DocumentElement;
}

Expand Down Expand Up @@ -238,7 +233,7 @@ public void Export(ISystemContext context, NodeState node, bool outputRedundantN
encoder.WriteVariantContents(variant.Value, variant.TypeInfo);

XmlDocument document = new XmlDocument();
document.InnerXml = encoder.Close();
document.LoadInnerXml(encoder.Close());
value.Value = document.DocumentElement;
}

Expand Down Expand Up @@ -292,7 +287,7 @@ public void Export(ISystemContext context, NodeState node, bool outputRedundantN
}
else
{
exportedNode.Description = new LocalizedText[0];
exportedNode.Description = Array.Empty<LocalizedText>();
}

exportedNode.Documentation = node.NodeSetDocumentation;
Expand Down Expand Up @@ -351,7 +346,7 @@ public void Export(ISystemContext context, NodeState node, bool outputRedundantN
Array.Copy(this.Items, nodes, this.Items.Length);
}

nodes[count-1] = exportedNode;
nodes[count - 1] = exportedNode;

this.Items = nodes;

Expand Down Expand Up @@ -957,7 +952,7 @@ private string Export(Opc.Ua.QualifiedName source, NamespaceTable namespaceUris)
}
else
{
output.DisplayName = new LocalizedText[0];
output.DisplayName = Array.Empty<LocalizedText>();
}

output.Description = Export(new Opc.Ua.LocalizedText[] { field.Description });
Expand Down Expand Up @@ -1042,7 +1037,7 @@ private Opc.Ua.DataTypeDefinition Import(UADataType dataType, Opc.Ua.Export.Data
{
output.IsOptional = false;
}
else if(sd.StructureType == (StructureType)3 || //StructureType.StructureWithSubtypedValues ||
else if (sd.StructureType == (StructureType)3 || //StructureType.StructureWithSubtypedValues ||
sd.StructureType == (StructureType)4) //StructureType.UnionWithSubtypedValues)
{
output.IsOptional = field.AllowSubTypes;
Expand Down Expand Up @@ -1255,7 +1250,7 @@ private ushort ExportNamespaceIndex(ushort namespaceIndex, NamespaceTable namesp
{
if (this.NamespaceUris[ii] == targetUri)
{
return (ushort)(ii+1); // add 1 to adjust for the well-known URIs which are not stored.
return (ushort)(ii + 1); // add 1 to adjust for the well-known URIs which are not stored.
}
}

Expand All @@ -1270,7 +1265,7 @@ private ushort ExportNamespaceIndex(ushort namespaceIndex, NamespaceTable namesp
Array.Copy(this.NamespaceUris, uris, count - 1);
}

uris[count-1] = targetUri;
uris[count - 1] = targetUri;
this.NamespaceUris = uris;

// return the new index.
Expand All @@ -1289,13 +1284,13 @@ private ushort ImportNamespaceIndex(ushort namespaceIndex, NamespaceTable namesp
}

// return a bad value if parameters are bad.
if (namespaceUris == null || this.NamespaceUris == null || this.NamespaceUris.Length <= namespaceIndex-1)
if (namespaceUris == null || this.NamespaceUris == null || this.NamespaceUris.Length <= namespaceIndex - 1)
{
return UInt16.MaxValue;
}

// find or append uri.
return namespaceUris.GetIndexOrAppend(this.NamespaceUris[namespaceIndex-1]);
return namespaceUris.GetIndexOrAppend(this.NamespaceUris[namespaceIndex - 1]);
}

/// <summary>
Expand All @@ -1318,7 +1313,7 @@ private ushort ExportNamespaceUri(string namespaceUri, NamespaceTable namespaceU
}

// find an existing index.
int count = 1;;
int count = 1; ;

if (this.NamespaceUris != null)
{
Expand Down Expand Up @@ -1390,7 +1385,7 @@ private uint ExportServerIndex(uint serverIndex, StringTable serverUris)
Array.Copy(this.ServerUris, uris, count - 1);
}

uris[count-1] = targetUri;
uris[count - 1] = targetUri;
this.ServerUris = uris;

// return the new index.
Expand All @@ -1409,7 +1404,7 @@ private uint ImportServerIndex(uint serverIndex, StringTable serverUris)
}

// return a bad value if parameters are bad.
if (serverUris == null || this.ServerUris == null || this.ServerUris.Length <= serverIndex-1)
if (serverUris == null || this.ServerUris == null || this.ServerUris.Length <= serverIndex - 1)
{
return UInt16.MaxValue;
}
Expand Down
2 changes: 1 addition & 1 deletion Stack/Opc.Ua.Core/Stack/Configuration/ApplicationConfiguration.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public object Create(object parent, object configContext, System.Xml.XmlNode sec
element = element.NextSibling;
}

XmlReader reader = XmlReader.Create(new StringReader(element.OuterXml));
XmlReader reader = XmlReader.Create(new StringReader(element.OuterXml), Utils.DefaultXmlReaderSettings());

try
{
Expand Down
9 changes: 6 additions & 3 deletions Stack/Opc.Ua.Core/Stack/Configuration/SecurityConfigurationManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,11 @@ public void WriteConfiguration(string filePath, SecuredApplication configuration

// load from file.
XmlDocument document = new XmlDocument();
document.Load(new FileStream(filePath, FileMode.Open));

using (var stream = new FileStream(filePath, FileMode.Open))
using (var xmlReader = XmlReader.Create(stream, Utils.DefaultXmlReaderSettings()))
{
document.Load(xmlReader);
}
XmlElement element = Find(document.DocumentElement, "SecuredApplication", Namespaces.OpcUaSecurity);

// update secured application.
Expand Down Expand Up @@ -377,7 +380,7 @@ private static string SetObject(Type type, object value)

// must extract the inner xml.
XmlDocument document = new XmlDocument();
document.InnerXml = Encoding.UTF8.GetString(memoryStream.ToArray());
document.LoadInnerXml(Encoding.UTF8.GetString(memoryStream.ToArray()));
return document.DocumentElement.InnerXml;
}
}
Expand Down

0 comments on commit 77a3da8

Please sign in to comment.