Escape snapshot names in libvirt XML to prevent XML injection

Author: mprokopchuk

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateVMSnapshotCommandWrapper.java

@@ -19,6 +19,7 @@
 
 package com.cloud.hypervisor.kvm.resource.wrapper;
 
+import org.apache.commons.lang.StringEscapeUtils;
 import org.libvirt.Connect;
 import org.libvirt.Domain;
 import org.libvirt.DomainInfo.DomainState;
@@ -57,7 +58,7 @@ public Answer execute(final CreateVMSnapshotCommand cmd, final LibvirtComputingR
                         "Create Instance Snapshot Failed due to Instance is not running: " + vmName + " with domainState = " + domainState);
             }
 
-            String vmSnapshotXML = "<domainsnapshot>" + "  <name>" + vmSnapshotName + "</name>"
+            String vmSnapshotXML = "<domainsnapshot>" + "  <name>" + StringEscapeUtils.escapeXml(vmSnapshotName) + "</name>"
                     + "  <memory snapshot='internal' />" + "</domainsnapshot>";
 
             dm.snapshotCreateXML(vmSnapshotXML);

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java

@@ -22,6 +22,7 @@
 import java.io.File;
 import java.text.MessageFormat;
 
+import org.apache.commons.lang.StringEscapeUtils;
 import org.libvirt.Connect;
 import org.libvirt.Domain;
 import org.libvirt.DomainInfo.DomainState;
@@ -79,7 +80,7 @@ public Answer execute(final ManageSnapshotCommand command, final LibvirtComputin
                         + "            <uuid>{1}</uuid>" + "        </domain>" + "    </domainsnapshot>");
 
                 final String vmUuid = vm.getUUIDString();
-                final Object[] args = new Object[] {snapshotName, vmUuid};
+                final Object[] args = new Object[] {StringEscapeUtils.escapeXml(snapshotName), vmUuid};
                 final String snapshot = snapshotXML.format(args);
                 logger.debug(snapshot);
                 if (command.getCommandSwitch().equalsIgnoreCase(ManageSnapshotCommand.CREATE_SNAPSHOT)) {

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtUtilitiesHelper.java

@@ -26,6 +26,7 @@
 import com.cloud.hypervisor.kvm.storage.KVMStoragePool;
 import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager;
 import org.apache.cloudstack.storage.to.PrimaryDataStoreTO;
+import org.apache.commons.lang.StringEscapeUtils;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
 import org.libvirt.Connect;
@@ -112,9 +113,9 @@ public Script buildScript(final String scriptPath) {
     }
 
     public String generateVMSnapshotXML(VMSnapshotTO snapshot, VMSnapshotTO parent, String domainXmlDesc) {
-        String parentName = (parent == null)? "": ("  <parent><name>" + parent.getSnapshotName() + "</name></parent>\n");
+        String parentName = (parent == null)? "": ("  <parent><name>" + StringEscapeUtils.escapeXml(parent.getSnapshotName()) + "</name></parent>\n");
         String vmSnapshotXML = "<domainsnapshot>\n"
-                + "  <name>" + snapshot.getSnapshotName() + "</name>\n"
+                + "  <name>" + StringEscapeUtils.escapeXml(snapshot.getSnapshotName()) + "</name>\n"
                 + "  <state>running</state>\n"
                 + parentName
                 + "  <creationTime>" + (int) Math.rint(snapshot.getCreateTime()/1000) + "</creationTime>\n"

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtUtilitiesHelperTest.java

@@ -25,6 +25,7 @@
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 
+import com.cloud.agent.api.VMSnapshotTO;
 import com.cloud.utils.Pair;
 
 import junit.framework.TestCase;
@@ -75,4 +76,30 @@ public void validateIsLibvirtVersionEqualOrHigherThanVersionInParameterLibvirtVe
         Assert.assertEquals(String.valueOf(libvirtVersion), result.first());
         Assert.assertTrue(result.second());
     }
+
+    @Test
+    public void generateVMSnapshotXMLEscapesSnapshotName() {
+        VMSnapshotTO snapshot = Mockito.mock(VMSnapshotTO.class);
+        Mockito.doReturn("i-2-3-VM_VS_</name><disks><disk name='vda'/></disks><name>x").when(snapshot).getSnapshotName();
+        Mockito.doReturn(1000L).when(snapshot).getCreateTime();
+
+        String xml = libvirtUtilitiesHelperSpy.generateVMSnapshotXML(snapshot, null, "<domain/>");
+
+        Assert.assertTrue(xml.contains("<name>i-2-3-VM_VS_&lt;/name&gt;&lt;disks&gt;&lt;disk name=&apos;vda&apos;/&gt;&lt;/disks&gt;&lt;name&gt;x</name>"));
+        Assert.assertFalse(xml.contains("<disks>"));
+    }
+
+    @Test
+    public void generateVMSnapshotXMLEscapesParentSnapshotName() {
+        VMSnapshotTO snapshot = Mockito.mock(VMSnapshotTO.class);
+        Mockito.doReturn("child").when(snapshot).getSnapshotName();
+        Mockito.doReturn(1000L).when(snapshot).getCreateTime();
+        VMSnapshotTO parent = Mockito.mock(VMSnapshotTO.class);
+        Mockito.doReturn("parent</name><disks/>").when(parent).getSnapshotName();
+
+        String xml = libvirtUtilitiesHelperSpy.generateVMSnapshotXML(snapshot, parent, "<domain/>");
+
+        Assert.assertTrue(xml.contains("<parent><name>parent&lt;/name&gt;&lt;disks/&gt;</name></parent>"));
+        Assert.assertFalse(xml.contains("<disks/>"));
+    }
 }