Merge lp:~zhangew401/usensord/fix-lp-1433590 into lp:usensord

/proc/PID/comm cannot be trusted for security related decisions. See inline comment.

Thanks Tyler.
can /proc/PID/cmdline be trusted? or do you have any other suggestion to identify the process name?

No, you can't trust /proc/PID/cmdline, either.

You can ask dbus for the AppArmor profile that is confining the connecting process. All click apps are confined by AppArmor and, therefore, cannot change to other profiles or escape confinement. All processes that are unconfined can be treated as "trusted" processes since they're services that are shipped as part of the image we produce.

To ask dbus for a peer's AppArmor confinement context, call GetConnectionCredentials and then use the LinuxSecurityLabel in the returned dict. See this for more info:

  https://dbus.freedesktop.org/doc/dbus-specification.html#bus-messages-get-connection-credentials

Then you'll need to use libapparmor's aa_splitcon() function to break the confinement context into a label and a mode. See the aa_splitcon(3) man page for details.

You can use the label to identify the dbus peer. It will be "unconfined" if it is a trusted process. I believe that will be the case for maliit-server. You can verify by running `ps auxZ | grep maliit-server` and looking at the first column to see its AppArmor confinement context.

Thanks a lot for your detailed explanation, Tyler
Please let me explain our requirement in the bug.
1. If the caller is OSK, we will do vibration always.
2. If the caller is app, including click package and others like system-settings who have used Toolkit in their implementation, we will do vibration based the value of exposed property OtherVibrate.

I don't know if system settings have unconfined profile. But from the info I got from Zosmbi, the user of Toolkit includes unconfined apps.

Thanks again.

Hi again Tyler,
For the bug, since OSK has unconfined AppArmor, so we can trusted its comm, is this correct?
If so, the logic is if it is unconfined, and then if its comm is maliit-server, then we are sure it is OSK. right?
Thanks a lot.

If you want to identify the executable, calling os.Readlink() on /proc/$PID/exe would be more appropriate:

    $ ps x | grep maliit
     5823 ? Ssl 2:38 maliit-server
    25788 pts/16 S+ 0:00 grep --color=auto maliit
    $ ls -l /proc/5823/exe
    lrwxrwxrwx 1 phablet phablet 0 Jul 7 11:47 /proc/5823/exe -> /usr/bin/maliit-server

I'd combine that with the a check that the security label is "unconfined" as Tyler suggested (which you can do using the code fragment I gave via mail). That should be enough to ensure you aren't being faked out by an untrusted application, and are talking to the expected system service.

As for other comments on the branch:

Hard coding /home/phablet looks wrong. If you want to determine the user's home directory, the os/user package should help. Something like:

    u, err := user.Current()
    configFile := path.Join(u.HomeDir, ".config", "usensord", "whatever")

Also, storing a plain integer in the file could limit what you can do going forward if you need to add more configuration settings. Perhaps consider using something a bit more structured that will let you add more settings later without breaking forward or backward compatibility. Go's json package might be a good fit here.

Thanks so much James.
@Tyler, if you also agree with the proposition by James, I will begin to do it.
Thanks.

I don't love the approach of reading /proc/PID/exe and I have some real blockers that I mention inline below.

Since your entire goal is to identify maliit-server, I'd much prefer that you confine maliit-server with an AppArmor profile (it could even be loosely confined at first). You'd then call GetConnectionCredentials() and use the LinuxSecurityLabel to identify maliit-server by its AppArmor profile name. This approach has the benefit that we confine another process on the system (more hardening) and don't have to rely on oddities in the behavior of /proc/PID/exe (see proc(5) man page for an example seen in child threads).

Confining maliit-server could be easy or it could become more difficult if it requires changes to other AppArmor profiles. I can't really say at this point but I'd like for this approach to at least be considered. If it is too complex then you should at least address the issues I mention inline below.

Thanks for working through this merge feedback! :)

Thank you so much for suggestions, Tyler.
Sorry I don't quite understand "I'd much prefer that you confine maliit-server with an AppArmor profile", do you mean we need to change the AppArmor profile of maliit-server?

I will address your mentioned issues at least at the time.
Thank you.

Is maliit-server confined by an AppArmor profile on your device? It isn't on my emulator instance:

root@ubuntu-phablet:~# system-image-cli -i
current build number: 27
device name: generic_x86
channel: ubuntu-touch/rc/ubuntu
last update: 2016-07-08 05:47:59
version version: 27
version ubuntu: 20160708
version device: 20160401.1
version custom: 20160708
root@ubuntu-phablet:~# ps auxZ | grep maliit
unconfined phablet 2120 0.2 8.3 278072 42392 ? Ssl 19:47 0:00 maliit-server

(the first column is the AppArmor confinement context)

What I'm suggesting is that you confine maliit-server with an AppArmor profile and then you can securely identify it.

It is unconfined on the device.
unconfined phablet 3067 8.5 4.1 281312 40820 ? Ssl 14:18 0:01 maliit-server
Let me ask OSK team if they can confine maliit-server with and AppArmor profile.

Thanks Zhang for working with us on this; this looks good, with a few small points:

"unkown", "faild"

Is this line correct?
if _, err := os.Lstat(file); os.IsNotExist(err) {

There are many possible reasons why the os.Lstat() might fail, it would probably be better to just log the actual error.

The error information from erreadexe isn't printed.

os.MkdirAll(configPath, 0776)

This mode looks funny; why not 0700 or 0750 or 0755? What are the goals?

Thanks

Thanks Seth.
0776 is really my careless fault. I will update accordingly soon.

Hi Zhang, Tyler

 When you say that you'd like the maliit-server confined with an apparmor profile, do you purely need this for identification at the moment? i.e. could we just add a profile that uses the unconfined template? Or do you actually need more strict confinement of maliit-server?

Hi Mike,
IMHO, we only need this for identification. i.e. to use the unconfined template with a name, then I can use that name for identification in usensord.
@Seth, could you please help check if my understanding is correct? Thanks a lot.

Correct, using an AppArmor profile for identification and not confinement is fine at this point. (Though I think meaningful improvements of security via a better-than-unconfined profile can still be made with just half an hour or so of profiling work.)

Thanks

Thanks a lot Seth.
@Mike, I think you need to decide if we only use one named unconfined profile or a better-than-unconfined profile.
Anyway I believe the name must be unique. Thanks so much. And also please send me the binary and tell me the name then I could test usensord. Thank you.

The changes look good to me, thanks!

Thanks Seth.
I will update usensord if Mike can do the better solution from OSK.

Hi John,
Could you please help review this patch to fix lp:1433590?
I need your top approval on it.
Thanks.

I'm not happy to approve it until tyhicks is +1

Hi Tyler,
Could you please help review again? Thanks a lot.

Looks good. Thanks for iterating on this!

Thanks Tyler.
@John, could you please help review?
Thanks.

I proposed https://code.launchpad.net/~pat-mcgowan/ubuntu-system-settings/other-vibrations/+merge/303566 with the system settings support.

The default should be to have this enabled since it is on for everyone now, can you make that change?

The default should be to have this enabled since it is on for everyone now

Hi Pat,
Done and silo17 verified.

Thanks Bill for top approval.
@Bill and Pat, sorry I am not familiar with the process.
After this branch is approved, do I need to manually merge it back to lp:usensord?
Thanks a lot.