The XslTransformation task is unable to transform XML files containing DTD

[ackh]

Issue Description

Attempting to transform an XML file that begins with a Document Type Definition (DTD) using MSBuild's XslTransformation task results in the following error message:

error MSB3703: Unable to execute transformation. For security reasons DTD is prohibited in this XML document. To enable DTD processing set the DtdProcessing property on XmlReaderSettings to Parse and pass the settings into XmlReader.Create method.

Steps to Reproduce

Run the following sample project using MSBuild in order to get the above mentioned error message:

<?xml version="1.0" encoding="UTF-8"?>
<Project DefaultTargets="Build" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Target Name="Build">
    <PropertyGroup>
      <XmlContent>
        &lt;!DOCTYPE note SYSTEM &quot;Note.dtd&quot;&gt;
        &lt;Repro&gt;
          &lt;Bug /&gt;
        &lt;/Repro&gt;
      </XmlContent>
      <XslContent>
        &lt;xsl:stylesheet xmlns:xsl=&quot;http://www.w3.org/1999/XSL/Transform&quot; version=&quot;1.0&quot;&gt;
          &lt;xsl:output method=&quot;xml&quot; indent=&quot;yes&quot; /&gt;
          &lt;xsl:template match=&quot;@*|node()&quot;&gt;
            &lt;xsl:copy&gt;
              &lt;xsl:apply-templates select=&quot;@*|node()&quot; /&gt;
            &lt;/xsl:copy&gt;
          &lt;/xsl:template&gt;
          &lt;xsl:template match=&quot;/Repro/Bug&quot;&gt;
            &lt;Fix /&gt;
          &lt;/xsl:template&gt;
        &lt;/xsl:stylesheet&gt;
      </XslContent>
      <OutputFile>output.xml</OutputFile>
    </PropertyGroup>
    <XslTransformation XmlContent="$(XmlContent)" XslContent="$(XslContent)" OutputPaths="$(OutputFile)" />
  </Target>
</Project>

Deleting the line <!DOCTYPE note SYSTEM "Note.dtd"> results in a proper transformation.

Analysis

Line 222 of the XslTransformation source code sets the DtdProcessing property of the XmlReaderSettings object that is used to DtdProcessing.Ignore. This prevents XML files containing DTDs from being transformed. To be able to transform such files the property would need to be set to DtdProcessing.Parse.

.NET offers this property to prevent denial of service attacks (see here for more info). However, I'm not sure whether that would be an issue in an MSBuild task. If so, it would make sense to expose a property that changes the value of the DtdProcessing property.

Not being able to transform DTD containing XML files is just cumbersome. You'd need to remove all DTD lines before the transformation and insert them again afterwards.

[benvillalobos]

Team Triage: We're not sure we want to take a fix on this, as it's a security issue. Note that the docs on XmlReaderSettings.DtdProcessing state that the default should be Prohibit anyway.

Related issue: http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1043311

[ackh]

I don't have access to http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1043311 so I don't know about its content. However, here's my general view regarding that topic:

MSBuild tasks are to the design of build processes what .NET (or any other technology) APIs are to the design of software systems. If the .NET framework is improperly used, you can end up with nasty security problems and a whole array of other issues. But if you'd lock down the .NET framework in order to prevent certain things from being done you'd severely limit its usefulness.

The same applies to MSBuild tasks. If used improperly, you can create lots of problems as well. Use the Delete task improperly and you might end up reinstalling the operating system. Lock them down too much and they become impractical to solve anything else than the narrow happy case. People will turn away from them and use other means to get the job done. Other means that might even be more problematic. security-wise.

So, I think the MSBuild tasks should strive to be simple and secure to use BUT at the same time be flexible and powerful enough to
solve more than just happy cases. I opened this issue because in the Apple eco system, property list files (plist) are riddled with DTD and I had to turn to other means because MSBuild's XML manipulation tasks made me surrender and walk away to find other approaches.

Currently, there are three tasks shipped together with MSBuild that are designed to manipulate XML. These task behave differently in terms of DTD:

1. XmlPeek: Contains a bool property named ProhibitDtd which was discussed here.
2. XmlPoke: Internally sets DtdProcessing.Ignore which means it simply strips out DTD when writing the modified file. I have opened a bug report regarding this here.
3. XslTransformation: Also sets DtdProcessing.Ignore internally which leads to the issue reported above.

So, XmlPoke and XslTransformation ignore DTD which makes them useless when having to deal with DTD containing XML files while XmlPeek can be configured to work with DTD.

I think a better approach would be to change all three tasks to prohibit DTD by default which would make them save to use by default. However, all three tasks should then be equipped with a property that would enable DTD. That would make them capable of dealing with more than just the happy case. From my perspective, that would be a good trade-off between ease-of-use and security.

So, I would add the ProhibitDtd bool property already available in XmlPeek to both XmlPoke and XslTransformation. Internally, the default should be DtdProcessing.Prohibit as you already pointed out here. Enabling the property should change it to DtdProcessing.Parse for both XmlPoke and XslTransformation while leaving it at DtdProcessing.Ignore would be fine for XmlPeek. However, it could also be changed to DtdProcessing.Prohibit for XmlPeek but that would be a breaking change, I guess.

Note that the part of the argumentation for introducing that property on XmlPeek was

The security settings should be OK to turn off since you should only open project files from a trusted source anyway

(see here).

The argument above contains a valid point from my perspective. Security is a concern but shielding the tasks with a property would properly address that concern from my perspective.

[benvillalobos]

Just a heads up, this is still being discussed internally.