[RFC,4/4] security: Enforce limitations on return values from LSMs

Commit Message

Roberto Sassu Nov. 15, 2022, 5:56 p.m. UTC

From: Roberto Sassu <[e-mail]>

LSMs should not be able to return arbitrary return values, as the callers
of the LSM infrastructure might not be ready to handle unexpected values
(e.g. positive values that are first converted to a pointer with ERR_PTR,
and then evaluated with IS_ERR()).

Modify call_int_hook() to call is_ret_value_allowed(), so that the return
value from each LSM for a given hook is checked. If for the interval the
return value falls into the corresponding flag is not set, change the
return value to the default value, just for the current LSM.

A misbehaving LSM would not have impact on the decision of other LSMs, as
the loop terminates whenever the return value is not zero.

Signed-off-by: Roberto Sassu <[e-mail]>
---
 security/security.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Paul Moore Nov. 16, 2022, 2:35 a.m. UTC | #1

On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
<[e-mail]> wrote:
>
> From: Roberto Sassu <[e-mail]>
>
> LSMs should not be able to return arbitrary return values, as the callers
> of the LSM infrastructure might not be ready to handle unexpected values
> (e.g. positive values that are first converted to a pointer with ERR_PTR,
> and then evaluated with IS_ERR()).
>
> Modify call_int_hook() to call is_ret_value_allowed(), so that the return
> value from each LSM for a given hook is checked. If for the interval the
> return value falls into the corresponding flag is not set, change the
> return value to the default value, just for the current LSM.
>
> A misbehaving LSM would not have impact on the decision of other LSMs, as
> the loop terminates whenever the return value is not zero.
>
> Signed-off-by: Roberto Sassu <[e-mail]>
> ---
>  security/security.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)

Casey touched on some of this in his reply to patch 0/4, but basically
I see this as a BPF LSM specific problem and not a generalized LSM
issue that should be addressed at the LSM layer.  Especially if the
solution involves incurring additional processing for every LSM hook
instantiation, regardless if a BPF LSM is present.  Reading your
overall patchset description I believe that you understand this too.

If you want to somehow instrument the LSM hook definitions (what I
believe to be the motivation behind patch 3/4) to indicate valid
return values for use by the BPF verifier, I think we could entertain
that, or at least discuss it further, but I'm not inclined to support
any runtime overhead at the LSM layer for a specific LSM.

Roberto Sassu Nov. 16, 2022, 2:36 p.m. UTC | #2

On Tue, 2022-11-15 at 21:35 -0500, Paul Moore wrote:
> On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
> <[e-mail]> wrote:
> > From: Roberto Sassu <[e-mail]>
> > 
> > LSMs should not be able to return arbitrary return values, as the callers
> > of the LSM infrastructure might not be ready to handle unexpected values
> > (e.g. positive values that are first converted to a pointer with ERR_PTR,
> > and then evaluated with IS_ERR()).
> > 
> > Modify call_int_hook() to call is_ret_value_allowed(), so that the return
> > value from each LSM for a given hook is checked. If for the interval the
> > return value falls into the corresponding flag is not set, change the
> > return value to the default value, just for the current LSM.
> > 
> > A misbehaving LSM would not have impact on the decision of other LSMs, as
> > the loop terminates whenever the return value is not zero.
> > 
> > Signed-off-by: Roberto Sassu <[e-mail]>
> > ---
> >  security/security.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> 
> Casey touched on some of this in his reply to patch 0/4, but basically
> I see this as a BPF LSM specific problem and not a generalized LSM
> issue that should be addressed at the LSM layer.  Especially if the
> solution involves incurring additional processing for every LSM hook
> instantiation, regardless if a BPF LSM is present.  Reading your
> overall patchset description I believe that you understand this too.

Yes, I had this concern too. Thanks Paul and Casey for taking the time
to reply.

I liked the fact that the fix is extremely simple, but nevertheless it
should not impact the performance, if there are alternative ways. I
thought maybe we look at non-zero values, since the check is already
there. But it could be that there is an impact for it too (maybe for
audit_rule_match?).

> If you want to somehow instrument the LSM hook definitions (what I
> believe to be the motivation behind patch 3/4) to indicate valid
> return values for use by the BPF verifier, I think we could entertain
> that, or at least discuss it further, but I'm not inclined to support
> any runtime overhead at the LSM layer for a specific LSM.

Ok, yes. Patches 1-3 would help to keep in sync the LSM infrastructure
and eBPF, but it is not strictly needed. I could propose an eBPF-only
alternative to declare sets of functions per interval.

More or less, I developed an eBPF-based alternative also for patch 4.
It is just a proof of concept. Will propose it, to validate the idea.

Thanks

Roberto

Paul Moore Nov. 16, 2022, 10:06 p.m. UTC | #3

On Wed, Nov 16, 2022 at 9:37 AM Roberto Sassu
<[e-mail]> wrote:
> On Tue, 2022-11-15 at 21:35 -0500, Paul Moore wrote:
> > If you want to somehow instrument the LSM hook definitions (what I
> > believe to be the motivation behind patch 3/4) to indicate valid
> > return values for use by the BPF verifier, I think we could entertain
> > that, or at least discuss it further, but I'm not inclined to support
> > any runtime overhead at the LSM layer for a specific LSM.
>
> Ok, yes. Patches 1-3 would help to keep in sync the LSM infrastructure
> and eBPF, but it is not strictly needed. I could propose an eBPF-only
> alternative to declare sets of functions per interval.
>
> More or less, I developed an eBPF-based alternative also for patch 4.
> It is just a proof of concept. Will propose it, to validate the idea.

Thanks, I think that might be the best approach.  Also, please
resubmit patches 1/4 and 2/4 with those small changes; those are nice
improvements that just need a couple of small tweaks to be acceptable
:)

Patch

diff --git a/security/security.c b/security/security.c
index 4041d24e3283..cd417a8a0e65 100644
--- a/security/security.c
+++ b/security/security.c
@@ -716,6 +716,35 @@  static int lsm_superblock_alloc(struct super_block *sb)
 #include <linux/lsm_hook_defs.h>
 #undef LSM_HOOK
 
+/*
+ * The return value flags of the LSM hook are defined in linux/lsm_hook_defs.h
+ * and can be accessed with:
+ *
+ *	LSM_RET_FLAGS(<hook_name>)
+ *
+ * The macros below define static constants for the return value flags of each
+ * LSM hook.
+ */
+#define LSM_RET_FLAGS(NAME) (NAME##_ret_flags)
+#define DECLARE_LSM_RET_FLAGS(RET_FLAGS, NAME) \
+	static const u32 __maybe_unused LSM_RET_FLAGS(NAME) = (RET_FLAGS);
+#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
+	DECLARE_LSM_RET_FLAGS(RET_FLAGS, NAME)
+
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
+
+static bool is_ret_value_allowed(int ret, u32 ret_flags)
+{
+	if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
+	    (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
+	    (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
+	    (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
+		return false;
+
+	return true;
+}
+
 /*
  * Hook list operation macros.
  *
@@ -741,6 +770,11 @@  static int lsm_superblock_alloc(struct super_block *sb)
 								\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
 			RC = P->hook.FUNC(__VA_ARGS__);		\
+			if (!is_ret_value_allowed(RC, LSM_RET_FLAGS(FUNC))) { \
+				WARN_ONCE(1, "Illegal ret %d for " #FUNC " from %s, forcing %d\n", \
+					  RC, P->lsm, LSM_RET_DEFAULT(FUNC)); \
+				RC = LSM_RET_DEFAULT(FUNC);	\
+			}					\
 			if (RC != 0)				\
 				break;				\
 		}						\