xapi_vm: Implement RBAC checking for keys in VM.other_config and VM.platform

[last-genius]

Add per-key RBAC checking for VM.platform and VM.other_config, to cover a case where a lower-prileged user could circumvent permission checks on other_config:{pci,hvm_serial} and platform:hvm_serial.

Introduces a generic per-key RBAC checker for map setters based on Task's manual RBAC checker (introduced in a3f2c6e). Uses it for the fields above.

This is part of XSA-489 / CVE-2026-23562 and CVE-2026-42486

[last-genius]

Best reviewed outside of Github, with colorMoved set to distinguish new lines from moved lines.

[contificate]

Looks good to me. It'd be nice to get an "all clear" on the XenRT suites from Citrix.

[last-genius]

my unit test isn't testing exactly what it claims to (it created a role without any permissions at all, not vm-admin). it seems much more complicated to test something related to external auth in unit tests than I'd imagined, plugins are required in particular:

XENAPI_MISSING_PLUGIN: [ extauth-hook ]`

So I've dropped the unit test

[lindig]

This introduces a mechanism to match keys against permissions. Would it make sense to subject all keys to a role and some keys to more specific roles? For example, we could have a permission for * (all keys) and additionally more specific keys. This way we would have one mechanism to declare permissions.

[last-genius]

This introduces a mechanism to match keys against permissions. Would it make sense to subject all keys to a role and some keys to more specific roles? For example, we could have a permission for * (all keys) and additionally more specific keys. This way we would have one mechanism to declare permissions.

This is already the case, though in a less obvious way.

field gets default permissions from its object, thus for vm, * (all keys) are vm-admin-writeable by default:

    ~messages_default_allowed_roles:_R_VM_ADMIN

and then special keys have particular permissions:

            ~map_keys_roles:
              [
                ("pci", _R_POOL_ADMIN)
              ; ("folder", _R_VM_OP)
              ; ("XenCenter.CustomFields.*", _R_VM_OP)
              ]

These definitions are quite far away from each other in source code and are not obvious...

[last-genius]

I've verified this fixes the issue manually

> import xmlrpc.client
> x = xmlrpc.client.ServerProxy(ip)
> s = x.session.login_with_password(user, pw)['Value']
> ref=x.VM.get_by_uuid(s, uuid)['Value']

> print(x.VM.add_to_other_config(s, ref, 'pci', 'aaa'))
{'Status': 'Failure', 'ErrorDescription': ['RBAC_PERMISSION_DENIED', 'vm.add_to_other_config/key:pci', 'No permission in user session. (Roles with this permission: pool-admin)']}
> print(x.VM.set_other_config(s, ref, {}))
{'Status': 'Failure', 'ErrorDescription': ['RBAC_PERMISSION_DENIED', 'vm.remove_from_other_config/key:pci', 'No permission in user session. (Roles with this permission: pool-admin)']}
> print(x.VM.set_other_config(s, ref, {"pci":"ddd"}))
{'Status': 'Failure', 'ErrorDescription': ['RBAC_PERMISSION_DENIED', 'vm.add_to_other_config/key:pci', 'No permission in user session. (Roles with this permission: pool-admin)']}

[lindig]

Could/should we include the wildcard * key in the list?

[last-genius]

Could/should we include the wildcard * key in the list?

We could, code already supports it, not sure if we should

[lindig]

I think such an entry would add clarity; or have a comment that tells where the default role is coming from if the key does not match.

[last-genius]

Added another commit that a fixes a similar issue for {other_config,platform}:hvm_serial

[contificate]

Looks good to me, I'm glad the old matching logic for when this was done for task was helpful.

[lindig]

Is there room to factor out the trie implementation because ocaml/xapi/xapi_task.ml contains a very similar implementation? On the other hand, it's not a lot of code.

[last-genius]

Is there room to factor out the trie implementation because ocaml/xapi/xapi_task.ml contains a very similar implementation? On the other hand, it's not a lot of code.

I've moved the trie implementation from xapi_task.ml to helpers.ml entirely in the first commit

[Zhengchai]

There are 21 usage and mentioned in the XenServer customer docs about 'other_config, are there any change here will need to update the XenServer customer document also for the usage ofother_config`? Thanks