On Thu, Jan 28, 2010 at 09:34:35PM +0000, Roger Leigh wrote: > OK, I have a fix! (attached) [...] > This has been committed into git, but I'll need to ponder the > implications for a little bit before I upload it.
After further review: the patch as presented (really attached this time) introduces a security hole that while user->otheruser switching is forbidden, user->user authentication will always succeed. While there's no hole *at present*, the altered semantics of auth_null could result in a future bug if we make the assumption of failure. Solution: 1) Keep auth_null as the default session authentication method. 2) Add auth_pam to *all* of schroot/dchroot/dchroot-dsa by adding add_session_auth to schroot::main_base (common to dchroot, dchroot-dsa and schroot). Make virtual to allow addition of user-switching for schroot. Mostly done, but won't be finished tonight. Since this has potential for security problems, I'll need to carefully review it. Regards, Roger -- .''`. Roger Leigh : :' : Debian GNU/Linux http://people.debian.org/~rleigh/ `. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/ `- GPG Public Key: 0x25BFB848 Please GPG sign your mail.
From 615b8ebc2a32f4619bb08618cf0bb458e656b1bd Mon Sep 17 00:00:00 2001 From: Roger Leigh <[email protected]> Date: Thu, 28 Jan 2010 21:06:30 +0000 Subject: [PATCH 1/4] sbuild::auth_null: Allow authentication skip if local and remote match If the local and remote uids are the same, don't throw and error. This allows root to run in a chroot where not specifically authorised by the configuration. --- sbuild/sbuild-auth-null.cc | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/sbuild/sbuild-auth-null.cc b/sbuild/sbuild-auth-null.cc index ca03340..1936151 100644 --- a/sbuild/sbuild-auth-null.cc +++ b/sbuild/sbuild-auth-null.cc @@ -100,7 +100,10 @@ auth_null::authenticate (status auth_status) break; case STATUS_USER: - throw error(AUTHENTICATION, strerror(ENOTSUP)); + // We don't support user authentication, so throw an error if + // the local and remote users are different. + if (this->ruid != this->uid) + throw error(AUTHENTICATION, strerror(ENOTSUP)); break; case STATUS_FAIL: -- 1.6.6
From d059b78cdee7b1b01bb41db88275a2ec7d7bdbc2 Mon Sep 17 00:00:00 2001 From: Roger Leigh <[email protected]> Date: Thu, 28 Jan 2010 22:25:47 +0000 Subject: [PATCH 2/4] sbuild::auth_null: Only root can authenticate Throw an error if the user is not root. i.e. Only root->root is permitted. All other authentication attempts fail. Note that this could be used to escape pam_rootok restrictions on systems with PAM also available and pam_rootok not enabled. --- sbuild/sbuild-auth-null.cc | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/sbuild/sbuild-auth-null.cc b/sbuild/sbuild-auth-null.cc index 1936151..31cdc04 100644 --- a/sbuild/sbuild-auth-null.cc +++ b/sbuild/sbuild-auth-null.cc @@ -101,8 +101,11 @@ auth_null::authenticate (status auth_status) case STATUS_USER: // We don't support user authentication, so throw an error if - // the local and remote users are different. - if (this->ruid != this->uid) + // the user is not root. i.e. Only root->root is permitted. + // All other authentication attempts fail. Note that this could + // be used to escape pam_rootok restrictions on systems with PAM + // also available and pam_rootok not enabled. + if (this->ruid != 0) throw error(AUTHENTICATION, strerror(ENOTSUP)); break; -- 1.6.6
From 97317788e86ee0fa8e16f526b44de87136883d62 Mon Sep 17 00:00:00 2001 From: Roger Leigh <[email protected]> Date: Thu, 28 Jan 2010 22:27:31 +0000 Subject: [PATCH 3/4] sbuild::auth_pam: Remove unneeded header --- sbuild/sbuild-auth-pam.cc | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/sbuild/sbuild-auth-pam.cc b/sbuild/sbuild-auth-pam.cc index 3dbefd7..ba489a7 100644 --- a/sbuild/sbuild-auth-pam.cc +++ b/sbuild/sbuild-auth-pam.cc @@ -20,7 +20,6 @@ #include "sbuild-auth-pam.h" #include "sbuild-auth-pam-conv.h" -#include "sbuild-auth-pam-conv-tty.h" #include <cassert> #include <cerrno> -- 1.6.6
From 2834df966ef6e1a88e6f9b49e423ff08ea39f7fc Mon Sep 17 00:00:00 2001 From: Roger Leigh <[email protected]> Date: Thu, 28 Jan 2010 22:28:30 +0000 Subject: [PATCH 4/4] schroot: dchroot and dchroot-dsa also use PAM Add PAM authentication handler installation into common setup code. schroot overrides this to also configure user switching. --- bin/schroot/schroot-main-base.cc | 26 ++++++++++++++++++++++++++ bin/schroot/schroot-main-base.h | 6 ++++++ bin/schroot/schroot-main.cc | 25 +++++-------------------- bin/schroot/schroot-main.h | 3 +++ 4 files changed, 40 insertions(+), 20 deletions(-) diff --git a/bin/schroot/schroot-main-base.cc b/bin/schroot/schroot-main-base.cc index 3c6c883..2d8894e 100644 --- a/bin/schroot/schroot-main-base.cc +++ b/bin/schroot/schroot-main-base.cc @@ -22,6 +22,7 @@ #include <sbuild/sbuild-config.h> #ifdef SBUILD_FEATURE_PAM +#include <sbuild/sbuild-auth-pam.h> #include <sbuild/sbuild-auth-pam-conv.h> #include <sbuild/sbuild-auth-pam-conv-tty.h> #endif // SBUILD_FEATURE_PAM @@ -291,6 +292,7 @@ main_base::run_impl () try { create_session(sess_op); + add_session_auth(); if (!this->options->command.empty()) this->session->get_auth()->set_command(this->options->command); @@ -321,3 +323,27 @@ main_base::run_impl () else return EXIT_FAILURE; } + +void +main_base::add_session_auth () +{ + // Add PAM authentication handler. If PAM isn't available, just + // continue to use the default handler + +#ifdef SBUILD_FEATURE_PAM + sbuild::auth::ptr auth = sbuild::auth_pam::create("schroot"); + + sbuild::auth_pam_conv_tty::auth_ptr auth_ptr = + std::tr1::dynamic_pointer_cast<sbuild::auth_pam>(auth); + + sbuild::auth_pam_conv::ptr conv = sbuild::auth_pam_conv_tty::create(auth_ptr); + + /* Set up authentication timeouts. */ + time_t curtime = 0; + time(&curtime); + conv->set_warning_timeout(curtime + 15); + conv->set_fatal_timeout(curtime + 20); + + this->session->set_auth(auth); +#endif // SBUILD_FEATURE_PAM +} diff --git a/bin/schroot/schroot-main-base.h b/bin/schroot/schroot-main-base.h index cb21e31..25b62e5 100644 --- a/bin/schroot/schroot-main-base.h +++ b/bin/schroot/schroot-main-base.h @@ -132,6 +132,12 @@ namespace schroot virtual void create_session (sbuild::session::operation sess_op) = 0; + /** + * Add PAM authentication handler to the session. + */ + virtual void + add_session_auth (); + protected: /// The program options. options_base::ptr options; diff --git a/bin/schroot/schroot-main.cc b/bin/schroot/schroot-main.cc index 6565ca7..2f083d5 100644 --- a/bin/schroot/schroot-main.cc +++ b/bin/schroot/schroot-main.cc @@ -19,10 +19,6 @@ #include <config.h> #include <sbuild/sbuild-config.h> -#ifdef SBUILD_FEATURE_PAM -#include <sbuild/sbuild-auth-pam.h> -#include <sbuild/sbuild-auth-pam-conv-tty.h> -#endif #include "schroot-main.h" @@ -82,23 +78,12 @@ main::create_session(sbuild::session::operation sess_op) this->session = sbuild::session::ptr (new sbuild::session("schroot", this->config, sess_op, this->chroots)); +} -#ifdef SBUILD_FEATURE_PAM - sbuild::auth::ptr auth = sbuild::auth_pam::create("schroot"); - - sbuild::auth_pam_conv_tty::auth_ptr auth_ptr = - std::tr1::dynamic_pointer_cast<sbuild::auth_pam>(auth); - - sbuild::auth_pam_conv::ptr conv = sbuild::auth_pam_conv_tty::create(auth_ptr); - - /* Set up authentication timeouts. */ - time_t curtime = 0; - time(&curtime); - conv->set_warning_timeout(curtime + 15); - conv->set_fatal_timeout(curtime + 20); - - this->session->set_auth(auth); -#endif // SBUILD_FEATURE_PAM +void +main::add_session_auth () +{ + main_base::add_session_auth(); if (!this->options->user.empty()) this->session->get_auth()->set_user(this->options->user); diff --git a/bin/schroot/schroot-main.h b/bin/schroot/schroot-main.h index dcfe2f4..5050c6f 100644 --- a/bin/schroot/schroot-main.h +++ b/bin/schroot/schroot-main.h @@ -56,6 +56,9 @@ namespace schroot protected: virtual void create_session(sbuild::session::operation sess_op); + + void + add_session_auth (); }; } -- 1.6.6
signature.asc
Description: Digital signature
