Skip to content
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.

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

Missing validation for data from D-Bus #302

Closed
joseivanlopez opened this issue Nov 14, 2022 · 2 comments
Closed

Missing validation for data from D-Bus #302

joseivanlopez opened this issue Nov 14, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@joseivanlopez
Copy link
Contributor

joseivanlopez commented Nov 14, 2022

The D-Bus API provided by D-Installer uses variant type for some values. For example, when calling to calculate a new storage proposal:

$ busctl call \
    org.opensuse.DInstaller.Storage \
    /org/opensuse/DInstaller/Storage/Proposal1 \
    org.opensuse.DInstaller.Storage.Proposal1 \
    Calculate \
    a{sv} 1 CandidateDevices as 1 /dev/vdc

There is no validation from backend size about the types sent, so it could fail if it receives an unexpected value:

$ busctl call \
    org.opensuse.DInstaller.Storage \
    /org/opensuse/DInstaller/Storage/Proposal1 \
    org.opensuse.DInstaller.Storage.Proposal1 \
    Calculate \
    a{sv} 1 CandidateDevices au 1 1000

The backend should verify the data received from D-Bus.

@mvidner
Copy link
Contributor

mvidner commented Nov 14, 2022

From the perspective of the D-Bus protocol and the ruby-dbus bindings library, this is out of scope: a Calculate method is getting an a{sv} argument and D-Bus has no other information to help in the validation. So it is the application's responsibility.

That said, there's an example from NM and a related bug in ruby-dbus:

Just for reference, we've recently seen how NetworkManager handles this kind of errors, in mvidner/ruby-dbus#124:

connection.permissions: wrong type; should be a list of strings. error_name=org.freedesktop.NetworkManager.Settings.Connection.InvalidProperty (DBus::Error)

ipv4.address-data: can't set property of type 'aa{sv}' from value of type 'av' error_name=org.freedesktop.NetworkManager.Settings.Connection.InvalidProperty (DBus::Error)

Even when ruby-dbus has the type information, for properties, it still fails to check the types. That's part of mvidner/ruby-dbus#118. Example:

$ sudo busctl --system \
  call \
  org.opensuse.DInstaller.Questions \
  /org/opensuse/DInstaller/Questions1 \
  org.opensuse.DInstaller.Questions1 \
  New sasas "should I stay or should I go" 2 yes no 1 yes
$ sudo busctl --system \
  set-property \
  org.opensuse.DInstaller.Questions \
  /org/opensuse/DInstaller/Questions1/1 \
  org.opensuse.DInstaller.Question1 \
  Answer i 13
Failed to set property Answer on interface org.opensuse.DInstaller.Question1: undefined method `to_sym' for 13:Integer
            backend.answer = option.to_sym

The exception comes from a random place in the method body, not from a typechecking guard.

So when I add the missing type checks, I'll keep in mind an option to let the methods be usable by applications for "custom properties".

@ancorgs
Copy link
Contributor

ancorgs commented May 15, 2024

Now that we have switched from D-Bus to HTTP as main communication protocol, I closing this. I know D-Bus is still used internally at some parts, but this is likely not worth any investment.

Feel free to reopen if you disagree.

@ancorgs ancorgs closed this as completed May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants