Re: [PATCH] Fix curses running as root on tty of other user

This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
--===============0345442638==
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature";
	boundary="------------enigBB0DE96FF94B01DA5A834381"

This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
--------------enigBB0DE96FF94B01DA5A834381
Content-Type: multipart/mixed; boundary="------------070605010502010802000403"

This is a multi-part message in MIME format.
--------------070605010502010802000403
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

On 02/15/2011 03:12 PM, Stanislav Ochotnicky wrote:
> After doing "su -", ownership of current tty stays with original
> user. If we drop all capabilities we will not be able to open current
> tty to setup curses screen.
>=20
> So we keep ipc_lock together with dac_override capabilities until we
> open tty for R/W, then we drop dac_override capability.
>=20
> This patch also fixes second cap_set_proc call in lock_pool that was
> originally "cap_ipc_lock+p", but should probably be
> "cap_ipc_lock-e". Original call had no effect because ipc_lock
> capability was already permitted. Instead it was supposed to drop
> effective capability enabled in first call.

One more thing to note here is that I don't really see why we should
keep cap_ipc_lock after calling mlock at all. It seems unnecessary since
memory is already locked, but I am probably missing some nuance of
capabilities and/or mlock-ing.

Attached patch is my attempt number two. Original had:
+  if (init)
+    cap_set_proc( cap_from_text("cap_dac_override+ep") );
+  else
+    cap_set_proc ( cap_from_text("cap_ipc_lock=3Dp") );

This would drop the cap_ipc_lock from permitted and so we wouldn't be
able to set it when restoring capability.

Now it's:
+  if (init)
+    cap_set_proc( cap_from_text("cap_dac_override+ep cap_ipc_lock=3Dp") =
);
+  else
+    cap_set_proc ( cap_from_text("cap_ipc_lock=3Dp") );

However like I said in previous paragraph, even the original version
shouldn't cause any problems since no code path uses ipc_lock capability
after setting up secure memory AFAIK (well before tty initialisation).


> See https://bugzilla.redhat.com/show_bug.cgi?id=3D676034 for details an=
d
> reproducer

If the patch needs work, let me know and I'll fix it up.


--=20
Stanislav Ochotnicky <[email protected]>
Associate Software Engineer - Base Operating Systems Brno

PGP: 7B087241
Red Hat Inc.                               http://cz.redhat.com

--------------070605010502010802000403
Content-Type: text/plain;
	name="0001-Fix-curses-running-as-root-on-tty-of-other-user.patch"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
	filename*0="0001-Fix-curses-running-as-root-on-tty-of-other-user.patch"

=46rom fa16163e7981890fd8e57f4ca0f6b9acc96dc727 Mon Sep 17 00:00:00 2001
From: Stanislav Ochotnicky <[email protected]>
Date: Tue, 15 Feb 2011 12:43:40 +0100
Subject: [PATCH] Fix curses running as root on tty of other user

After doing "su -", ownership of current tty stays with original
user. If we drop all capabilities we will not be able to open current
tty to setup curses screen.

So we keep ipc_lock together with dac_override capabilities until we
open tty for R/W, then we drop dac_override capability.

This patch also fixes second cap_set_proc call in lock_pool that was
originally "cap_ipc_lock+p", but should probably be
"cap_ipc_lock-e". Original call had no effect because ipc_lock
capability was already permitted. Instead it was supposed to drop
effective capability enabled in first call.

See https://bugzilla.redhat.com/show_bug.cgi?id=3D676034 for details and
reproducer
---
 pinentry/pinentry-curses.c |   25 ++++++++++++++++++++++++-
 secmem/secmem.c            |    6 ++++--
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/pinentry/pinentry-curses.c b/pinentry/pinentry-curses.c
index 76ddbdd..75653a7 100644
--- a/pinentry/pinentry-curses.c
+++ b/pinentry/pinentry-curses.c
@@ -93,6 +93,21 @@ struct dialog
 };
 typedef struct dialog *dialog_t;
=20
+/* When pinentry-curses runs as root, current tty can be owned by
+  original user (before doing "su -"). We need to preserve
+  dac_override capability to be able to read/write on tty */
+static int tty_cap_setup(int init)
+{
+#if defined(USE_CAPABILITIES)
+  if (init)
+    cap_set_proc( cap_from_text("cap_dac_override+ep cap_ipc_lock=3Dp") =
);
+  else
+    cap_set_proc ( cap_from_text("cap_ipc_lock=3Dp") );
+#endif
+  return 0;
+}
+
+
 =0C
 /* Return the next line up to MAXLEN columns wide in START and LEN.
    The first invocation should have 0 as *LEN.  If the line ends with
@@ -635,17 +650,25 @@ dialog_run (pinentry_t pinentry, const char *tty_na=
me, const char *tty_type)
   /* Open the desired terminal if necessary.  */
   if (tty_name)
     {
+      tty_cap_setup(1);
       ttyfi =3D fopen (tty_name, "r");
       if (!ttyfi)
-	return -1;
+	{
+	  int err =3D errno;
+	  tty_cap_setup(0);
+	  errno =3D err;
+	  return -1;
+	}
       ttyfo =3D fopen (tty_name, "w");
       if (!ttyfo)
 	{
 	  int err =3D errno;
 	  fclose (ttyfi);
+	  tty_cap_setup(0);
 	  errno =3D err;
 	  return -1;
 	}
+      tty_cap_setup(0);
       screen =3D newterm (tty_type, ttyfo, ttyfi);
       set_term (screen);
     }
diff --git a/secmem/secmem.c b/secmem/secmem.c
index fed7e97..b31e270 100644
--- a/secmem/secmem.c
+++ b/secmem/secmem.c
@@ -130,11 +130,13 @@ lock_pool( void *p, size_t n )
 #if defined(USE_CAPABILITIES) && defined(HAVE_MLOCK)
     int err;
=20
-    cap_set_proc( cap_from_text("cap_ipc_lock+ep") );
+    /* we have to preserve dac_override to be able to open current tty
+     * even if it is owned by other user (usually after doing su) */
+    cap_set_proc( cap_from_text("cap_ipc_lock+ep cap_dac_override+p") );=

     err =3D mlock( p, n );
     if( err && errno )
 	err =3D errno;
-    cap_set_proc( cap_from_text("cap_ipc_lock+p") );
+    cap_set_proc( cap_from_text("cap_ipc_lock-e cap_dac_override+p") );
=20
     if( err ) {
 	if( errno !=3D EPERM
--=20
1.7.4


--------------070605010502010802000403--

--------------enigBB0DE96FF94B01DA5A834381
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBCgAGBQJNW+kjAAoJELy9dS57CHJB8MUP/RrcXOCNqr0v3Rv1kKbXKMkE
PeCknOUAomOz1oma7v4hQM2o8io8lzelBAhIQoy/Skr7rmWZUoxxiTks4IVrIRff
Hhk/BhMuScgUH8DxwxdTWwMl8xqSLq8VLQM0V2dkCVoOo8WZvyXccxV6WwYhuL3b
40rWxmgleZkjUIgceS5NBc0qZUPLFXv8cSy1khQVDefk8bG2z9wA37m1F20LHc3I
nUxwCfTPSYLCTkbYnmWi2xCa+fxaXckVdKb2NwEvdN8WHnhO+GO4+fuKSJnEw5jO
6VPyel6wGPjWvzzxj71Or4OJBRGxM/KZnm2WJLOx/d7mrgI6NYSZbImRXY6YRLbu
s1vZTtcbqy9BVdbSv1JbKu1EzXYxVoE5eT3/dkLNtvbgIXrtuVd9wLEpkllqKHzG
fqdj6q5cB0bZMJgqpLmrpRqz9+JxZOwZ9o9GK7YsJbvSqOPl5yqXkjwoYVX+ZYKZ
P8/vKseB3TZzIHORES8kybynuinGNnNnWJkJvUIicbk+fHbvmCwLN/QnT6WevDCH
osZOWerUgGfFd+x2CSuwOYZIqX1hpI3mVmRkcwAMOt4Yz2DvirNSIjqa6LPm8E70
eYF5HKFKLWJYiNEg4SsVV8t8Q3zA6aBnonDqQL8CWQ6unqxuIeMvIcdiqkxqWFdQ
dK9k4FufTUaE15h0+AX0
=R/Uh
-----END PGP SIGNATURE-----

--------------enigBB0DE96FF94B01DA5A834381--


--===============0345442638==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Gpa-dev mailing list
[email protected]
http://lists.gnupg.org/mailman/listinfo/gpa-dev

--===============0345442638==--