general protection fault in keyctl_pkey_params_get

From: Eric Biggers <[email protected]>

We need to check the return value of match_token() for Opt_err (-1) before doing anything with it.

Reported-by [email protected]

Fixes 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]")

Signed-off-by Eric Biggers <[email protected]>

---
 security/keys/keyctl_pkey.c | 2 ++
 1 file changed, 2 insertions(+)

security/keys/keyctl_pkey.c +2 -0

index 783978842f13a..987fac8051d70 100644
@@ -50,6 +50,8 @@ static int keyctl_pkey_params_parse(struct kernel_pkey_params *params)
 		if (*p = '\0' || *p = ' ' || *p = '\t')
 			continue;
 		token = match_token(p, param_keys, args);
+		if (token = Opt_err)
+			return -EINVAL;
 		if (__test_and_set_bit(token, &token_mask))
 			return -EINVAL;
 		q = args[0].from;

Signature · 1 line

2.19.1

Quoted 27 lines · Eric Biggers

From: Eric Biggers <[email protected]>

We need to check the return value of match_token() for Opt_err (-1) before doing anything with it.

Reported-by [email protected]

Fixes 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]")

Signed-off-by Eric Biggers <[email protected]>

---
 security/keys/keyctl_pkey.c | 2 ++
 1 file changed, 2 insertions(+)

security/keys/keyctl_pkey.c +2 -0

index 783978842f13a..987fac8051d70 100644
@@ -50,6 +50,8 @@ static int keyctl_pkey_params_parse(struct kernel_pkey_params *params)
 		if (*p = '\0' || *p = ' ' || *p = '\t')
 			continue;
 		token = match_token(p, param_keys, args);
+		if (token = Opt_err)
+			return -EINVAL;
 		if (__test_and_set_bit(token, &token_mask))
 			return -EINVAL;
 		q = args[0].from;

Signature · 1 line

2.19.1

Ping. David, are you planning to apply this?

- Eric

Hi Linus, please consider applying this patch. It's been ignored by the keyrings maintainer for a month and a half with multiple reminders. It fixes an easily reachable stack corruption in the new keyctl operations that were added in v4.20. It was immediately reached by syzbot even without any definitions for the new keyctls yet.

Or maybe just remove it entirely, since it's clearly entirely incorrect from the very start.

So the *simplest* fix would seem to be to literally remove all those "= -1" for the Opt_err initialization. Making the code smaller, simpler, and fixing the bug in the process.

Honestly, for being about "security", all of this code seems to be doing some really questionable things with all those Opt_xyz enums.

Quoted 3 lines · Linus Torvalds

Honestly, for being about "security", all of this code seems to be doing some really questionable things with all those Opt_xyz enums.

Yeah, at least security/keys/trusted.c ends up mixing that enum and just using "int" completely randomly, and you have datablob_parse() returning either a negative integer _or_ an "Opt_xyz" value, so having Opt_err be -1 is doubly confusing there (it would also be "- EPERM" depending on how you treat it).

There doesn't seem to be any _actual_ confusion (because Opt_err is always turned into an actual real error code), but it's just another sign of "those enums should not be negative".

So on the whole, I think that the "Opt_err = -1" is a serious mistake, but at least for now, ima_policy.c clearly has (bogus) code that relies on it.

But the two cases that use "test_and_set_bit()" do not seem to have any reason to use that -1 enum, so while we can't do the "just remove -1" in general, I do think the proper fix is to just do it for those two files.

Eric, mind testing a patch like that? Untested patch attached just for completeness..

If this is to replace Eric's patch, didn't you want to set token_mask to (1<<Opt_err)?

Talking about the conflicting ones: Opt_hash checks that Opt_policydigest isn't set. But Opt_policydigest doesn't check that Opt_hash isn't set, so you can mix the two if you just do it in the right order.

But that's a separate bug, and doesn't seem to be a huge deal.

But it *is* an example of how bogus all of this stuff is. Clearly people weren't really paying attention when writing any of this code.

Quoted 3 lines · James Bottomley

If this is to replace Eric's patch, didn't you want to set token_mask to (1<<Opt_err)?

No, let's not add any extra code that is trying to be subtle. Subtle interactions was where the bug came from.

The code already checks the actual Opt_xyz for errors in a switch statement. The token_mask should be _purely_ about duplicate options (or conflicting ones).

Talking about the conflicting ones: Opt_hash checks that Opt_policydigest isn't set. But Opt_policydigest doesn't check that Opt_hash isn't set, so you can mix the two if you just do it in the right order.

But that's a separate bug, and doesn't seem to be a huge deal.

But it *is* an example of how bogus all of this stuff is. Clearly people weren't really paying attention when writing any of this code.

Quoted 3 lines · James Bottomley

If this is to replace Eric's patch, didn't you want to set token_mask to (1<<Opt_err)?

No, let's not add any extra code that is trying to be subtle. Subtle interactions was where the bug came from.

The code already checks the actual Opt_xyz for errors in a switch statement. The token_mask should be _purely_ about duplicate options (or conflicting ones).

Talking about the conflicting ones: Opt_hash checks that Opt_policydigest isn't set. But Opt_policydigest doesn't check that Opt_hash isn't set, so you can mix the two if you just do it in the right order.

But that's a separate bug, and doesn't seem to be a huge deal.

But it *is* an example of how bogus all of this stuff is. Clearly people weren't really paying attention when writing any of this code.

Linus

KEYCTL_PKEY_QUERY is still failing basic fuzzing even after Linus' fix that changed Opt_err from -1 to 0. The crash is still in keyctl_pkey_params_parse():

                token = match_token(p, param_keys, args);
                if (__test_and_set_bit(token, &token_mask))
                        return -EINVAL;
                q = args[0].from;
                if (!q[0])
                        return -EINVAL;

Now it crashes on '!q[0]' because 'args[0].from' is uninitialized when token = Opt_err. args[0] is only initialized when the parsed token had a pattern that set it.

Quoted 4 lines · Linus Torvalds

So the *simplest* fix would seem to be to literally remove all those "= -1" for the Opt_err initialization. Making the code smaller, simpler, and fixing the bug in the process.

Something like this

    git  grep -l 'Opt_err = -1' | xargs sed -i 's/Opt_err = -1/Opt_err/'

would do it, but I also notice that security/integrity/ima/ima_policy.c then has some truly funky code that plays around with the enum numbers , ie

  #define pt(token)     policy_tokens[token + Opt_err].pattern

which actually depends on the ordering of policy_tokens[], and depends on the exact values, ie it literally (and quite insanely) sets Opt_err to -1, and then Opt_measure to 1, and leaves 0 unused. That code seriously makes no sense at all, and is fundamentally broken.

It would be better to use

    #define pt(token)     policy_tokens[token - Opt_measure].pattern

instead, but even then you should have a huge comment about how the policy_tokens[] array absolutely has to be properly ordered.

Honestly, for being about "security", all of this code seems to be doing some really questionable things with all those Opt_xyz enums.

It's being used for parsing and displaying the policy, which do need to be in sync.

Quoted 6 lines · Eric Biggers

Hi Linus, please consider applying this patch. It's been ignored by the keyrings maintainer for a month and a half with multiple reminders. It fixes an easily reachable stack corruption in the new keyctl operations that were added in v4.20. It was immediately reached by syzbot even without any definitions for the new keyctls yet.

The getoptions() code in security/keys/trusted.c has exactly the same buggy pattern, and seems to actually be the source of that idiocy.

Mind fixing that one too and getting rid of this incorrect code entirely?

Also, maybe the right fix is to do the "check for duplicate tokens" only *after* all the other validation (ie after the switch())?

Or maybe just remove it entirely, since it's clearly entirely incorrect from the very start.

Finally, the code was actually originally introduced in commit 5208cc83423d ("keys, trusted: fix: *do not* allow duplicate key options"), this second place you found is just pattern matching from that original garbage, that was acked and "reviewed" by several people.

The fix should have that clarification. Commit 00d60fd3b932 wasn't the _origin_ of this bug, even if it made a copy of it.

Looking around, nobody else has picked up that incorrect pattern.

Linus

--
You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/CAHk-%3Dwhmh8WdcKHdYjioJNjyeewv%3DfO1H0hxXqDh6kiX0YvzmQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.