QNetworkSession::isOpen() always returns false

[Michael Zanetti]

Using QNetworkSession::isOpen() in confined apps on a phone running vivid always returns false. This might be an apparmor thing, however, I couldn't find any REJECTED entries in log files.

The test app in lp:~mzanetti/+junk/nmsessiontest can reproduce the issue. Open this project in ubuntu-sdk's qtcreator and run it on a vivid device. Press the button and watch the debug prints.

On a vivid-desktop or a RTM based phone it will print "all is well". On a vivid phone however, it'll print "network session not open..."

This used to work fine at least back in utopic images.

[Lorn Potter (lorn-potter)]

Looks like an app armour thing.

Debug-helper> Executing /usr/bin/qmlscene['/usr/bin/qmlscene', '$@', 'share/qml/nmsessiontest/nmsessiontest.qml']
process 7885: arguments to dbus_message_new_method_call() were incorrect, assertion "_dbus_check_is_valid_path (path)" failed in file ../../dbus/dbus-message.c line 1344.
This is normally a bug in some application using the D-Bus library.
process 7885: arguments to dbus_message_set_auto_start() were incorrect, assertion "message != NULL" failed in file ../../dbus/dbus-message.c line 3029.
This is normally a bug in some application using the D-Bus library.
process 7885: arguments to dbus_message_iter_init_append() were incorrect, assertion "message != NULL" failed in file ../../dbus/dbus-message.c line 2426.
This is normally a bug in some application using the D-Bus library.
QDBusConnection: error: could not send message to service "org.freedesktop.NetworkManager" path "" interface "org.freedesktop.DBus.Introspectable" member "Introspect":
using blocking call!
Using blocking call!
process 7885: arguments to dbus_message_new_method_call() were incorrect, assertion "_dbus_check_is_valid_path (path)" failed in file ../../dbus/dbus-message.c line 1344.
This is normally a bug in some application using the D-Bus library.
process 7885: arguments to dbus_message_set_auto_start() were incorrect, assertion "message != NULL" failed in file ../../dbus/dbus-message.c line 3029.
This is normally a bug in some application using the D-Bus library.
process 7885: arguments to dbus_message_iter_init_append() were incorrect, assertion "message != NULL" failed in file ../../dbus/dbus-message.c line 2426.
This is normally a bug in some application using the D-Bus library.
QDBusConnection: error: could not send message to service "org.freedesktop.NetworkManager" path "" interface "org.freedesktop.DBus.Introspectable" member "Introspect":
using blocking call!
Using blocking call!
UbuntuWindow::handleSurfaceFocusChange(focused=true)
UbuntuWindow::handleSurfaceResize(width=540, height=919)
UbuntuClipboard - Got invalid serialized mime data. Ignoring it.

There was another bug mentioning that abstract dbus interface does not introspect, whereas QDBusInterface does
QDBusInterface is used all over in the nm QtBearer plugin.

[Lorn Potter (lorn-potter)]

[05:34:57] <lpotter> QDBusConnection: error: could not send message to service "org.freedesktop.NetworkManager" path "" interface "org.freedesktop.DBus.Introspectable" member "Introspect":
[05:35:01] <lpotter> hmm
[05:35:24] <mzanetti> oh, you're still working. I hope it's not just for this
[05:35:34] <lpotter> morning time :)
[05:35:44] <lpotter> just thought I'd look at it
[05:35:50] <mzanetti> ok :)
[05:35:58] <mzanetti> that looks like apparmor
[05:36:05] <lpotter> there's also some other odd output
[05:36:15] <lpotter> yes, my thought too
[05:36:56] <mzanetti> not sure if we can convince jamie to allow apps calling Introspect on that interface
[05:40:09] <lpotter> there was another bug mentioning that abstract dbus interface does not introspect, whereas QDBusInterface does
[05:40:41] <mzanetti> ah, that might well be true
[05:40:56] <lpotter> I use QDBusInterface all over in those classes
[05:41:40] <mzanetti> hmm, I guess if that would solve the issue it would be way to go... I think it's also in upstream's interest to allow more fine grained control on Qt's permissions
[05:49:05] <lpotter> wish I could remember what bug was that. it was only yesterday I looked at it
[05:50:33] <lpotter> ahh 1403508
[05:54:05] <lpotter> I even thought about changing those to use QDBusAbstractInterface when I was updating it....

[Lorn Potter (lorn-potter)]

Added some error reporting:

QNetworkManagerInterface::QNetworkManagerInterface(QObject*) propsReply "An AppArmor policy prevents this sender from sending this message to this recipient, 0 matched rules; type="method_call", sender=":1.136" (uid=32011 pid=5705 comm="/usr/lib/arm-linux-gnueabihf/qt5/bin/qmlscene $@ s") interface="org.freedesktop.DBus.Properties" member="GetAll" error name="(unset)" destination="org.freedesktop.NetworkManager" (uid=0 pid=1314 comm="NetworkManager ")"

QNetworkManagerInterface::QNetworkManagerInterface(QObject*) nmReply "An AppArmor policy prevents this sender from sending this message to this recipient, 0 matched rules; type="method_call", sender=":1.136" (uid=32011 pid=5705 comm="/usr/lib/arm-linux-gnueabihf/qt5/bin/qmlscene $@ s") interface="org.freedesktop.NetworkManager" member="GetDevices" error name="(unset)" destination="org.freedesktop.NetworkManager" (uid=0 pid=1314 comm="NetworkManager ")"

[Michael Zanetti (mzanetti)]

This upstream patch removes the use of Introspect: https://codereview.qt-project.org/#/c/102665/

However, apparmor still prevents the required Get calls on properties so it still doesn't work and prints the error pasted in comment #3.

[Jamie Strandboge (jdstrand)]

There are explicit deny rules for talking to NetworkManager that silence the denials. Apps are supposed to use connectivity-api instead. (QNetworkSession::isOpen() could be adjusted to use the connectivity-api instead of NetworkManager).

[Lorn Potter (lorn-potter)]

QNetworkSession is part of Qt. Making isOpen use connectivity-api will not work, because the QNetworkSession state isn't necessarily a one to one relationship to the connectivity state.

QNetworkAccessManager and friends use QtBearer and the network manager backends it provides for network access.

So any Qml or Qt app that uses QNetworkAccessManager will not function correctly.

Take for example this:

QNetworkAccessManager *manager = new QNetworkAccessManager(this);
manager->get(QNetworkRequest(QUrl("http://qt-project.org")));

QNAM uses a QNetworkConfiguration internally, which in turn uses network manager via dbus.

[Dmitry Shachnev (mitya57)]

Is https://codereview.qt-project.org/102665 something we want to have in our Qt 5.3.2 packages? Should I include it in my next vivid upload?

[Michael Zanetti (mzanetti)]

@Dimitry, this upstream patch doesn't seem to completely get around this issue, however, it should for sure improve the situation and if we decide to punch a hole into apparmors policy, this patch would require less punching. So IMO, yes, let's add it.

[Launchpad Janitor (janitor)]

This bug was fixed in the package qtbase-opensource-src - 5.3.2+dfsg-4ubuntu9

---------------
qtbase-opensource-src (5.3.2+dfsg-4ubuntu9) vivid; urgency=medium

  [ Dmitry Shachnev ]
  * Add a patch to prefer QPA implementation for systemtrayicon, when it
    is available. This makes tray icons work on Plasma 5.
  * Refresh debian/patches/enable-tests.patch to not patch the same file
    twice.

  [ Timo Jyrinki ]
  * Update Use-a-property-cache-to-cut-down-on-blocking-calls.patch to
    not print warnings about using blocking call.
  * Add a patch Refactor-networkmanager-QtBearer-backend-to-use-QDBu.patch
    (LP: #1404188)
 -- Dmitry Shachnev <email address hidden> Tue, 27 Jan 2015 16:25:13 +0300

[Michael Zanetti (mzanetti)]

This is still not working

[Michael Zanetti (mzanetti)]

re-added apparmor-easyprof-ubuntu

[Jamie Strandboge (jdstrand)]

Sorry, this will not be fixed in apparmor-easyprof-ubuntu (which is why the task was removed previously) because NetworkManager is not designed to have arbitrary apps talking to it (this has been discussed at length elsewhere, which is why connectivity-api was written). This used to work fine for people-- did a patch get dropped somewhere else for using connectivity-api?

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Jamie Strandboge (jdstrand)]

Note, if someone presented a list of rules that need to be added to the policy that would fix this bug since 5.3.2+dfsg-4ubuntu9 was added, I'd be happy to review them. However, before you do, please see https://lists.launchpad.net/ubuntu-phone/msg04455.html: "It will talk to network-manager, but it needs wide permissions which reveal too much information (it will drill down far into the API which exposes MAC addresses, SSIDs, etc to apps). I tried giving only what made sense, but qtdeclaritive5-systeminfo-plugin didn't work right if it got some info from network-manager, but not all." I understand that this is about qtbase-opensource-src and QNetworkManagerInterface, but I've looked at NetworkManager accesses by various libs many times and it never ends well.

[Lorn Potter (lorn-potter)]

You do realize this means QNetworkAccessManager most likely will not work as well.

Any network request, GET, etc... done with Qt runs though QNetworkConfiguration, which uses the same backend plugin for accessing network-manager. In which case Qt should probably be configured with the -no-feature-bearermanagement to disable bearer management.

I cannot test this, as my ubuntu phone is currently as good as bricked.

[Timo Jyrinki (timo-jyrinki)]

There's now a PPA with a Qt build for vivid+overlay that allows you to experiment with the option of not using the NM bearer backend but the generic backend (with very limited functionality compared). With that you can get "all is fine" even with a click installed / confined nmsessiontest app. But this does bring the notorious bug #1357321 back. However, for comparison:

https://launchpad.net/~canonical-qt5-edgers/+archive/ubuntu/qt5-daily

That is:
sudo apt-add-repository ppa:canonical-qt5-edgers/qt5-daily
sudo apt update
sudo vi /etc/apt/preferences.d/silotest.pref
# Add the following content to the file:
Package: *
Pin: release o=LP-PPA-canonical-qt5-edgers-qt5-daily
Pin-Priority: 1002

sudo apt upgrade

The mentioned -no-feature-bearermanagement is a non-standard configuration parameter that is probably only suitable for specialized builds, so that can't even be considered for a general Ubuntu build. At least it broke building various network tests of the default test enable build: http://is.gd/YGcaY8 http://is.gd/TFSdQ1

[Timo Jyrinki (timo-jyrinki)]

The dummier generic plugin works since it returns a result without actually checking anything. It does not seem to be realistic option to switch to it.

Currently there's no other offered solution than the apparmor way, and this bug would still need to get fixed. Would it be possible to push the earlier attempt to somewhere in bzr and we could try to refine it? Maybe for example one could get the Michael's test app to work with more restrictions even if the qtsysteminfo would not like it enough.

[Timo Jyrinki (timo-jyrinki)]

The connectivity-api option was discussed in comment #6 and unlikely to solve the issue in a way to make developers like the bug filer happy. Other proper/robust solutions would include writing a proxy service to filter sensitive data out, or adding "a single nm api call that doesn't make the app traverse all of the nm api". Those however do not have a plan or an implementer at the moment.

For the apparmor policy relaxing option, Jamie opposes a policy that would expose MAC addresses, SSIDs etc to all apps. But there's possibility to try to experiment with the rules, by:

< jdstrand> this easiest thing to do is this: create an app with the networking policy group that does what people what it to do. install it. then modify the generated apparmor profile in /var/lib/apparmor/profiles for the app to remove the explicit deny rules for nm (because they silence the denials). then do sudo apparmor_parser -r /var/lib/apparmor/profiles/<profile>
< jdstrand> then tail -f /var/log/syslog in one terminal, then launch the app to use whatever api people are trying to use. in the syslog there will be an apparmor denial. add a rule to allow the access to the profile, then reload the profile
< jdstrand> then repeat until you have a full list of rules. important-- the rules should be very specific (ie, interface, path and method are specified) so it is clear everything that is being exposed
< jdstrand> people may want to do 'sudo sysctl -w kernel.printk_ratelimit=0' to suppress rate limiting

[Timo Jyrinki (timo-jyrinki)]

Playing with it, I came up with the attached minimal set of rules to get the test case working. There's a whole majority of Network Manager dbus calls still being denied, and three allowed. Despite that I assume this minimal set reveals the discussed "too much", but it might be useful reference.

Using the patch as is results in log noise due to default being apparently "audit deny", and I didn't know how to "deny everything without audit like before, but allow these three exceptions to pass".

[Timo Jyrinki (timo-jyrinki)]

The still denied ones.

[Jamie Strandboge (jdstrand)]

This is likely too specific (ActiveConnection/0):
+ dbus (send)
+ bus=system
+ interface="org.freedesktop.DBus.Properties"
+ path=/org/freedesktop/NetworkManager/ActiveConnection/0
+ member=GetAll
+ peer=(name=org.freedesktop.NetworkManager),

This is too lenient:
+ dbus (send)
+ bus=system
+ interface="org.freedesktop.DBus.Properties"
+ path=/org/freedesktop/NetworkManager
+ member=GetAll
+ peer=(name=org.freedesktop.NetworkManager),

The Get*() and Set() methods are precisely what I was talking about before because these methods specify what to get and set based on member arguments and thus cannot be further restriceted by apparmor. In other words, this method allows an app to get any property from network manager, which is far too lenient. Network manager is designed with a different trust model that does not fit within the context of trusted helpers. Namely, network manager relies on policy kit and assumes the user that is able to talk to it (via policy kit ACLs) is trusted. On touch, the app is not trusted but the user is allowed to talk to network manager. Therefore, network manager's current design does not afford itself to opening up its api in the manner requested by this patch.

Which gets us back to the several choices:
* use connectivity api instead. It was specifically designed for this. apps talk to connectivity api via its simple and safe api, and it talks to network manager.
* write a proxy that is able to inspect the member data and filter it accordingly.
* patch network manager to provide a safe api for the things apps need

connectivity api is implemented precisely to address the concerns in this bug and it seems clear (to me) that we should be using it. The proxy idea would work and other libraries wouldn't have to be patched (much) to use it, but is yet another service, is error prone and the hardest to implement in the short term. Patching network manager requires effort, increases maintenance costs and ultimately the same work for apps to use it as adjusting them to use connectivity api.

[Lorn Potter (lorn-potter)]

Not sure that QtBearer is being understood.
http://doc.qt.io/qt-5/bearer-management.html

QNetworkAccessManager and friends (QNetworkRequest, etc) use bearer management for starting and stopping connections when needed and if allowed by the system.

QtNetwork / Bearer Management cannot be made to use connectivity-api. connectivity-api is much too simple. There is no way to control the interfaces and start a connection (wifi/mobile data, etc) to be made when a network request is made and there is no existing connection. There is no way to get list of network configurations/wifi API's, etc.

The only realistic solution I see is to either only use generic plugin or completely compile out use of bearermanagement (QtBase configured with —no-feature-bearermanagement)

This way, people using QNetworkAccessManager and friends (QNetworkRequest, etc) (if that is even allowed by apparmor, I'm not too knowledgeable about what's allowed and what is not) to GET and whatnot will not be depending on QtBearerManagement and NetworkManager dbus API.
Developers won't be mislead that they will have a fully working bearer (generic plugin has many issues in regards to a fully working bearer).

[Jamie Strandboge (jdstrand)]

Thanks Lorn, and just so I'm clear-- I don't think that QtNetwork/etc should be modified if it doesn't make sense for it in the general case.

All I'm getting at is untrusted apps shouldn't be able to control interfaces and start a connection as you mentioned, let alone get privileged info out of network manager. These untrusted apps simply need to know if they are online or not and maybe some other details that connectivity-api can provide. How they get that information I don't particularly care so long as they don't get this privileged access.

It seems clear that QtBearer is written with the traditional session/policykit trust model, which is fine, but it doesn't align with the app store trust model where apps are untrusted by the system and session, so perhaps your idea makes sense. I'll let others work out the details of what needs to change and how.

[Lorn Potter (lorn-potter)]

QtBearer just runs on top of the system networking, so if the platform specifies it doesn't allow start/stop of interfaces, like the generic bearer plugin, it is allowed to return an error of 'not allowed' or somesuch.

It's the current network-manager bearer plugin that does not take into account security limitations.

That said, a QtBearer plugin could be written using connectivity-api. It would simply supply one configuration "System configuration" or some name and advertise it is not able to start and stop interfaces/connections.

Maybe I will do that today and you guys could use it or not.

[Lorn Potter (lorn-potter)]

Might try using bearer plugin based on connectivity-api

https://codereview.qt-project.org/#/c/140752/

[Tony Espy (awe)]

A couple of comments.

I've actually been chasing down a DBus performance issue on touch since mid-Summer, related to NetworkManager and WiFi scanning.

The high-level summary is that we have a number of system processes listening for NM AccessPoint DBus signals on specific AccessPoint DBus objects, and never cleaning up the associated DBus watch rules when the access points are removed. This can lead to a single processing adding 5k match rules to the system bus if WiFi is enabled long enough. For details see bug #1480877, an in particular from the following comment onwards:

https://bugs.launchpad.net/ubuntu-rtm/+source/sync-monitor/+bug/1480877/comments/65

I've traced what I think is the cause to the bearer mgmt networkmanager plugin and am attempting to patch it.

Note, this bug that I'm now adding this comment to, is about a confined app's usage of QNetworkSession. I should remind folks that there are a number of system processes ( eg. unity8 ) that either use QNetworkSession directly, or via QNetworkAccessManager, so we'll need to be careful we don't break anything if we decide to deprecate the networkmanager backend in favor of a connectivity API based backend.

[Timo Jyrinki (timo-jyrinki)]

We need only a subset of the NM bearer features so it would be ok to replace or patch to limit its behavior.

What we need is existing QNAM instances to not fall dead if user is on 3G + wifi and then disables wifi, or alternatively if on 3G and wifi gets enabled the connections should migrate to wifi. I don't know if connectivity-api currently has what would be needed for the bearer.

We do not need network controlling in bearer since indicator-network handles that.

[Lorn Potter (lorn-potter)]

I tried a working connectivity-api bearer plugin (verified with unconfined test app) with the attached example app, and I still got app armor errors:

Syslog> Jan 21 06:55:34 ubuntu-phablet dbus[1809]: apparmor="DENIED" operation="dbus_method_call" bus="session" path="/com/ubuntu/connectivity1/NetworkingStatus" interface="org.freedesktop.DBus.Properties" member="GetAll" mask="send" name="com.ubuntu.connectivity1" pid=11241 label="com.ubuntu.developer.mzanetti.nmsessiontest_myapp_0.1" peer_pid=2631 peer_label="unconfined"

Even if we wanted to move to connectivity-api, we would need to fix those.

After looking further atks the connectivity-api, it is quite limited and cannot even detect when network is using mobile data or wifi.

We could:
* Use connectivity-api knowing that bearer functionality will be mostly broken, but should allow basic bearer use, and allow QNAM & friends to function unhindered.

* add some functionality to connectivity-api

[Jamie Strandboge (jdstrand)]

Note, there is already a 'connectivity' policy group that should allow this, but it is not included by default. Please adjust your security manifest to use both "networking" and "connectivity".

[Lorn Potter (lorn-potter)]

After adding 'connectivity' to the manifest, it is working with connectivity-api plugin

[Lorn Potter (lorn-potter)]

If this is still an issue Silo 032 currently has a fix for this. Using this set of patches I do not recall having any issues with isOpen

[Michael Mess (michael-michaelmess)]

This bug seems to affect dekko (See Dan's comment on bug 1501912 ), but this is probably not critical.

[Michael Zanetti (mzanetti)]

I've just tested this. It is *NOT* fixed.

[Lorn Potter (lorn-potter)]

So the solution to this is using connectivity-api in a new bearer plugin.

The connectivity-api based bearer plugin is blocked by the bandwidthLimitations property not working correctly
https://bugs.launchpad.net/ubuntu/+source/indicator-network/+bug/1362592