Updates to Portable SR Functionality (master) #5842

alexbrett · 2024-07-16T12:30:47Z

XSA-459 (https://xenbits.xen.org/xsa/advisory-459.html) covers a security issue with the Portable SR (also known as VM Metadata Backup/Restore) functionality.

Now the embargo has lifted, this PR includes:

Fixes to dry-run handling
Fixes to vbd cleanup
Forward port from 1.249-lcm of a security improvement to use debugfs rather than probe-device-for-file
The security fix for XSA-459

Shell quoting changes in xen-api.git 65f152d broke the dry-run functionality, as by quoting parameters in the way it was done it meant the space separation was not properly handled to separate parameters etc. Signed-off-by: Alex Brett <alex.brett@cloud.com>

The xe-[backup,restore]-metadata scripts have cleanup logic designed to ensure we do not leave any vbd objects etc behind. This logic calls `vbd-unplug` with a 20s timeout, and then (regardless of the result) allows up to 10s for any device specified in the VBD to disappear - if it does not, it does not trigger a `vbd-destroy`. This logic fails in the case where a VDI is attached to a VM running on the same host, as the `device` field in the new VBD will be populated with the backend device for the running VM. In this case, the `vbd-unplug` fails immediately (as the vbd is not plugged because the original `vbd-plug` attempt fails as the VDI is in use), but then we sit waiting for 10s for the device to disappear (which is obviously does not), and then fail to trigger a `vbd-destroy`, leaving the VBD behind. Fix this by simply removing the wait for the device to disappear and always attempting a `vbd-destroy`, as I am not aware of any situation where this additional 10s wait will give any benefit given current behaviours. Signed-off-by: Alex Brett <alex.brett@cloud.com>

This patch makes the feature use the debugfs utility, part of e2fsprogs. This makes the system as a whole a heck of a lot better, if only because it won't be able to parse XFS, ReiserFS or any of the many plugins of libfsimage. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

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 is XSA-459 / CVE-2024-31144 Signed-off-by: Alex Brett <alex.brett@cloud.com>

scripts/xe-restore-metadata

+
+     if [ -e "${mnt}/.ctxs-metadata-backup" ]; then
+        ${debug} echo "Found backup metadata on VDI: ${vdi_uuid}" >&2
+       matched_vdis+=( ${vdi_uuid} )


scripts/xe-restore-metadata

+vdi_uuid=${matched_vdis[0]}
+xe vdi-param-set uuid="${vdi_uuid}" other-config:ctxs-pool-backup=true
+createvbd
+if [ $? -ne 0 ]; then


scripts/xe-restore-metadata

+  exit 1
+fi
+mountvbd
+if [ $? -ne 0 ]; then


scripts/xe-restore-metadata

@@ -319,8 +362,8 @@
 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}"
-   "@OPTDIR@/bin/xe" vm-import filename="${full_dir}/${meta}" sr-uuid="${sr_uuid}" --metadata --preserve"${force_flag}""${dry_run_flag}"
+   echo xe vm-import filename="${meta}" sr-uuid="${sr_uuid}" --metadata --preserve${force_flag}${dry_run_flag}


scripts/xe-restore-metadata

-   echo xe vm-import filename="${meta}" sr-uuid="${sr_uuid}" --metadata --preserve"${force_flag}""${dry_run_flag}"
-   "@OPTDIR@/bin/xe" vm-import filename="${full_dir}/${meta}" sr-uuid="${sr_uuid}" --metadata --preserve"${force_flag}""${dry_run_flag}"
+   echo xe vm-import filename="${meta}" sr-uuid="${sr_uuid}" --metadata --preserve${force_flag}${dry_run_flag}
+   "@OPTDIR@/bin/xe" vm-import filename="${full_dir}/${meta}" sr-uuid="${sr_uuid}" --metadata --preserve${force_flag}${dry_run_flag}


scripts/xe-restore-metadata

-   echo xe vm-import filename="${meta}" sr-uuid="${sr_uuid}" --metadata --preserve"${force_flag}""${dry_run_flag}"
-   "@OPTDIR@/bin/xe" vm-import filename="${full_dir}/${meta}" sr-uuid="${sr_uuid}" --metadata --preserve"${force_flag}""${dry_run_flag}"
+   echo xe vm-import filename="${meta}" sr-uuid="${sr_uuid}" --metadata --preserve${force_flag}${dry_run_flag}
+   "@OPTDIR@/bin/xe" vm-import filename="${full_dir}/${meta}" sr-uuid="${sr_uuid}" --metadata --preserve${force_flag}${dry_run_flag}


alexbrett · 2024-07-16T12:33:42Z

Associated xsconsole PR xapi-project/xsconsole#47

scripts/xe-backup-metadata

- Quote a parameter - Adjust how we check the returncode of some function calls to satifsy shellcheck - Disable the warnings where we are explicitly relying on string splitting Signed-off-by: Alex Brett <alex.brett@cloud.com>

scripts/xe-restore-metadata

lindig · 2024-07-17T12:43:31Z

@alexbrett can you address the CI problems?

alexbrett · 2024-07-17T13:05:07Z

@alexbrett can you address the CI problems?

As far as I can see the one remaining issue is a false positive (and not from anything I've touched) - it claims yes is unused, but it is actually used (see e.g. line 331), so not sure how to deal with this (I could add a comment to make shellcheck ignore that rule, but that doesn't really feel like the right thing to do...)

lindig · 2024-07-17T13:08:11Z

@edwintorok can you review this

edwintorok · 2024-07-19T13:20:36Z

There are 2 files: xe-backup-metadata and xe-restore-metadata.
yes inside xe-restore-metadata is used.
yes inside xe-backup-metadata is unused, and can be deleted.

grep yes xe-backup-metadata xe-restore-metadata
xe-backup-metadata:    echo " -y: Assume non-interactive mode and yes to all questions"
xe-backup-metadata:yes=0
xe-backup-metadata:    y) yes=1 ;;
xe-restore-metadata:    echo " -y: Assume non-interactive mode and yes to all questions"
xe-restore-metadata:yes=0
xe-restore-metadata:    y) yes=1 ;;
xe-restore-metadata:if [ "$yes" -eq 0 ]; then
xe-restore-metadata:   echo "Please type in 'yes' and <enter> to continue."
xe-restore-metadata:   if [ "$response" != "yes" ]; then

edwintorok · 2024-07-19T13:24:40Z

Suggested fix (we do want to retain the flag for backward compat):

diff --git a/scripts/xe-backup-metadata b/scripts/xe-backup-metadata
index 9aac72573..63c2b8811 100755
--- a/scripts/xe-backup-metadata
+++ b/scripts/xe-backup-metadata
@@ -70,7 +70,6 @@ just_find_vdi=0
 fs_uninitialised=0
 usage_alert=90
 force_backup=0
-yes=0
 while getopts "yhvink:u:dcf" opt ; do
     case $opt in
     h) usage ;;
@@ -81,7 +80,8 @@ while getopts "yhvink:u:dcf" opt ; do
     d) leave_mounted=1 ;;
     n) just_find_vdi=1 ;;
     v) debug="" ;;
-    y) yes=1 ;;
+    y) # ignored for backward compat
+      ;;
     f) force_backup=1 ;;
     *) echo "Invalid option"; usage ;;
     esac

edwintorok · 2024-07-19T13:27:00Z

The reason shellcheck complains about this now, is that previously there was code that using "$yes" -eq 0, and that code got deleted in this PR, turning yes into an unused variable.

alexbrett · 2024-07-19T13:28:03Z

Suggested fix (we do want to retain the flag for backward compat):

It was only introduced in 7f1d315 as part of the original changes to bring in some of this functionality, so we can probably safely remove it entirely as I don't think any other components/users will have consumed it?

lindig · 2024-07-19T13:51:32Z

@alexbrett are you going to update your PR?

This parameter was added in 7f1d315, but the changes to always use the new metadata VDIs with known UUIDs means it is no longer required, so remove it. Signed-off-by: Alex Brett <alex.brett@cloud.com>

alexbrett and others added 4 commits July 16, 2024 11:18

github-advanced-security bot found potential problems Jul 16, 2024

View reviewed changes

alexbrett mentioned this pull request Jul 16, 2024

CP-49228: Modifications to Portable SR functionality xapi-project/xsconsole#47

Merged

lindig approved these changes Jul 16, 2024

View reviewed changes

edwintorok reviewed Jul 16, 2024

View reviewed changes

scripts/xe-backup-metadata Show resolved Hide resolved

Fixes for shellcheck

f3ad0bc

- Quote a parameter - Adjust how we check the returncode of some function calls to satifsy shellcheck - Disable the warnings where we are explicitly relying on string splitting Signed-off-by: Alex Brett <alex.brett@cloud.com>

alexbrett force-pushed the XSA-459_master branch from 716607a to f3ad0bc Compare July 16, 2024 13:22

edwintorok reviewed Jul 16, 2024

View reviewed changes

scripts/xe-restore-metadata Show resolved Hide resolved

Remove unused yes parameter in xe-backup-metadata

783988a

This parameter was added in 7f1d315, but the changes to always use the new metadata VDIs with known UUIDs means it is no longer required, so remove it. Signed-off-by: Alex Brett <alex.brett@cloud.com>

lindig approved these changes Jul 19, 2024

View reviewed changes

edwintorok approved these changes Jul 19, 2024

View reviewed changes

lindig merged commit 2189996 into xapi-project:master Jul 19, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to Portable SR Functionality (master) #5842

Updates to Portable SR Functionality (master) #5842

alexbrett commented Jul 16, 2024

alexbrett commented Jul 16, 2024

lindig commented Jul 17, 2024

alexbrett commented Jul 17, 2024

lindig commented Jul 17, 2024

edwintorok commented Jul 19, 2024 •

edited

Loading

edwintorok commented Jul 19, 2024 •

edited

Loading

edwintorok commented Jul 19, 2024

alexbrett commented Jul 19, 2024

lindig commented Jul 19, 2024

Updates to Portable SR Functionality (master) #5842

Updates to Portable SR Functionality (master) #5842

Conversation

alexbrett commented Jul 16, 2024

alexbrett commented Jul 16, 2024

lindig commented Jul 17, 2024

alexbrett commented Jul 17, 2024

lindig commented Jul 17, 2024

edwintorok commented Jul 19, 2024 • edited Loading

edwintorok commented Jul 19, 2024 • edited Loading

edwintorok commented Jul 19, 2024

alexbrett commented Jul 19, 2024

lindig commented Jul 19, 2024

edwintorok commented Jul 19, 2024 •

edited

Loading

edwintorok commented Jul 19, 2024 •

edited

Loading