| Message ID | 155570011247.27135.12509150054846153288.stgit@chester (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | proc: prevent changes to overridden credentials | expand |
On Fri, Apr 19, 2019 at 2:55 PM Paul Moore <paul@paul-moore.com> wrote: > Prevent userspace from changing the the /proc/PID/attr values if the > task's credentials are currently overriden. This not only makes sense > conceptually, it also prevents some really bizarre error cases caused > when trying to commit credentials to a task with overridden > credentials. > > Cc: <stable@vger.kernel.org> > Reported-by: "chengjian (D)" <cj.chengjian@huawei.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > fs/proc/base.c | 5 +++++ > 1 file changed, 5 insertions(+) I sent this to the LSM list as I figure it should probably go via James' linux-security tree since it is cross-LSM and doesn't really contain any LSM specific code. That said, if you don't want this James let me know and I'll send it via the SELinux tree assuming I can get ACKs from John and Casey (this should only affect SELinux, AppArmor, and Smack). > diff --git a/fs/proc/base.c b/fs/proc/base.c > index ddef482f1334..87ba007b86db 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2539,6 +2539,11 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, > rcu_read_unlock(); > return -EACCES; > } > + /* Prevent changes to overridden credentials. */ > + if (current_cred() != current_real_cred()) { > + rcu_read_unlock(); > + return -EBUSY; > + } > rcu_read_unlock(); > > if (count > PAGE_SIZE) >
On 4/19/19 11:55 AM, Paul Moore wrote: > Prevent userspace from changing the the /proc/PID/attr values if the > task's credentials are currently overriden. This not only makes sense > conceptually, it also prevents some really bizarre error cases caused > when trying to commit credentials to a task with overridden > credentials. > > Cc: <stable@vger.kernel.org> > Reported-by: "chengjian (D)" <cj.chengjian@huawei.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> looks good Acked-by: John Johansen <john.johansen@canonical.com> > --- > fs/proc/base.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index ddef482f1334..87ba007b86db 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2539,6 +2539,11 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, > rcu_read_unlock(); > return -EACCES; > } > + /* Prevent changes to overridden credentials. */ > + if (current_cred() != current_real_cred()) { > + rcu_read_unlock(); > + return -EBUSY; > + } > rcu_read_unlock(); > > if (count > PAGE_SIZE) >
On Fri, 19 Apr 2019, Paul Moore wrote: > On Fri, Apr 19, 2019 at 2:55 PM Paul Moore <paul@paul-moore.com> wrote: > > Prevent userspace from changing the the /proc/PID/attr values if the > > task's credentials are currently overriden. This not only makes sense > > conceptually, it also prevents some really bizarre error cases caused > > when trying to commit credentials to a task with overridden > > credentials. > > > > Cc: <stable@vger.kernel.org> > > Reported-by: "chengjian (D)" <cj.chengjian@huawei.com> > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > fs/proc/base.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > I sent this to the LSM list as I figure it should probably go via > James' linux-security tree since it is cross-LSM and doesn't really > contain any LSM specific code. That said, if you don't want this > James let me know and I'll send it via the SELinux tree assuming I can > get ACKs from John and Casey (this should only affect SELinux, > AppArmor, and Smack). This is fine to go via your tree. Acked-by: James Morris <james.morris@microsoft.com>
On 4/19/2019 11:55 AM, Paul Moore wrote: > Prevent userspace from changing the the /proc/PID/attr values if the > task's credentials are currently overriden. This not only makes sense > conceptually, it also prevents some really bizarre error cases caused > when trying to commit credentials to a task with overridden > credentials. > > Cc: <stable@vger.kernel.org> > Reported-by: "chengjian (D)" <cj.chengjian@huawei.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> Acked-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/proc/base.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index ddef482f1334..87ba007b86db 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2539,6 +2539,11 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, > rcu_read_unlock(); > return -EACCES; > } > + /* Prevent changes to overridden credentials. */ > + if (current_cred() != current_real_cred()) { > + rcu_read_unlock(); > + return -EBUSY; > + } > rcu_read_unlock(); > > if (count > PAGE_SIZE) >
On Fri, Apr 19, 2019 at 4:27 PM James Morris <jmorris@namei.org> wrote: > On Fri, 19 Apr 2019, Paul Moore wrote: > > On Fri, Apr 19, 2019 at 2:55 PM Paul Moore <paul@paul-moore.com> wrote: > > > Prevent userspace from changing the the /proc/PID/attr values if the > > > task's credentials are currently overriden. This not only makes sense > > > conceptually, it also prevents some really bizarre error cases caused > > > when trying to commit credentials to a task with overridden > > > credentials. > > > > > > Cc: <stable@vger.kernel.org> > > > Reported-by: "chengjian (D)" <cj.chengjian@huawei.com> > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > --- > > > fs/proc/base.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > I sent this to the LSM list as I figure it should probably go via > > James' linux-security tree since it is cross-LSM and doesn't really > > contain any LSM specific code. That said, if you don't want this > > James let me know and I'll send it via the SELinux tree assuming I can > > get ACKs from John and Casey (this should only affect SELinux, > > AppArmor, and Smack). > > This is fine to go via your tree. Okay. I just merged this into selinux/next. I was sitting on this patch to see how the other thread developed, but that doesn't really seem to be reaching any conclusion and I really want this to get at least one week in -next before the merge window opens. Thanks everyone.
diff --git a/fs/proc/base.c b/fs/proc/base.c index ddef482f1334..87ba007b86db 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2539,6 +2539,11 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, rcu_read_unlock(); return -EACCES; } + /* Prevent changes to overridden credentials. */ + if (current_cred() != current_real_cred()) { + rcu_read_unlock(); + return -EBUSY; + } rcu_read_unlock(); if (count > PAGE_SIZE)
Prevent userspace from changing the the /proc/PID/attr values if the task's credentials are currently overriden. This not only makes sense conceptually, it also prevents some really bizarre error cases caused when trying to commit credentials to a task with overridden credentials. Cc: <stable@vger.kernel.org> Reported-by: "chengjian (D)" <cj.chengjian@huawei.com> Signed-off-by: Paul Moore <paul@paul-moore.com> --- fs/proc/base.c | 5 +++++ 1 file changed, 5 insertions(+)