[PATCH 0/4] backlight-helper: fixes and improvements

[PATCH 0/4] backlight-helper: fixes and improvements

[Hans de Goede]

Hi All,

I've tried the backlight code as merged with X running as a regular user +
pkexec, and they work as advertised.

When reviewing the changes I noticed some minor and not so minor (security!)
issues, which this patch series address.

Thanks & Regards,

Hans

[PATCH 1/4] backlight: Explain better why we support both pkexec and suid root for the helper

[Hans de Goede]

Update the comment about trying suid-root first with some explanations of
why pkexec may be preferable in some cases.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 src/backlight.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/backlight.c b/src/backlight.c
index 70c6559..0e63ba5 100644
--- a/src/backlight.c
+++ b/src/backlight.c
@@ -270,8 +270,10 @@ static int __backlight_helper_init(struct backlight *b, char *iface)
 	int use_pkexec = 0;
 	int fds[2];
 
-	/* If system policy is to disallow setuid helpers,
-	 * we fallback to invoking PolicyKit. However, as pkexec
+	/*
+	 * Some systems may prefer using PolicyKit's pkexec over
+	 * making the helper suid root, since the suid option will allow
+	 * anyone to control the backlight.  However, as pkexec
 	 * is quite troublesome and not universally available, we
 	 * still try the old fashioned and simple method first.
 	 * Either way, we have to trust that it is our backlight-helper
-- 
1.8.5.3

[PATCH 2/4] backlight: Use System instead of system when checking for pkexec

[Hans de Goede]

Event though we've failed to open the backlight normally, we may still be
running under a suid-root xserver, so use the servers build in System instead
of system so as to properly drop root rights.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 src/backlight.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/backlight.c b/src/backlight.c
index 0e63ba5..518d756 100644
--- a/src/backlight.c
+++ b/src/backlight.c
@@ -41,6 +41,7 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <dirent.h>
+#include <xf86.h>
 
 #include "backlight.h"
 #include "fd.h"
@@ -283,7 +284,7 @@ static int __backlight_helper_init(struct backlight *b, char *iface)
 		return 0;
 
 	if ((st.st_mode & (S_IFREG | S_ISUID | S_IXUSR)) != (S_IFREG | S_ISUID | S_IXUSR)) {
-		if (system("pkexec --version"))
+		if (System("pkexec --version"))
 			return 0;
 
 		use_pkexec = 1;
-- 
1.8.5.3

[PATCH 3/4] backlight: Drop rights before executing pkexec

[Hans de Goede]

Event though we've failed to open the backlight normally, we may still be
running under a suid-root xserver, so drop any elevated rights before
executing what we hope will be pkxec.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 src/backlight.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/backlight.c b/src/backlight.c
index 518d756..dc26307 100644
--- a/src/backlight.c
+++ b/src/backlight.c
@@ -295,6 +295,10 @@ static int __backlight_helper_init(struct backlight *b, char *iface)
 
 	switch ((b->pid = fork())) {
 	case 0:
+		if (setgid(getgid()) != 0)
+			_exit(127);
+		if (setuid(getuid()) != 0)
+			_exit(127);
 		close(fds[1]);
 		dup2(fds[0], 0);
 		close(fds[0]);
-- 
1.8.5.3

[PATCH 4/4] backlight-helper: Simplify reading the level from stdin

[Hans de Goede]

Since the helper is a standalone app, the usual xserver rules of not using
stdio because of signal handling don't apply.

And since the helper does run with elevated rights, it is important to keep
the code KISS so that it can be audited easily.

This commit replaces the hard to read "raw" read loop with a much simpler
loop using fgets, improving readability of the code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 tools/backlight_helper.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/tools/backlight_helper.c b/tools/backlight_helper.c
index fc16fce..11abebc 100644
--- a/tools/backlight_helper.c
+++ b/tools/backlight_helper.c
@@ -9,7 +9,7 @@
 int main(int argc, char *argv[])
 {
 	struct stat st;
-	char buf[1024], *b = buf;
+	char buf[1024];
 	int len, fd;
 
 	if (argc != 2) {
@@ -24,27 +24,12 @@ int main(int argc, char *argv[])
 		return 1;
 	}
 
-	while ((len = read(0, b, sizeof(buf) - (b - buf) - 1)) > 0) {
-		len += b - buf;
-		buf[len] = '\0';
-
-		b = buf;
-		do {
-			char *end = strchr(b, '\n');
-			if (end == NULL)
-				break;
-
-			++end;
-			if (write(fd, b, end - b) != end - b) {
-				fprintf(stderr, "Failed to update backlight interface '%s'\n", argv[1]);
-				return 2;
-			}
-
-			b = end;
-		} while (1);
-
-		memmove(buf, b, len = buf + len - b);
-		b = buf + len;
+	while (fgets(buf, sizeof(buf), stdin)) {
+		len = strlen(buf);
+		if (write(fd, buf, len) != len) {
+			fprintf(stderr, "Failed to update backlight interface '%s'\n", argv[1]);
+			return 2;
+		}
 	}
 
 	return 0;
-- 
1.8.5.3

Re: [PATCH 4/4] backlight-helper: Simplify reading the level from stdin

[Chris Wilson]

On Mon, Feb 17, 2014 at 01:16:54PM +0100, Hans de Goede wrote:
> Since the helper is a standalone app, the usual xserver rules of not using
> stdio because of signal handling don't apply.
> 
> And since the helper does run with elevated rights, it is important to keep
> the code KISS so that it can be audited easily.
> 
> This commit replaces the hard to read "raw" read loop with a much simpler
> loop using fgets, improving readability of the code.

Ok, the simplicity of the final code won me over.

Thanks for the patches, pushed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre