Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: GnuPG: gpa

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

 

 

GnuPG gpa RSS feed   Index | Next | Previous | View Threaded


sochotnicky at redhat

Feb 15, 2011, 6:12 AM

Post #1 of 4 (1949 views)
Permalink
[PATCH] Fix curses running as root on tty of other user

I have recently received bug report where running pinentry as root
with tty set was failing. After some strac-ing, I found the culprit in
dialog_run function inside pinentry-curses.c. It tries to open current
tty if it is set, but it fails because pinentry removes all
capabilities except ipc_lock.

I created a patch fixing this behaviour by keeping dac_override
capability until after we open ttys.

I also fixed another one small capability issue that I believe was
present. See the patch for details on this.

To reproduce do this:
1. login as normal user
2. su -
3. ls -l `tty` should show you original user as owner
4. gpg2 --symmetric .bashrc

With this patch last command succeeds, otherwise it fails

diffstat:
pinentry/pinentry-curses.c | 24 +++++++++++++++++++++++-
secmem/secmem.c | 6 ++++--
2 files changed, 27 insertions(+), 3 deletions(-)

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


sochotnicky at redhat

Feb 15, 2011, 6:12 AM

Post #2 of 4 (1869 views)
Permalink
[PATCH] Fix curses running as root on tty of other user [In reply to]

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=676034 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..bfc5013 100644
--- a/pinentry/pinentry-curses.c
+++ b/pinentry/pinentry-curses.c
@@ -93,6 +93,21 @@ struct dialog
};
typedef struct dialog *dialog_t;

+/* 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") );
+ else
+ cap_set_proc ( cap_from_text("cap_ipc_lock=p") );
+#endif
+ return 0;
+}
+
+

/* 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_name, const char *tty_type)
/* Open the desired terminal if necessary. */
if (tty_name)
{
+ tty_cap_setup(1);
ttyfi = fopen (tty_name, "r");
if (!ttyfi)
- return -1;
+ {
+ int err = errno;
+ tty_cap_setup(0);
+ errno = err;
+ return -1;
+ }
ttyfo = fopen (tty_name, "w");
if (!ttyfo)
{
int err = errno;
fclose (ttyfi);
+ tty_cap_setup(0);
errno = err;
return -1;
}
+ tty_cap_setup(0);
screen = 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;

- 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 = mlock( p, n );
if( err && errno )
err = 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") );

if( err ) {
if( errno != EPERM
--
1.7.4


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


sochotnicky at redhat

Feb 16, 2011, 7:11 AM

Post #3 of 4 (1868 views)
Permalink
Re: [PATCH] Fix curses running as root on tty of other user [In reply to]

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.
>
> 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.

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=p") );

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=p") );
+ else
+ cap_set_proc ( cap_from_text("cap_ipc_lock=p") );

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=676034 for details and
> reproducer

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


--
Stanislav Ochotnicky <sochotnicky [at] redhat>
Associate Software Engineer - Base Operating Systems Brno

PGP: 7B087241
Red Hat Inc. http://cz.redhat.com
Attachments: 0001-Fix-curses-running-as-root-on-tty-of-other-user.patch (3.17 KB)
  signature.asc (0.82 KB)


wk at gnupg

Feb 17, 2011, 9:38 AM

Post #4 of 4 (1856 views)
Permalink
Re: [PATCH] Fix curses running as root on tty of other user [In reply to]

On Tue, 15 Feb 2011 15:12, sochotnicky [at] redhat said:
> I have recently received bug report where running pinentry as root
> with tty set was failing. After some strac-ing, I found the culprit in
> dialog_run function inside pinentry-curses.c. It tries to open current
> tty if it is set, but it fails because pinentry removes all
> capabilities except ipc_lock.

Capabilities? Hmmm, I just checked and figured that there is use
enabled by default. The code is more than 10 years old and I was not
aware that it still works. For about the same time there is no need for
capabilities because at least Linux allows to mlock a few packages
without running under uid 0 (or with the respective capability).

The fix is to configure --without-libcap to disable the whole code and
best remove the code completely and update the secmem code to the one
used today by Libgcrypt or gpg 1.4.


Shalom-Salam,

Werner

--
Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.


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

GnuPG gpa RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.