Xen Security Advisory 459 v2 (CVE-2024-31144) - Xapi: Metadata injection attack against backup/restore functionality

Xen.org security team posted 1 patch 3 months ago
Failed in applying to current master (apply log)
scripts/xe-backup-metadata  |  32 +------
scripts/xe-restore-metadata | 164 ++++++++++++++++++++++++------------
2 files changed, 111 insertions(+), 85 deletions(-)
Xen Security Advisory 459 v2 (CVE-2024-31144) - Xapi: Metadata injection attack against backup/restore functionality
Posted by Xen.org security team 3 months ago
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

            Xen Security Advisory CVE-2024-31144 / XSA-459
                               version 2

  Xapi: Metadata injection attack against backup/restore functionality

UPDATES IN VERSION 2
====================

Public release.

ISSUE DESCRIPTION
=================

For a brief summary of Xapi terminology, see:

  https://xapi-project.github.io/xen-api/overview.html#object-model-overview

Xapi contains functionality to backup and restore metadata about Virtual
Machines and Storage Repositories (SRs).

The metadata itself is stored in a Virtual Disk Image (VDI) inside an
SR.  This is used for two purposes; a general backup of metadata
(e.g. to recover from a host failure if the filer is still good), and
Portable SRs (e.g. using an external hard drive to move VMs to another
host).

Metadata is only restored as an explicit administrator action, but
occurs in cases where the host has no information about the SR, and must
locate the metadata VDI in order to retrieve the metadata.

The metadata VDI is located by searching (in UUID alphanumeric order)
each VDI, mounting it, and seeing if there is a suitable metadata file
present.  The first matching VDI is deemed to be the metadata VDI, and
is restored from.

In the general case, the content of VDIs are controlled by the VM owner,
and should not be trusted by the host administrator.

A malicious guest can manipulate its disk to appear to be a metadata
backup.

A guest cannot choose the UUIDs of its VDIs, but a guest with one disk
has a 50% chance of sorting ahead of the legitimate metadata backup.  A
guest with two disks has a 75% chance, etc.

IMPACT
======

If a fraudulent metadata backup has been written into an SR which also
contains a legitimate metadata backup, and an administrator explicitly
chooses to restore from backup, the fraudulent metadata might be
consumed instead of the legitimate metadata.

Control over meta data includes: which VMs are created, disk assignment,
vCPU/RAM requirements, GPU allocation, etc.

VULNERABLE SYSTEMS
==================

Systems running Xapi v1.249.x are affected.

Systems running Xapi v24.x are potentially affected.  However it is
believed that the only supported products using this version of Xapi
have not shipped the metadata backup/restore functionality.

To leverage the vulnerability, an attacker would likely need insider
information to construct a plausible-looking metadata backup, and would
have to persuade a real administrator to perform a data-recovery action.

MITIGATION
==========

Not using the metadata restore functionality avoids the vulnerability.

CREDITS
=======

This issue was discovered by XenServer.

RESOLUTION
==========

The attached patches resolve the issue for Xapi v1.249.x releases.

xsa459-xen-api.patch (based on v1.249.37) causes all new metadata VDIs
to be created with a deterministic UUID, and restore functionality to use
that UUID only; not to search all disks to find the metadata.

After installing the updated Xapi, a new metadata backup should be
taken, to create a VDI with the new deterministic UUID.

The ability to restore from an old backup VDI is retained, but the
administrator is required to specify the exact VDI to use, so as to
avoid searching the SR.

Because xsa459-xen-api.patch alters the behaviour of the
xe-{backup,restore}-metadata scripts, a companion patch
xsa459-xsconsole.patch (based on v10.1.13.1) is needed to keep the
pre-existing menu options working, and to ask for user conformation if
needing to restore from a prior backup.

Note: some work was carried out in public on this issue before the
security implications were understood.  These changes are present in
xen-api.git and tagged as v1.249.37, which is used as the base for this
patch.

$ sha256sum xsa459*
89dba36a1889a41fbf585a25432079d10991d9e9f3c2d9f93f285c11e17e02c3  xsa459-xen-api.patch
0fc4dabd3a84055644fe415f55d8a1148ad2c17aaa2f8b52889cb11800c612d2  xsa459-xsconsole.patch
$

DEPLOYMENT DURING EMBARGO
=========================

Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.

But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).

Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.


(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable.  This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)

For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
  http://www.xenproject.org/security-policy.html
-----BEGIN PGP SIGNATURE-----

iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmaWYLkMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZTwkIALcgQmF/UVzUfs54omUKi+E4woQmfEOPO1GNki5x
abjnmv7Nos7AJem6ytX2eLLcPvl7iEtFf8p+pYdXwjjyT+Gtg2+8E/k8m7b4Qx3u
ZoW0ID3LBNb0++Tc+DKJKuEOMg2/OINbcFqAQUWutzbz38QCMJ30JyAkZKU/UYmL
Hs/xb65PpI1khaZD/1ipjxCDP/XJIzV2l1vD23omb1TXiWhsdHtT9YKiypThECnA
/uBUyKHOC9+Tx1eYrG0H8am8t2MKoOQL0Lu2xWFJskrg2LHYkxk3he0OTKWcsVTz
OYs1ReZt1k9KSwpqsIq5uJj/HARUCm+fPmL126IB4q5tMQ4=
=9K9F
-----END PGP SIGNATURE-----
From 66647afff30808cde2938b7264d34727325dde70 Mon Sep 17 00:00:00 2001
From: Alex Brett <alex.brett@cloud.com>
Date: Tue, 9 Jul 2024 16:24:28 +0000
Subject: [PATCH] Updates to Portable SR Functionality

Add a new option `-o` to xe-restore-metadata, which is used to distinguish
whether to allow use of legacy backup VDIs, or enforce only use of the new
format VDIs with known UUIDs.

Also modify xe-restore-metadata such that it no longer stops searching the
candidate list if only one VDI is found, but instead identifies all possible
backup VDIs. If more than one is found, and you are doing anything other than
listing the VDIs, the script will abort. This is to cover the case where a
malicious legacy format VDI is present - we will detect it and the expected
'real' backup VDI.

Modify xe-backup-metadata to always expect to use the deterministic UUID to
identify the VDI to add backups to, do not rely on the
`other-config:ctxs-pool-backup` property for identification in any way.

This, together with changes in v1.249.37, is XSA-459 / CVE-2024-31144

Also fix the following issues introduced with changes in v1.249.37:
- Incorrect path to `xe` when calling vm-import
- Issues using 'dry run' functionality due to shell quoting changes

Signed-off-by: Alex Brett <alex.brett@cloud.com>
---
 scripts/xe-backup-metadata  |  32 +------
 scripts/xe-restore-metadata | 164 ++++++++++++++++++++++++------------
 2 files changed, 111 insertions(+), 85 deletions(-)

diff --git a/scripts/xe-backup-metadata b/scripts/xe-backup-metadata
index d25e0ccfa..1452fbaf7 100755
--- a/scripts/xe-backup-metadata
+++ b/scripts/xe-backup-metadata
@@ -55,23 +55,6 @@ function uuid5 {
   python -c "import uuid; print (uuid.uuid5(uuid.UUID('$1'), '$2'))"
 }
 
-function validate_vdi_uuid {
-  # we check that vdi has the expected UUID which depends on the UUID of
-  # the SR. This is a deterministic hash of the SR UUID and the
-  # namespace UUID $NS. This UUID must match what Xapi's Uuidx module is using.
-  local NS="e93e0639-2bdb-4a59-8b46-352b3f408c19"
-  local sr="$1"
-  local vdi="$2"
-  local uuid
-
-  uuid=$(uuid5 "$NS" "$sr")
-  if [ "$vdi" != "$uuid" ]; then
-    return 1
-  else
-    return 0
-  fi
-}
-
 function test_sr {
   sr_uuid_found=$(${XE} sr-list uuid="$1" --minimal)
   if [ "${sr_uuid_found}" != "$1" ]; then
@@ -120,8 +103,8 @@ fi
 test_sr "${sr_uuid}"
 
 sr_name=$(${XE} sr-param-get uuid="${sr_uuid}" param-name=name-label)
-# see if a backup VDI already exists on the selected SR
-vdi_uuid=$(${XE} vdi-list other-config:ctxs-pool-backup=true sr-uuid="${sr_uuid}" params=uuid --minimal)
+# assume use of the new format predictable UUID
+vdi_uuid=$(${XE} vdi-list uuid="$(uuid5 "e93e0639-2bdb-4a59-8b46-352b3f408c19" "$sr_uuid")" --minimal)
 
 mnt=
 function cleanup {
@@ -160,17 +143,6 @@ function cleanup {
    fi
 }
 
-# if we can't validate the UUID of the VDI, prompt the user
-if [ -n "${vdi_uuid}" ]; then
-    if ! validate_vdi_uuid "${sr_uuid}" "${vdi_uuid}" && [ "$yes" -eq 0 ]; then
-        echo "Backup VDI $vdi_uuid was most likley create by an earlier"
-        echo "version of this code. Make sure this is a VDI that you"
-        echo "created as we can't validate it without mounting it."
-        read -p "Continue? [Y/N]" -n 1 -r; echo
-        if [[ ! $REPLY =~ ^[Yy]$ ]]; then exit 1; fi
-    fi
-fi
-
 echo "Using SR: ${sr_name}"
 if [ -z "${vdi_uuid}" ]; then
   if [ "${create_vdi}" -gt 0 ]; then
diff --git a/scripts/xe-restore-metadata b/scripts/xe-restore-metadata
index 8c6299e9c..1f921f7ee 100755
--- a/scripts/xe-restore-metadata
+++ b/scripts/xe-restore-metadata
@@ -35,11 +35,11 @@ default_restore_mode="all"
 debug="/bin/true"
 
 function usage {
-    echo "Usage: $0 [-h] [-v] [-y] [-n] [-p] [-f] [-x <VDI UUID>] [-u <SR UUID>] [-m all|sr]"
+    echo "Usage: $0 [-h] [-v] [-y] [-n] [-p] [-f] [-o] [-x <VDI UUID>] [-u <SR UUID>] [-m all|sr]"
     echo
     echo " -h: Display this help message"
     echo " -x: Specify the VDI UUID to override probing"
-    echo " -p: Just scan for a metadata VDI and print out its UUID to stdout"
+    echo " -p: Just scan for metadata VDI(s) and print out UUID(s) to stdout"
     echo " -u: UUID of the SR you wish to restore from"
     echo " -n: Perform a dry run of the metadata import commands (default: false)"
     echo " -l: Just list the available backup dates"
@@ -48,6 +48,7 @@ function usage {
     echo " -v: Verbose output"
     echo " -y: Assume non-interactive mode and yes to all questions"
     echo " -f: Forcibly restore VM metadata, dangerous due to its destructive nature, please always do a dry run before using this (default: false)"
+    echo " -o: Allow use of legacy backup VDIs (this should not be used with SRs with untrusted VDIs)"
     echo 
     exit 1
 }
@@ -75,7 +76,9 @@ just_probe=0
 chosen_date=""
 restore_mode=${default_restore_mode}
 force=0
-while getopts "yhpvx:d:lnu:m:f" opt ; do
+legacy=0
+specified_vdi=
+while getopts "yhpvx:d:lnu:m:fo" opt ; do
     case $opt in
     h) usage ;;
     u) sr_uuid=${OPTARG} ;;
@@ -85,9 +88,10 @@ while getopts "yhpvx:d:lnu:m:f" opt ; do
     v) debug="" ;;
     d) chosen_date=${OPTARG} ;;
     m) restore_mode=${OPTARG} ;;
-    x) vdis=${OPTARG} ;;
+    x) specified_vdi=${OPTARG} ;;
     y) yes=1 ;;
     f) force=1 ;;
+    o) legacy=1 ;;
     *) echo "Invalid option"; usage ;;
     esac
 done
@@ -118,16 +122,75 @@ sr_name=$(${XE} sr-param-get uuid="${sr_uuid}" param-name=name-label)
 # probe first for a VDI with known UUID derived from the SR to avoid
 # scanning for a VDI
 backup_vdi=$(uuid5 "${NS}" "${sr_uuid}")
-if [ -z "${vdis}" ]; then
-  vdis=$(${XE} vdi-list uuid="${backup_vdi}" sr-uuid="${sr_uuid}" read-only=false --minimal)
+
+# Only allow a specified VDI that does not match the known UUID if operating in
+# legacy mode
+if [ -n "${specified_vdi}" ]; then
+  if [ "${specified_vdi}" != "${backup_vdi}" ] && [ "$legacy" -eq 0 ]; then
+    echo "The specified VDI UUID is not permitted, if attempting to use a legacy backup VDI please use the -o flag" >&2
+    exit 1
+  fi
+  vdis=${specified_vdi}
 fi
 
-# get a list of all VDIs if an override has not been provided on the cmd line
 if [ -z "${vdis}" ]; then
-  vdis=$(${XE} vdi-list params=uuid sr-uuid="${sr_uuid}" read-only=false --minimal)
+  if [ "$legacy" -eq 0 ]; then
+    # In non-legacy mode, only use the known backup_vdi UUID
+    vdis=$(${XE} vdi-list uuid="${backup_vdi}" sr-uuid="${sr_uuid}" read-only=false --minimal)
+  else
+    # In legacy mode, scan all VDIs
+    vdis=$(${XE} vdi-list params=uuid sr-uuid="${sr_uuid}" read-only=false --minimal)
+  fi
 fi
 
 mnt=
+vdi_uuid=
+vbd_uuid=
+device=
+function createvbd {
+  ${debug} echo -n "Creating VBD: " >&2
+  vbd_uuid=$(${XE} vbd-create vm-uuid="${CONTROL_DOMAIN_UUID}" vdi-uuid="${vdi_uuid}" device=autodetect 2>/dev/null)
+
+  if [ $? -ne 0 -o -z "${vbd_uuid}" ]; then
+    ${debug} echo "error creating VBD for VDI ${vdi_uuid}" >&2
+    cleanup
+    return 1
+  fi
+
+  ${debug} echo "${vbd_uuid}" >&2
+
+  ${debug} echo -n "Plugging VBD: " >&2
+  ${XE} vbd-plug uuid="${vbd_uuid}"
+  device=/dev/$(${XE} vbd-param-get uuid="${vbd_uuid}" param-name=device)
+
+  if [ ! -b "${device}" ]; then
+     ${debug} echo "${device}: not a block special" >&2
+     cleanup
+     return 1
+  fi
+
+  ${debug} echo "${device}" >&2
+  return 0
+}
+
+function mountvbd {
+  mnt="/var/run/pool-backup-${vdi_uuid}"
+  mkdir -p "${mnt}"
+  /sbin/fsck -a "${device}" >/dev/null 2>&1
+  if [ $? -ne 0 ]; then
+    echo "File system integrity error.  Please correct manually." >&2
+    cleanup
+    return 1
+  fi
+  mount "${device}" "${mnt}" >/dev/null 2>&1
+  if [ $? -ne 0 ]; then
+    ${debug} echo failed >&2
+    cleanup
+    return 1
+  fi
+  return 0
+}
+
 function cleanup {
    cd /
    if [ ! -z "${mnt}" ]; then
@@ -165,65 +228,32 @@ function cleanup {
 
 if [ -z "${vdis}" ]; then
    echo "No VDIs found on SR." >&2
+   if [ "$legacy" -eq 0 ]; then
+      echo "If you believe there may be a legacy backup VDI present, you can use the -o flag to search for it (this should not be used with untrusted VDIs)" >&2
+   fi
    exit 0
 fi
 
 trap cleanup SIGINT ERR
 
+declare -a matched_vdis
 for vdi_uuid in ${vdis}; do
-   if [ "${vdi_uuid}" != "${backup_vdi}" ] && [ "$yes" -eq 0 ]; then
-      echo "Probing VDI ${vdi_uuid}."
-      echo "This VDI was created with a prior version of this code."
-      echo "Its validity can't be checked without mounting it first."
-      read -p "Continue? [Y/N]" -n 1 -r; echo
-      if [[ ! $REPLY =~ ^[Yy]$ ]]; then exit 1; fi
-   fi
-
-   ${debug} echo -n "Creating VBD: " >&2
-   vbd_uuid=$(${XE} vbd-create vm-uuid="${CONTROL_DOMAIN_UUID}" vdi-uuid="${vdi_uuid}" device=autodetect 2>/dev/null)
-
-   if [ $? -ne 0 -o -z "${vbd_uuid}" ]; then
-      ${debug} echo "error creating VBD for VDI ${vdi_uuid}" >&2
-      cleanup
-      continue
-   fi
-
-   ${debug} echo "${vbd_uuid}" >&2
-
-   ${debug} echo -n "Plugging VBD: " >&2
-   ${XE} vbd-plug uuid="${vbd_uuid}"
-   device=/dev/$(${XE} vbd-param-get uuid="${vbd_uuid}" param-name=device)
-
-   if [ ! -b "${device}" ]; then
-     ${debug} echo "${device}: not a block special" >&2
-     cleanup
+   createvbd
+   if [ $? -ne 0 ]; then
      continue
    fi
 
-   ${debug} echo "${device}" >&2
-
    ${debug} echo -n "Probing device: " >&2
    mnt=
    if [ "$(file_exists "${device}" "/.ctxs-metadata-backup")" = y ]; then
      ${debug} echo "found metadata backup" >&2
-     mnt="/var/run/pool-backup-${vdi_uuid}"
-     mkdir -p "${mnt}"
-     /sbin/e2fsck -p -f "${device}" >/dev/null 2>&1
+     mountvbd
      if [ $? -ne 0 ]; then
-        echo "File system integrity error.  Please correct manually." >&2
-        cleanup
         continue
      fi
-     mount -o ro,nosuid,noexec,nodev "${device}" "${mnt}" >/dev/null 2>&1
-     if [ $? -ne 0 ]; then
-       ${debug} echo failed >&2
-       cleanup
-     else
-       if [ -e "${mnt}/.ctxs-metadata-backup" ]; then
-          ${debug} echo "Found backup metadata on VDI: ${vdi_uuid}" >&2
-          xe vdi-param-set uuid="${vdi_uuid}" other-config:ctxs-pool-backup=true
-          break
-       fi
+     if [ -e "${mnt}/.ctxs-metadata-backup" ]; then
+        ${debug} echo "Found backup metadata on VDI: ${vdi_uuid}" >&2
+       matched_vdis+=( ${vdi_uuid} )
      fi
    else
      ${debug} echo "backup metadata not found" >&2
@@ -232,11 +262,35 @@ for vdi_uuid in ${vdis}; do
 done
 
 if [ $just_probe -gt 0 ]; then
-   echo "${vdi_uuid}"
-   cleanup
+   for vdi_uuid in "${matched_vdis[@]}"; do
+      echo "${vdi_uuid}"
+   done
    exit 0
 fi
 
+if [ "${#matched_vdis[@]}" -eq 0 ]; then
+  echo "Metadata backups not found." >&2
+  exit 1
+fi
+
+if [ "${#matched_vdis[@]}" -gt 1 ]; then
+  echo "Multiple metadata backups found, please use -x to specify the VDI UUID to use" >&2
+  exit 1
+fi
+
+vdi_uuid=${matched_vdis[0]}
+xe vdi-param-set uuid="${vdi_uuid}" other-config:ctxs-pool-backup=true
+createvbd
+if [ $? -ne 0 ]; then
+  echo "Failure creating VBD for backup VDI ${vdi_uuid}" >&2
+  exit 1
+fi
+mountvbd
+if [ $? -ne 0 ]; then
+  echo "Failure mounting backup VDI ${vdi_uuid}" >&2
+  exit 1
+fi
+
 cd "${mnt}"
 ${debug} echo "" >&2
 
@@ -323,8 +377,8 @@ else
 fi
 shopt -s nullglob
 for meta in *.vmmeta; do
-   echo xe vm-import filename="${meta}" sr-uuid="${sr_uuid}" --metadata --preserve"${force_flag}""${dry_run_flag}"
-   "@BINDIR@/bin/xe" vm-import filename="${full_dir}/${meta}" sr-uuid="${sr_uuid}" --preserve"${force_flag}""${dry_run_flag}"
+   echo xe vm-import filename="${meta}" sr-uuid="${sr_uuid}" --metadata --preserve${force_flag}${dry_run_flag}
+   "@BINDIR@/xe" vm-import filename="${full_dir}/${meta}" sr-uuid="${sr_uuid}" --metadata --preserve${force_flag}${dry_run_flag}
    if [ $? -gt 0 ]; then
       error_count=$(( $error_count + 1 ))
    else
-- 
2.25.1

From 20c02765c30bdb7e14e3c020b2d7e956f88c8150 Mon Sep 17 00:00:00 2001
From: Alex Brett <alex.brett@cloud.com>
Date: Tue, 9 Jul 2024 16:45:16 +0000
Subject: [PATCH] Updates to Portable SR functionality

Modifies functionality to understand the use of legacy format VDIs:
- Adds an option to search for a legacy format VDI if a new one is not found
- Handles the case where multiple legacy VDIs are identified
- Passes the appropriate option to `xe-restore-metadata` when needed

This is XSA-459 / CVE-2024-31144

Also update the subprocess calls to be compatible with python3

Signed-off-by: Alex Brett <alex.brett@cloud.com>
---
 plugins-base/XSFeatureDRRestore.py | 172 ++++++++++++++++++++++-------
 1 file changed, 135 insertions(+), 37 deletions(-)

diff --git a/plugins-base/XSFeatureDRRestore.py b/plugins-base/XSFeatureDRRestore.py
index 2cadde5..ea7fd7c 100644
--- a/plugins-base/XSFeatureDRRestore.py
+++ b/plugins-base/XSFeatureDRRestore.py
@@ -19,14 +19,93 @@ if __name__ == "__main__":
 from XSConsoleStandard import *
 import subprocess
 
+def _listBackups(sr_uuid, vdi_uuid, legacy=False):
+    # list the available backups
+    Layout.Inst().TransientBanner(Lang("Found VDI, retrieving available backups..."))
+    command = ["%s/xe-restore-metadata" % (Config.Inst().HelperPath(),), "-l", "-u", sr_uuid, "-x", vdi_uuid]
+    if legacy:
+        command.append("-o")
+    cmd = subprocess.Popen(command,
+                           stdout = subprocess.PIPE,
+                           stderr = subprocess.PIPE,
+                           universal_newlines = True)
+    output, errput = cmd.communicate()
+    status = cmd.returncode
+    if status != 0:
+        raise Exception("(%s,%s)" % (output,errput))
+    Layout.Inst().PushDialogue(DRRestoreSelection(output, vdi_uuid, sr_uuid, legacy=legacy))
+
+class DRRestoreVDISelection(Dialogue):
+    def __init__(self, sr_uuid, vdi_uuids):
+        Dialogue.__init__(self)
+
+        choices = []
+
+        self.sr_uuid = sr_uuid
+        self.vdi_uuids = vdi_uuids
+        index = 0
+        for choice in self.vdi_uuids:
+            cdef = ChoiceDef(choice, lambda i=index: self.HandleVDIChoice(i))
+            index = index + 1
+            choices.append(cdef)
+
+        self.testMenu = Menu(self, None, "", choices)
+        self.ChangeState('LISTVDIS')
+
+    def BuildPane(self):
+        pane = self.NewPane(DialoguePane(self.parent))
+        pane.TitleSet(Lang('Restore Virtual Machine Metadata'))
+        pane.AddBox()
+
+    def UpdateFieldsLISTVDIS(self):
+        pane = self.Pane()
+        pane.ResetFields()
+
+        pane.TitleSet("Available Metadata VDIs")
+        pane.AddTitleField(Lang("Select Metadata VDI to Restore From"))
+        pane.AddWarningField(Lang("You should only restore metadata from a trustworthy VDI; loading untrustworthy metadata may put your system at risk"))
+        pane.AddMenuField(self.testMenu)
+        pane.AddKeyHelpField( { Lang("<Enter>") : Lang("OK"), Lang("<Esc>") : Lang("Cancel") } )
+
+    def UpdateFields(self):
+        self.Pane().ResetPosition()
+        getattr(self, 'UpdateFields'+self.state)() # Despatch method named 'UpdateFields'+self.state
+
+    def ChangeState(self, inState):
+        self.state = inState
+        self.BuildPane()
+        self.UpdateFields()
+
+    def HandleVDIChoice(self, inChoice):
+        _listBackups(self.sr_uuid, self.vdi_uuids[inChoice], legacy=True)
+
+    def HandleKeyLISTVDIS(self, inKey):
+        handled = self.testMenu.HandleKey(inKey)
+        if not handled and inKey == 'KEY_LEFT':
+            Layout.Inst().PopDialogue()
+            handled = True
+        return handled
+
+    def HandleKey(self, inKey):
+        handled = False
+        if hasattr(self, 'HandleKey'+self.state):
+            handled = getattr(self, 'HandleKey'+self.state)(inKey)
+
+        if not handled and inKey == 'KEY_ESCAPE':
+            Layout.Inst().PopDialogue()
+            handled = True
+
+        return handled
+
 class DRRestoreSelection(Dialogue):
 
-    def __init__(self, date_choices, vdi_uuid, sr_uuid):
+    def __init__(self, date_choices, vdi_uuid, sr_uuid, legacy=False):
         Dialogue.__init__(self)
 
         choices = []
         self.vdi_uuid = vdi_uuid
         self.sr_uuid = sr_uuid
+        self.legacy = legacy
         self.date_choices = date_choices.splitlines()
         index = 0
         for choice in self.date_choices:
@@ -86,14 +165,19 @@ class DRRestoreSelection(Dialogue):
             Layout.Inst().PushDialogue(InfoDialogue(Lang("Internal Error, unexpected choice: " + inChoice)))
         else:
             chosen_mode = inChoice
-            if dryRun:
-              dry_flag="-n "
-            else:
-              dry_flag=""
             Layout.Inst().TransientBanner(Lang("Restoring VM Metadata.  This may take a few minutes..."))
-            command = "%s/xe-restore-metadata -y %s -u %s -x %s -d %s -m %s" % (Config.Inst().HelperPath(), dry_flag, self.sr_uuid, self.vdi_uuid, self.chosen_date, chosen_mode)
-            status, output = commands.getstatusoutput(command)
-            status = os.WEXITSTATUS(status)
+            command = ["%s/xe-restore-metadata" % (Config.Inst().HelperPath(),), "-y", "-u", self.sr_uuid, "-x", self.vdi_uuid, "-d", self.chosen_date, "-m", chosen_mode]
+            if dryRun:
+                command.append("-n")
+            if self.legacy:
+                command.append("-o")
+
+            cmd = subprocess.Popen(command,
+                                stdout = subprocess.PIPE,
+                                stderr = subprocess.STDOUT,
+                                universal_newlines = True)
+            output, _ = cmd.communicate()
+            status = cmd.returncode
             Layout.Inst().PopDialogue()
             if status == 0:
                 Layout.Inst().PushDialogue(InfoDialogue(Lang("Metadata Restore Succeeded: ") + output))
@@ -137,40 +221,54 @@ class DRRestoreDialogue(SRDialogue):
         }
         SRDialogue.__init__(self) # Must fill in self.custom before calling __init__
 
+    def _searchForVDI(self, sr_uuid, legacy=False):
+        # probe for the restore VDI UUID
+        command = ["%s/xe-restore-metadata" % (Config.Inst().HelperPath(),), "-p", "-u", sr_uuid]
+        if legacy:
+            command.append("-o")
+        cmd = subprocess.Popen(command,
+                               stdout = subprocess.PIPE,
+                               stderr = subprocess.PIPE,
+                               universal_newlines = True)
+        output, errput = cmd.communicate()
+        status = cmd.returncode
+        if status != 0:
+            raise Exception("(%s,%s)" % (output,errput))
+        if len(output) == 0:
+            raise Exception(errput)
+        return output
+
+    def _earlierConfirmHandler(self, inYesNo, sr_uuid):
+        if inYesNo == 'y':
+            Layout.Inst().TransientBanner(Lang("Searching for backup VDI...\n\nCtrl-C to abort"))
+            try:
+                vdi_uuids = [v.strip() for v in self._searchForVDI(sr_uuid, legacy=True).splitlines()]
+                if len(vdi_uuids) == 1:
+                    _listBackups(sr_uuid, vdi_uuids[0], legacy=True)
+                else:
+                    Layout.Inst().PushDialogue(DRRestoreVDISelection(sr_uuid, vdi_uuids))
+                    return
+            except Exception as e:
+                Layout.Inst().PushDialogue(InfoDialogue( Lang("Metadata Restore failed: ")+Lang(e)))
+        else:
+            Layout.Inst().PushDialogue(InfoDialogue( Lang("Metadata Restore failed: a backup VDI could not be found")))
+        Data.Inst().Update()
+
     def DoAction(self, inSR):
         Layout.Inst().PopDialogue()
         Layout.Inst().TransientBanner(Lang("Searching for backup VDI...\n\nCtrl-C to abort"))
         sr_uuid = inSR['uuid']
         try:
-            # probe for the restore VDI UUID
-            command = "%s/xe-restore-metadata -p -u %s" % (Config.Inst().HelperPath(), sr_uuid)
-            cmd = subprocess.Popen(command,
-                                   stdout = subprocess.PIPE,
-                                   stderr = subprocess.PIPE,
-                                   shell = True)
-            output = "".join(cmd.stdout).strip()
-            errput = "".join(cmd.stderr).strip()
-            status = cmd.wait()
-            if status != 0:
-                raise Exception("(%s,%s)" % (output,errput))
-            if len(output) == 0:
-                raise Exception(errput)
-            vdi_uuid = output
-
-            # list the available backups
-            Layout.Inst().TransientBanner(Lang("Found VDI, retrieving available backups..."))
-            command = "%s/xe-restore-metadata -l -u %s -x %s" % (Config.Inst().HelperPath(), sr_uuid, vdi_uuid)
-            cmd = subprocess.Popen(command,
-                                   stdout = subprocess.PIPE,
-                                   stderr = subprocess.PIPE,
-                                   shell = True)
-            output = "".join(cmd.stdout).strip()
-            errput = "".join(cmd.stderr).strip()
-            status = cmd.wait()
-            if status != 0:
-                raise Exception("(%s,%s)" % (output,errput))
-            Layout.Inst().PushDialogue(DRRestoreSelection(output, vdi_uuid, sr_uuid))
-        except Exception, e:
+            try:
+                vdi_uuid = self._searchForVDI(sr_uuid).strip()
+            except Exception as e:
+                # We could not uniquely identify the required VDI, ask the user if they want to check for legacy ones
+                message = Lang("A backup VDI could not be positively identified. Do you wish to scan for backup VDIs created with earlier versions (Warning: this operation should only be performed if you trust the contents of all VDIs in this storage repository)?")
+                Layout.Inst().PushDialogue(QuestionDialogue(message, lambda x: self._earlierConfirmHandler(x, sr_uuid)))
+                return
+
+            _listBackups(sr_uuid, vdi_uuid)
+        except Exception as e:
             Layout.Inst().PushDialogue(InfoDialogue( Lang("Metadata Restore failed: ")+Lang(e)))
         Data.Inst().Update()
 
-- 
2.25.1