Skip to content

Improve VirtualBox descriptor parsing #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
Schamper wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Improve VirtualBox descriptor parsing #71

Schamper wants to merge 2 commits into from
+900 −444

Conversation

@Schamper
Copy link
Member

@Schamper Schamper commented Dec 22, 2025

Fixes #70.

@codecov
Copy link

codecov bot commented Dec 22, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 89.62963% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.64%. Comparing base (b19a789) to head (aa365d6).

Files with missing lines Patch % Lines
dissect/hypervisor/descriptor/vbox.py 89.62% 14 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #71      +/-   ##
==========================================
+ Coverage   78.01%   78.64%   +0.62%     
==========================================
  Files          26       26              
  Lines        2202     2332     +130     
==========================================
+ Hits         1718     1834     +116     
- Misses        484      498      +14
Flag Coverage Δ
unittests 78.64% <89.62%> (+0.62%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Schamper Schamper force-pushed the improve-vbox branch 2 times, most recently from 8901abe to de2c7f5 Compare January 9, 2026 15:24
Miauwkeru
Miauwkeru reviewed Jan 20, 2026
View reviewed changes
Copy link
Contributor

@Miauwkeru Miauwkeru left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there already a PR in dissect.target that incorporates these changes? seeing as vbox.disks isn't a thing anymore

tests/descriptor/test_hyperv.py
@@ -1,63 +1,64 @@
from __future__ import annotations
Copy link
Contributor

@Miauwkeru Miauwkeru Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand most of these tests have been changed due to the changes for the vbox. Once this gets squashed and merged I wouldn't expect a commit called "Improve VirtualBox descriptor parsing" to also touch and change all the different tests.

Copy link
Member Author

@Schamper Schamper Jan 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split them up so we can merge without squashing.

dissect/hypervisor/descriptor/vbox.py
return None


class HardDisk:
Copy link
Contributor

@Miauwkeru Miauwkeru Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it not be useful to also have the format of the disk available? Not for target-query, but for the package as its own entity

Copy link
Contributor

@Miauwkeru Miauwkeru Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed that a HardDisk can have an additional <Property name="CRYPT/KeyID"> and <Property name="CRYPT/KeyStore">. These exist when you tell virtualbox that it should encrypt a vm. It creates it for every associated disk in that case.

I think it might be useful to note down if the hard disk is encrypted. I can give you an example file if needed.

Copy link
Member Author

@Schamper Schamper Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have an example, sure.

Copy link
Contributor

@Miauwkeru Miauwkeru Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

<?xml version="1.0"?>
<!--
** DO NOT EDIT THIS FILE.
** If you make changes to this file while any VirtualBox related application
** is running, your changes will be overwritten later, without taking effect.
** Use VBoxManage or the VirtualBox Manager GUI to make changes.
**
** Written by VirtualBox 7.2.4 (r170995)
-->
<VirtualBox xmlns="http://www.virtualbox.org/" version="1.19-linux">
  <Machine uuid="{366f53dd-1710-434e-8021-1f91b0405b65}" name="encrypted test" OSType="Windows11_64" currentSnapshot="{cfbdc706-6561-44e8-8f3a-236df446b808}" snapshotFolder="Snapshots" lastStateChange="2026-01-20T13:01:05Z">
    <MediaRegistry>
      <HardDisks>
        <HardDisk uuid="{24cdb8e9-35d6-42f2-aa17-c2d78bf1e1de}" location="encrypted test.vdi" format="VDI" type="Normal">
          <Property name="CRYPT/KeyId" value="encrypted test"/>
          <Property name="CRYPT/KeyStore" value="U0NORQABQUVTLVhUUzI1Ni1QTEFJTjY0AAAAAAAAAAAAAAAAAABQQktERjItU0hB&#10;MjU2AAAAAAAAAAAAAAAAAAAAAAAAAEAAAACOEfHKuPU9MHJktr3UZAxmuL+epJsk&#10;SRfK3vhbPnb4USAAAABCoUSl+kb0+6OzG8N3wo16bru7UNsjHzIsB+OPBfZUPiBO&#10;AACa0Cbl1wJz09E5i8U10V3xElnY8iAIwEqSwVWpwFPiYsAJDgBAAAAAgcb2cMho&#10;wWkDpje3pQHrn62sH8oTyr4+wXaJoFUEreymEuIXYlnB+jeLjQDWEEIs9DUIXfiP&#10;PWw3Fph9h0a3lA=="/>
          <HardDisk uuid="{742e6d0f-8896-4aa6-97f4-e05d70e73029}" location="Snapshots/{742e6d0f-8896-4aa6-97f4-e05d70e73029}.vdi" format="VDI"/>
        </HardDisk>
        <HardDisk uuid="{1db0b9fe-36c2-44e8-9b7c-61fa5b6d1462}" location="encrypted test_1.vdi" format="VDI" type="Normal"/>
      </HardDisks>
    </MediaRegistry>
    <Snapshot uuid="{cfbdc706-6561-44e8-8f3a-236df446b808}" name="Encryption" timeStamp="2026-01-20T13:01:05Z">
      <Hardware>
        <Memory RAMSize="4096"/>
        <HID Pointing="USBTablet"/>
        <Display controller="VBoxSVGA" VRAMSize="128"/>
        <Firmware type="EFI"/>
        <BIOS>
          <IOAPIC enabled="true"/>
          <NVRAM path="Snapshots/2026-01-20T13-01-05-717390000Z.nvram"/>
          <SmbiosUuidLittleEndian enabled="true"/>
          <AutoSerialNumGen enabled="true"/>
        </BIOS>
        <TrustedPlatformModule type="v2_0" location=""/>
        <USB>
          <Controllers>
            <Controller name="XHCI" type="XHCI"/>
          </Controllers>
        </USB>
        <Network>
          <Adapter slot="0" enabled="true" MACAddress="0800274C9CFB" type="82540EM">
            <NAT localhost-reachable="true"/>
          </Adapter>
        </Network>
        <AudioAdapter controller="HDA" useDefault="true" driver="ALSA" enabled="true" enabledOut="true"/>
        <Clipboard/>
        <StorageControllers>
          <StorageController name="SATA" type="AHCI" PortCount="2" useHostIOCache="false" Bootable="true" IDE0MasterEmulationPort="0" IDE0SlaveEmulationPort="1" IDE1MasterEmulationPort="2" IDE1SlaveEmulationPort="3">
            <AttachedDevice type="HardDisk" hotpluggable="false" port="0" device="0">
              <Image uuid="{24cdb8e9-35d6-42f2-aa17-c2d78bf1e1de}"/>
            </AttachedDevice>
            <AttachedDevice passthrough="false" type="DVD" hotpluggable="false" port="1" device="0"/>
          </StorageController>
        </StorageControllers>
        <CPU count="2">
          <HardwareVirtExLargePages enabled="false"/>
          <PAE enabled="false"/>
          <LongMode enabled="true"/>
        </CPU>
      </Hardware>
    </Snapshot>
    <Hardware>
      <Memory RAMSize="4096"/>
      <HID Pointing="USBTablet"/>
      <Display controller="VBoxSVGA" VRAMSize="128"/>
      <Firmware type="EFI"/>
      <BIOS>
        <IOAPIC enabled="true"/>
        <SmbiosUuidLittleEndian enabled="true"/>
        <AutoSerialNumGen enabled="true"/>
      </BIOS>
      <TrustedPlatformModule type="v2_0" location=""/>
      <USB>
        <Controllers>
          <Controller name="XHCI" type="XHCI"/>
        </Controllers>
      </USB>
      <Network>
        <Adapter slot="0" enabled="true" MACAddress="0800274C9CFB" type="82540EM">
          <NAT localhost-reachable="true"/>
        </Adapter>
      </Network>
      <AudioAdapter controller="HDA" useDefault="true" driver="ALSA" enabled="true" enabledOut="true"/>
      <Clipboard/>
      <StorageControllers>
        <StorageController name="SATA" type="AHCI" PortCount="3" useHostIOCache="false" Bootable="true" IDE0MasterEmulationPort="0" IDE0SlaveEmulationPort="1" IDE1MasterEmulationPort="2" IDE1SlaveEmulationPort="3">
          <AttachedDevice type="HardDisk" hotpluggable="false" port="0" device="0">
            <Image uuid="{742e6d0f-8896-4aa6-97f4-e05d70e73029}"/>
          </AttachedDevice>
          <AttachedDevice passthrough="false" type="DVD" hotpluggable="false" port="1" device="0"/>
          <AttachedDevice type="HardDisk" hotpluggable="false" port="2" device="0">
            <Image uuid="{1db0b9fe-36c2-44e8-9b7c-61fa5b6d1462}"/>
          </AttachedDevice>
        </StorageController>
      </StorageControllers>
      <CPU count="2">
        <HardwareVirtExLargePages enabled="false"/>
        <PAE enabled="false"/>
        <LongMode enabled="true"/>
      </CPU>
    </Hardware>
  </Machine>
</VirtualBox>

From how I interpreted it, the encryption holds for all parent disks and their children.
In the case above I added another disk after enabling the "Encrypt all disks" tho it seems it doesn't add a key for that one. Whereas it does add the properties if you enable encryption with multiple disks attached to the vm.

Copy link
Member Author

@Schamper Schamper Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case above I added another disk after enabling the "Encrypt all disks" tho it seems it doesn't add a key for that one. Whereas it does add the properties if you enable encryption with multiple disks attached to the vm.

But the second disk is encrypted? Presumably using the same key as the other one?

Copy link
Contributor

@Miauwkeru Miauwkeru Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the second disk is encrypted? Presumably using the same key as the other one?

The second disk is not encrypted. Had to verify it to be sure. Tho the wording "encrypt all disks" made me assume otherwise at first.

@Schamper
Copy link
Member Author

Schamper commented Jan 20, 2026

Is there already a PR in dissect.target that incorporates these changes? seeing as vbox.disks isn't a thing anymore

Not yet.

@Miauwkeru
Copy link
Contributor

Miauwkeru commented Jan 20, 2026

Is there already a PR in dissect.target that incorporates these changes? seeing as vbox.disks isn't a thing anymore

Not yet.

The parsing looks good to me, I will approve the PR once there is a PR in target for the virtualbox changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Miauwkeru Miauwkeru Miauwkeru left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

VirtualBox descriptor parsing is bad

3 participants

@Schamper @Miauwkeru