Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
> | A trusted path is one that is inside is a root owned directory that > | is not group or world writable. /bin, /usr/bin, /usr/local/bin, are > | (under normal circumstances) considered trusted. Any non-root > | users home directory is not trusted, nor is /tmp. You need the entire path to be root owned and root write only from /, and to assume that the attacker can't get CAP_SYS_DAC. Compare that with labelling and labelling looks rather better. It's the same story as ever - paths are not very meaningful for security, content is. You label content not paths. The existing LSMs can provide both content label based execution protection and even path based. SELinux can do it right already, AppArmor appears to be able to do it compatibly wrong with your proposal already. > NOTE: root is never restricted by TPE Why are we talking about "root" not capabilities ? We stopped tying stuff to "root" 20 years ago. >* Can be bypassed by interpreted languages such as python. You can run > malicious code by doing: python -c 'evil code' Or using ld.so since ELF binaries are dynamically loaded and will even helpfully trying and dynamically load libraries not on your trust path completely configurably via environment variables. So all your funky protection goes to poop the moment you hit the most wannabe n00b attacker who knows how to set LD_PRELOAD. The unix x bit was never designed as a security feature. It got slightly tweaked for one for directory path walking but the purpose of the 'x' bit on a file is solely to stop the user accidentally executing garbage. > 2. Attacker on system replaces binary used by a privileged user with a >malicious one > > * This situation arises when administrator of a system leaves a binary >as world writable. > > * TPE is very effective against this threat model Keeping the executables root is allowed to use in a root owned, only root reachable space with no setuid bits works even better. Especially as you can mount it r/o almost all of the time. Pleae explain how your filesystem mode checks interact with existing file systems that implement other security semantics eg OpenAFS, or for that matter DOS ? [Not an idle question as most distributions keep their EFI system partition mounted even though it seems a rather silly thing to do] Alan
Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
> | A trusted path is one that is inside is a root owned directory that > | is not group or world writable. /bin, /usr/bin, /usr/local/bin, are > | (under normal circumstances) considered trusted. Any non-root > | users home directory is not trusted, nor is /tmp. You need the entire path to be root owned and root write only from /, and to assume that the attacker can't get CAP_SYS_DAC. Compare that with labelling and labelling looks rather better. It's the same story as ever - paths are not very meaningful for security, content is. You label content not paths. The existing LSMs can provide both content label based execution protection and even path based. SELinux can do it right already, AppArmor appears to be able to do it compatibly wrong with your proposal already. > NOTE: root is never restricted by TPE Why are we talking about "root" not capabilities ? We stopped tying stuff to "root" 20 years ago. >* Can be bypassed by interpreted languages such as python. You can run > malicious code by doing: python -c 'evil code' Or using ld.so since ELF binaries are dynamically loaded and will even helpfully trying and dynamically load libraries not on your trust path completely configurably via environment variables. So all your funky protection goes to poop the moment you hit the most wannabe n00b attacker who knows how to set LD_PRELOAD. The unix x bit was never designed as a security feature. It got slightly tweaked for one for directory path walking but the purpose of the 'x' bit on a file is solely to stop the user accidentally executing garbage. > 2. Attacker on system replaces binary used by a privileged user with a >malicious one > > * This situation arises when administrator of a system leaves a binary >as world writable. > > * TPE is very effective against this threat model Keeping the executables root is allowed to use in a root owned, only root reachable space with no setuid bits works even better. Especially as you can mount it r/o almost all of the time. Pleae explain how your filesystem mode checks interact with existing file systems that implement other security semantics eg OpenAFS, or for that matter DOS ? [Not an idle question as most distributions keep their EFI system partition mounted even though it seems a rather silly thing to do] Alan
Re: [kernel-hardening] Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On 06/04/2017 01:47 AM, Eric Biggers wrote:
On Sun, Jun 04, 2017 at 01:24:13AM -0400, Matt Brown wrote:
On 06/03/2017 02:33 AM, Al Viro wrote:
On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
+static int tpe_bprm_set_creds(struct linux_binprm *bprm)
+{
+ struct file *file = bprm->file;
+ struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
+ struct inode *file_inode = d_backing_inode(file->f_path.dentry);
Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?
Good catch. How does this look:
spin_lock(>i_lock);
spin_lock(_inode->i_lock);
if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
reason1 = "directory not owned by user";
else if (inode->i_mode & 0002)
reason1 = "file in world-writable directory";
else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
reason1 = "file in group-writable directory";
else if (file_inode->i_mode & 0002)
reason1 = "file is world-writable";
spin_unlock(>i_lock);
spin_unlock(_inode->i_lock);
and likewise for other places in the code?
No, it needs to take a reference on the parent dentry before using it, using
dget_parent(), I think, and then dropping it later with dput(). Taking i_lock
isn't needed.
Eric
Got it. Thank you!
Matt Brown
Re: [kernel-hardening] Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On 06/04/2017 01:47 AM, Eric Biggers wrote:
On Sun, Jun 04, 2017 at 01:24:13AM -0400, Matt Brown wrote:
On 06/03/2017 02:33 AM, Al Viro wrote:
On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
+static int tpe_bprm_set_creds(struct linux_binprm *bprm)
+{
+ struct file *file = bprm->file;
+ struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
+ struct inode *file_inode = d_backing_inode(file->f_path.dentry);
Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?
Good catch. How does this look:
spin_lock(>i_lock);
spin_lock(_inode->i_lock);
if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
reason1 = "directory not owned by user";
else if (inode->i_mode & 0002)
reason1 = "file in world-writable directory";
else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
reason1 = "file in group-writable directory";
else if (file_inode->i_mode & 0002)
reason1 = "file is world-writable";
spin_unlock(>i_lock);
spin_unlock(_inode->i_lock);
and likewise for other places in the code?
No, it needs to take a reference on the parent dentry before using it, using
dget_parent(), I think, and then dropping it later with dput(). Taking i_lock
isn't needed.
Eric
Got it. Thank you!
Matt Brown
Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On Sun, Jun 04, 2017 at 01:24:13AM -0400, Matt Brown wrote:
> On 06/03/2017 02:33 AM, Al Viro wrote:
> > On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
> >
> > > +static int tpe_bprm_set_creds(struct linux_binprm *bprm)
> > > +{
> > > + struct file *file = bprm->file;
> > > + struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
> > > + struct inode *file_inode = d_backing_inode(file->f_path.dentry);
> >
> > Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
> > somehow making sure that your 'inode' won't get freed right under you?
> >
>
> Good catch. How does this look:
>
> spin_lock(>i_lock);
> spin_lock(_inode->i_lock);
> if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
> reason1 = "directory not owned by user";
> else if (inode->i_mode & 0002)
> reason1 = "file in world-writable directory";
> else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
> reason1 = "file in group-writable directory";
> else if (file_inode->i_mode & 0002)
> reason1 = "file is world-writable";
> spin_unlock(>i_lock);
> spin_unlock(_inode->i_lock);
>
> and likewise for other places in the code?
Er... You have a pointer to object that might get freed by a thread
running on another CPU. So you attempt to take a spinlock sitting
inside that object. How exactly is that supposed to help anything?
Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On Sun, Jun 04, 2017 at 01:24:13AM -0400, Matt Brown wrote:
> On 06/03/2017 02:33 AM, Al Viro wrote:
> > On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
> >
> > > +static int tpe_bprm_set_creds(struct linux_binprm *bprm)
> > > +{
> > > + struct file *file = bprm->file;
> > > + struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
> > > + struct inode *file_inode = d_backing_inode(file->f_path.dentry);
> >
> > Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
> > somehow making sure that your 'inode' won't get freed right under you?
> >
>
> Good catch. How does this look:
>
> spin_lock(>i_lock);
> spin_lock(_inode->i_lock);
> if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
> reason1 = "directory not owned by user";
> else if (inode->i_mode & 0002)
> reason1 = "file in world-writable directory";
> else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
> reason1 = "file in group-writable directory";
> else if (file_inode->i_mode & 0002)
> reason1 = "file is world-writable";
> spin_unlock(>i_lock);
> spin_unlock(_inode->i_lock);
>
> and likewise for other places in the code?
Er... You have a pointer to object that might get freed by a thread
running on another CPU. So you attempt to take a spinlock sitting
inside that object. How exactly is that supposed to help anything?
Re: [kernel-hardening] Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On Sun, Jun 04, 2017 at 01:24:13AM -0400, Matt Brown wrote:
> On 06/03/2017 02:33 AM, Al Viro wrote:
> > On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
> >
> > > +static int tpe_bprm_set_creds(struct linux_binprm *bprm)
> > > +{
> > > + struct file *file = bprm->file;
> > > + struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
> > > + struct inode *file_inode = d_backing_inode(file->f_path.dentry);
> >
> > Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
> > somehow making sure that your 'inode' won't get freed right under you?
> >
>
> Good catch. How does this look:
>
> spin_lock(>i_lock);
> spin_lock(_inode->i_lock);
> if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
> reason1 = "directory not owned by user";
> else if (inode->i_mode & 0002)
> reason1 = "file in world-writable directory";
> else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
> reason1 = "file in group-writable directory";
> else if (file_inode->i_mode & 0002)
> reason1 = "file is world-writable";
> spin_unlock(>i_lock);
> spin_unlock(_inode->i_lock);
>
> and likewise for other places in the code?
No, it needs to take a reference on the parent dentry before using it, using
dget_parent(), I think, and then dropping it later with dput(). Taking i_lock
isn't needed.
Eric
Re: [kernel-hardening] Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On Sun, Jun 04, 2017 at 01:24:13AM -0400, Matt Brown wrote:
> On 06/03/2017 02:33 AM, Al Viro wrote:
> > On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
> >
> > > +static int tpe_bprm_set_creds(struct linux_binprm *bprm)
> > > +{
> > > + struct file *file = bprm->file;
> > > + struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
> > > + struct inode *file_inode = d_backing_inode(file->f_path.dentry);
> >
> > Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
> > somehow making sure that your 'inode' won't get freed right under you?
> >
>
> Good catch. How does this look:
>
> spin_lock(>i_lock);
> spin_lock(_inode->i_lock);
> if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
> reason1 = "directory not owned by user";
> else if (inode->i_mode & 0002)
> reason1 = "file in world-writable directory";
> else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
> reason1 = "file in group-writable directory";
> else if (file_inode->i_mode & 0002)
> reason1 = "file is world-writable";
> spin_unlock(>i_lock);
> spin_unlock(_inode->i_lock);
>
> and likewise for other places in the code?
No, it needs to take a reference on the parent dentry before using it, using
dget_parent(), I think, and then dropping it later with dput(). Taking i_lock
isn't needed.
Eric
Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On 06/03/2017 02:33 AM, Al Viro wrote:
On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
+static int tpe_bprm_set_creds(struct linux_binprm *bprm)
+{
+ struct file *file = bprm->file;
+ struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
+ struct inode *file_inode = d_backing_inode(file->f_path.dentry);
Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?
Good catch. How does this look:
spin_lock(>i_lock);
spin_lock(_inode->i_lock);
if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
reason1 = "directory not owned by user";
else if (inode->i_mode & 0002)
reason1 = "file in world-writable directory";
else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
reason1 = "file in group-writable directory";
else if (file_inode->i_mode & 0002)
reason1 = "file is world-writable";
spin_unlock(>i_lock);
spin_unlock(_inode->i_lock);
and likewise for other places in the code?
Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On 06/03/2017 02:33 AM, Al Viro wrote:
On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
+static int tpe_bprm_set_creds(struct linux_binprm *bprm)
+{
+ struct file *file = bprm->file;
+ struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
+ struct inode *file_inode = d_backing_inode(file->f_path.dentry);
Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?
Good catch. How does this look:
spin_lock(>i_lock);
spin_lock(_inode->i_lock);
if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
reason1 = "directory not owned by user";
else if (inode->i_mode & 0002)
reason1 = "file in world-writable directory";
else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
reason1 = "file in group-writable directory";
else if (file_inode->i_mode & 0002)
reason1 = "file is world-writable";
spin_unlock(>i_lock);
spin_unlock(_inode->i_lock);
and likewise for other places in the code?
Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
> +static int tpe_bprm_set_creds(struct linux_binprm *bprm)
> +{
> + struct file *file = bprm->file;
> + struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
> + struct inode *file_inode = d_backing_inode(file->f_path.dentry);
Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?
Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM
On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:
> +static int tpe_bprm_set_creds(struct linux_binprm *bprm)
> +{
> + struct file *file = bprm->file;
> + struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
> + struct inode *file_inode = d_backing_inode(file->f_path.dentry);
Bloody wonderful. Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?
