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

[Tyler Hicks]

Review: Disapprove

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

Diff comments:

> === modified file 'haptic/haptic.go'
> --- haptic/haptic.go	2015-04-22 23:06:23 +0000
> +++ haptic/haptic.go	2016-07-13 15:14:17 +0000
> @@ -66,7 +75,88 @@
>  	}
>  }
>  
> +func handlePropInterface(msg *dbus.Message) (reply *dbus.Message) {
> +        switch msg.Member {
> +        case "Get":
> +                var iname, pname string
> +                msg.Args(&iname, &pname)
> +                if iname == HAPTIC_DBUS_IFACE && pname == "OtherVibrate" {
> +                        reply = dbus.NewMethodReturnMessage(msg)
> +                        reply.AppendArgs(dbus.Variant{uint32(pvalue)})
> +                } else {
> +                        reply = dbus.NewErrorMessage(msg, "com.canonical.usensord.Error", "interface or property not correct")
> +                }
> +        case "GetAll":
> +                var iname string
> +                msg.Args(&iname)
> +                if iname == HAPTIC_DBUS_IFACE {
> +                        reply = dbus.NewMethodReturnMessage(msg)                        
> +                        reply.AppendArgs(dbus.Variant{uint32(pvalue)})
> +                } else {
> +                        reply = dbus.NewErrorMessage(msg, "com.canonical.usensord.Error", "interface or property not correct")
> +                }
> +        case "Set":
> +                var iname, pname string
> +                msg.Args(&iname, &pname, &pvalue)
> +                if iname == HAPTIC_DBUS_IFACE && pname == "OtherVibrate" && (pvalue == 1 || pvalue == 0) {
> +                        //save the property value
> +                        bs := []byte(strconv.FormatUint(uint64(pvalue), 10))
> +                        // write the whole body at once
> +                        errwrite := ioutil.WriteFile(PROP_FILE, bs, 0644)
> +                        if errwrite != nil {
> +                            logger.Println("WriteFile error:", errwrite)
> +                        }
> +
> +                        //reply = dbus.NewSignalMessage("/com/canonical/usensord/haptic", HAPTIC_DBUS_IFACE, "Set")
> +                        reply = dbus.NewMethodReturnMessage(msg)
> +                        reply.AppendArgs(dbus.Variant{uint32(pvalue)})
> +                        logger.Println("Set property to be ", pvalue)
> +                } else {
> +                        reply = dbus.NewErrorMessage(msg, "com.canonical.usensord.Error", "interface or property not correct")
> +                }
> +        default:
> +                logger.Println("Received unkown method call on", msg.Interface, msg.Member)
> +                reply = dbus.NewErrorMessage(msg, "org.freedesktop.DBus.Error.UnknownMethod", "Unknown method")
> +        }
> +        return reply
> +}
> +
>  func handleHapticInterface(msg *dbus.Message) (reply *dbus.Message) {
> +        messageBus := conn.Object("org.freedesktop.DBus", "/org/freedesktop/DBus")
> +        processreply, err := messageBus.Call("org.freedesktop.DBus", "GetConnectionCredentials", msg.Sender)
> +        if err != nil {
> +                reply = dbus.NewErrorMessage(msg, "com.canonical.usensord.Error", err.Error())
> +                return reply
> +        }
> +        var credentials map[string]dbus.Variant
> +        if err := processreply.Args(&credentials); err != nil {
> +                reply = dbus.NewErrorMessage(msg, "com.canonical.usensord.Error", err.Error())
> +                return reply
> +        }
> +        pid := credentials["ProcessID"].Value.(uint32)
> +        logger.Printf("caller process id: %d", pid)
> +        isOSK := false
> +        file := "/proc/" + strconv.FormatUint(uint64(pid), 10) + "/comm"

/proc/PID/comm can be trivially changed. See the proc(5) man page for details. You can't rely on comm to make a security related decision.

> +        if _, err := os.Stat(file); os.IsNotExist(err) {
> +                logger.Println("the comm for this pid not exist")
> +        } else {
> +                comm, erreadcomm := ioutil.ReadFile(file)
> +                if erreadcomm != nil {
> +                        logger.Println("faild to read the file:", file)
> +                } else {
> +                        pname := strings.TrimSpace(string(comm))
> +                        logger.Println("process name", pname)
> +                        if pname == OSK_PROCESS_NAME {
> +                            isOSK = true
> +                            logger.Println("OSK calling")
> +                        }
> +                }
> +        }
> +        if !isOSK && pvalue == 0 {
> +                logger.Println("not vibrate since not osk and pvalue is 0")
> +                reply = dbus.NewMethodReturnMessage(msg)
> +                return reply
> +        }
>  	switch msg.Member {
>  	case "Vibrate":
>  		var duration uint32


-- 
https://code.launchpad.net/~zhangew401/usensord/fix-lp-1433590/+merge/299959
Your team Ubuntu Phablet Team is subscribed to branch lp:usensord.