[ghak90,V7,06/21] audit: contid limit of 32k imposed to avoid DoS

Submitted by Richard Guy Briggs on Sept. 19, 2019, 1:22 a.m.

Details

Message ID 230e91cd3e50a3d8015daac135c24c4c58cf0a21.1568834524.git.rgb@redhat.com
State New
Series "audit: implement container identifier"
Headers show

Commit Message

Richard Guy Briggs Sept. 19, 2019, 1:22 a.m.
Set an arbitrary limit on the number of audit container identifiers to
limit abuse.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c | 8 ++++++++
 kernel/audit.h | 4 ++++
 2 files changed, 12 insertions(+)

Patch hide | download patch | download mbox

diff --git a/kernel/audit.c b/kernel/audit.c
index 53d13d638c63..329916534dd2 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -139,6 +139,7 @@  struct audit_net {
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
 /* Hash for contid-based rules */
 struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+int audit_contid_count = 0;
 
 static struct kmem_cache *audit_buffer_cache;
 
@@ -2384,6 +2385,7 @@  void audit_cont_put(struct audit_cont *cont)
 		put_task_struct(cont->owner);
 		list_del_rcu(&cont->list);
 		kfree_rcu(cont, rcu);
+		audit_contid_count--;
 	}
 }
 
@@ -2456,6 +2458,11 @@  int audit_set_contid(struct task_struct *task, u64 contid)
 					goto conterror;
 				}
 			}
+		/* Set max contids */
+		if (audit_contid_count > AUDIT_CONTID_COUNT) {
+			rc = -ENOSPC;
+			goto conterror;
+		}
 		if (!newcont) {
 			newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
 			if (newcont) {
@@ -2465,6 +2472,7 @@  int audit_set_contid(struct task_struct *task, u64 contid)
 				newcont->owner = current;
 				refcount_set(&newcont->refcount, 1);
 				list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+				audit_contid_count++;
 			} else {
 				rc = -ENOMEM;
 				goto conterror;
diff --git a/kernel/audit.h b/kernel/audit.h
index 162de8366b32..543f1334ba47 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -219,6 +219,10 @@  static inline int audit_hash_contid(u64 contid)
 	return (contid & (AUDIT_CONTID_BUCKETS-1));
 }
 
+extern int audit_contid_count;
+
+#define AUDIT_CONTID_COUNT	1 << 16
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1
 

Comments

Neil Horman Sept. 27, 2019, 12:51 p.m.
On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote:
> Set an arbitrary limit on the number of audit container identifiers to
> limit abuse.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c | 8 ++++++++
>  kernel/audit.h | 4 ++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 53d13d638c63..329916534dd2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -139,6 +139,7 @@ struct audit_net {
>  struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
>  /* Hash for contid-based rules */
>  struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
> +int audit_contid_count = 0;
>  
>  static struct kmem_cache *audit_buffer_cache;
>  
> @@ -2384,6 +2385,7 @@ void audit_cont_put(struct audit_cont *cont)
>  		put_task_struct(cont->owner);
>  		list_del_rcu(&cont->list);
>  		kfree_rcu(cont, rcu);
> +		audit_contid_count--;
>  	}
>  }
>  
> @@ -2456,6 +2458,11 @@ int audit_set_contid(struct task_struct *task, u64 contid)
>  					goto conterror;
>  				}
>  			}
> +		/* Set max contids */
> +		if (audit_contid_count > AUDIT_CONTID_COUNT) {
> +			rc = -ENOSPC;
> +			goto conterror;
> +		}
You should check for audit_contid_count == AUDIT_CONTID_COUNT here, no?
or at least >=, since you increment it below.  Otherwise its possible
that you will exceed it by one in the full condition.

>  		if (!newcont) {
>  			newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
>  			if (newcont) {
> @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
>  				newcont->owner = current;
>  				refcount_set(&newcont->refcount, 1);
>  				list_add_rcu(&newcont->list, &audit_contid_hash[h]);
> +				audit_contid_count++;
>  			} else {
>  				rc = -ENOMEM;
>  				goto conterror;
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 162de8366b32..543f1334ba47 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
>  	return (contid & (AUDIT_CONTID_BUCKETS-1));
>  }
>  
> +extern int audit_contid_count;
> +
> +#define AUDIT_CONTID_COUNT	1 << 16
> +
Just to ask the question, since it wasn't clear in the changelog, what
abuse are you avoiding here?  Ostensibly you should be able to create as
many container ids as you have space for, and the simple creation of
container ids doesn't seem like the resource strain I would be concerned
about here, given that an orchestrator can still create as many
containers as the system will otherwise allow, which will consume
significantly more ram/disk/etc.

>  /* Indicates that audit should log the full pathname. */
>  #define AUDIT_NAME_FULL -1
>  
> -- 
> 1.8.3.1
> 
>
Paul Moore Oct. 11, 2019, 12:38 a.m.
On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote:
> > Set an arbitrary limit on the number of audit container identifiers to
> > limit abuse.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.c | 8 ++++++++
> >  kernel/audit.h | 4 ++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 53d13d638c63..329916534dd2 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c

...

> > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> >                               newcont->owner = current;
> >                               refcount_set(&newcont->refcount, 1);
> >                               list_add_rcu(&newcont->list, &audit_contid_hash[h]);
> > +                             audit_contid_count++;
> >                       } else {
> >                               rc = -ENOMEM;
> >                               goto conterror;
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 162de8366b32..543f1334ba47 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
> >       return (contid & (AUDIT_CONTID_BUCKETS-1));
> >  }
> >
> > +extern int audit_contid_count;
> > +
> > +#define AUDIT_CONTID_COUNT   1 << 16
> > +
>
> Just to ask the question, since it wasn't clear in the changelog, what
> abuse are you avoiding here?  Ostensibly you should be able to create as
> many container ids as you have space for, and the simple creation of
> container ids doesn't seem like the resource strain I would be concerned
> about here, given that an orchestrator can still create as many
> containers as the system will otherwise allow, which will consume
> significantly more ram/disk/etc.

I've got a similar question.  Up to this point in the patchset, there
is a potential issue of hash bucket chain lengths and traversing them
with a spinlock held, but it seems like we shouldn't be putting an
arbitrary limit on audit container IDs unless we have a good reason
for it.  If for some reason we do want to enforce a limit, it should
probably be a tunable value like a sysctl, or similar.

--
paul moore
www.paul-moore.com