Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, Dec 31, 2018 at 2:45 PM Eric Biggers wrote: > > 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. Argh., how embarrassing. And it turns out that James' suggestion to initialize token_mask would actually have fixed that, for subtle reasons (but subtle was what I didn't want). I detest that match_token() interface, but this key code then mis-uses it in ways it wasn't even meant for, and tries to "share" error paths that aren't actually common. I'll take your original patch, which I clearly should have done originally. Thanks, and sorry for the wasted time, Linus
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
Hi David and Linus, On Mon, Dec 17, 2018 at 12:02:01PM -0800, Linus Torvalds wrote: > On Mon, Dec 17, 2018 at 11:51 AM James Bottomley > wrote: > > > > If this is to replace Eric's patch, didn't you want to set token_mask > > to (1< > 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. David, where are the tests for these new keyctls? - Eric
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, Dec 17, 2018 at 7:43 PM Linus Torvalds
wrote:
>
> On Mon, Dec 17, 2018 at 10:13 AM Eric Biggers wrote:
> >
> > 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.
... also acked by 0 tests added by that commit.
> 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 syzkaller-bugs+unsubscr...@googlegroups.com.
> 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.
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, 2018-12-17 at 12:02 -0800, Linus Torvalds wrote: > On Mon, Dec 17, 2018 at 11:51 AM James Bottomley > wrote: > > > > If this is to replace Eric's patch, didn't you want to set > > token_mask to (1< > No, let's not add any extra code that is trying to be subtle. Subtle > interactions was where the bug came from. Sure; I suppose liking the TPM gives me a taste for subtlety. > 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. Actually, I'm afraid it's not a bug, it's a command line ordering thing. To check the policy digest length, we need to know which hash algorithm you're using, so if you're specifying a hash algorithm, it has to appear *before* a policydigest as a command input. I take it this is another subtlety you'd like documenting ...? > 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. Hey, not going to argue here. The whole policy session passed in is questionable because some of the actions the kernel takes on the key have to depend on what was in the policy (which you don't know and can't deduce from the hash). The only way to get it to work universally is to pass the actual policy in and have the kernel construct the policy session itself. Fortunately fixing it can be low priority because we don't seem to have any users. James
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, Dec 17, 2018 at 12:21 PM Mimi Zohar wrote: > > It's being used for parsing and displaying the policy, which do need > to be in sync. Yes, but it needs a comment somewhere. Also, the way you use those enums as array indices also implies that for your case, Opt_err should definitely not be -1, and it should instead be at the *end* of the enum list (the same way it's at the end of the array). That would also automatically mean that "Opt_measure" would have value 0, and the pl(token) macro shouldn't need any offsetting at all, because the enums and the array indices just automatically match up (as long as they are always updated together!) Linus
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, 2018-12-17 at 12:02 -0800, Linus Torvalds wrote: > 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. A file signature is more restrictive than just a file hash. I'll clean up this and the other ugliness. Mimi
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, 2018-12-17 at 11:06 -0800, Linus Torvalds wrote: > On Mon, Dec 17, 2018 at 10:49 AM Linus Torvalds > wrote: > > > > 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 Yikes! > 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. Will do. > > 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. Mimi
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, Dec 17, 2018 at 11:51 AM James Bottomley wrote: > > If this is to replace Eric's patch, didn't you want to set token_mask > to (1<
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, 2018-12-17 at 11:39 -0800, Linus Torvalds wrote: > On Mon, Dec 17, 2018 at 11:06 AM Linus Torvalds > wrote: > > > > 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<
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, Dec 17, 2018 at 11:06 AM Linus Torvalds
wrote:
>
> 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..
Linus
security/keys/keyctl_pkey.c | 2 +-
security/keys/trusted.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
index 783978842f13..70e65a2ff207 100644
--- a/security/keys/keyctl_pkey.c
+++ b/security/keys/keyctl_pkey.c
@@ -25,7 +25,7 @@ static void keyctl_pkey_params_free(struct kernel_pkey_params *params)
}
enum {
- Opt_err = -1,
+ Opt_err,
Opt_enc, /* "enc=" eg. "enc=oaep" */
Opt_hash, /* "hash=" eg. "hash=sha1" */
};
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ff6789365a12..697bfc6c8192 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -711,7 +711,7 @@ static int key_unseal(struct trusted_key_payload *p,
}
enum {
- Opt_err = -1,
+ Opt_err,
Opt_new, Opt_load, Opt_update,
Opt_keyhandle, Opt_keyauth, Opt_blobauth,
Opt_pcrinfo, Opt_pcrlock, Opt_migratable,
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, Dec 17, 2018 at 10:49 AM Linus Torvalds wrote: > > 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. Linus
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, Dec 17, 2018 at 10:43 AM Linus Torvalds wrote: > > Or maybe just remove it entirely, since it's clearly entirely > incorrect from the very start. .. or another alternative: remove the "Opt_err = -1" entirely. Again, this seems to be a buggy pattern exclusive to the security code. Nobody else does it, and using that negative value is basically the source of those two bugs. It seems pointless and wrong. 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. Hmm? Linus
Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string
On Mon, Dec 17, 2018 at 10:13 AM Eric Biggers wrote:
>
> 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
