From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jason A. Donenfeld" Date: Sun, 17 Sep 2017 11:50:40 +0000 Subject: Re: [PATCH v5] security/keys: rewrite all of big_key crypto Message-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit List-Id: References: <20170916130034.17706-1-Jason@zx2c4.com> <20170916130533.23036-1-Jason@zx2c4.com> <20170917060457.GB10599@zzz.localdomain> In-Reply-To: <20170917060457.GB10599@zzz.localdomain> To: Eric Biggers Cc: linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, kernel-hardening@lists.openwall.com, LKML , David Howells , Herbert Xu , Kirill Marinushkin , security@kernel.org, stable@vger.kernel.org On Sun, Sep 17, 2017 at 8:04 AM, Eric Biggers wrote: > This should jump to 'err_enckey', otherwise it will leak 'enckey'. Yikes, good catch, thanks! > > Otherwise the changes all look good; after fixing the above, feel free to add my > Reviewed-by. Ack. > Yes, AES-GCM is the right choice here. It is, however, almost > certainly the case that if someone can modify your swap partition, they can > already own your system in many other ways, so the "authenticated" portion of > "authenticated encryption" may not actually buy much in this situation :-) True, though this is slightly different from people putting their big_keys on NFS, I guess (but who would do such a thing anyway?). > The patch is a little long and perhaps should be split into several patches, > each of which fixes one bug; but see what David thinks. Meh, it's a rewrite, so sure, it's long. I saw a bunch of bugs and rewrote the whole thing rather than going one-by-one with the bugs, which would have produced a series. Personally I'd prefer to keep it like this than spending the time needling away with git and extra commit messages and struggling to make them each build separately. Seems like very very little gain for the time required to do that right. *shrug* Will wait to hear from David. > I should also note, that while there definitely were some inadmissible bugs > here, the support for encrypting big_key's was only added recently, in the v4.7 > kernel. And obviously not encrypting at all is at least as much as a > "vulnerability" as using weak encryption. Yea that's fair. I'm mostly just running around like a headless chicken after seeing ECB and wondering how this got committed in the first place, but nobody really takes chickens seriously. But yes, I suppose one way of considering this is just to say, "big_keys did not use encryption before 4.14, even though the code was really complicated." > I'm also a little skeptical that > people actually care enough about big_key's for it to be worthwhile to mark a > rewrite like this for stable, though I suppose it wouldn't be *too* hard to at > least cherry-pick this to 4.9 if you wanted. (There is a small conflict so > you'd have to send the backport yourself after this goes into mainline.) I imagine the conflicts will be moving back from get_random_bytes_wait to get_random_bytes and maybe the loff_t/kernel_{read,write} thing. Is this what you had in mind too? I should be able to handle those fairly easily and send it to Greg manually after this lands. Thanks again for the review, very appreciated. Regards, Jason From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751402AbdIQLur (ORCPT ); Sun, 17 Sep 2017 07:50:47 -0400 Received: from frisell.zx2c4.com ([192.95.5.64]:57829 "EHLO frisell.zx2c4.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750803AbdIQLuo (ORCPT ); Sun, 17 Sep 2017 07:50:44 -0400 X-Google-Smtp-Source: AOwi7QAdmRoQRtGU+UbYTYgnF74sQIrtuDnQZlHVRz+mIWdznBa2cSPNUW7QLzqIxoRVyKNixptAeKCPiiCXIyFd4wM= MIME-Version: 1.0 In-Reply-To: <20170917060457.GB10599@zzz.localdomain> References: <20170916130034.17706-1-Jason@zx2c4.com> <20170916130533.23036-1-Jason@zx2c4.com> <20170917060457.GB10599@zzz.localdomain> From: "Jason A. Donenfeld" Date: Sun, 17 Sep 2017 13:50:40 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5] security/keys: rewrite all of big_key crypto To: Eric Biggers Cc: linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, kernel-hardening@lists.openwall.com, LKML , David Howells , Herbert Xu , Kirill Marinushkin , security@kernel.org, stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Sep 17, 2017 at 8:04 AM, Eric Biggers wrote: > This should jump to 'err_enckey', otherwise it will leak 'enckey'. Yikes, good catch, thanks! > > Otherwise the changes all look good; after fixing the above, feel free to add my > Reviewed-by. Ack. > Yes, AES-GCM is the right choice here. It is, however, almost > certainly the case that if someone can modify your swap partition, they can > already own your system in many other ways, so the "authenticated" portion of > "authenticated encryption" may not actually buy much in this situation :-) True, though this is slightly different from people putting their big_keys on NFS, I guess (but who would do such a thing anyway?). > The patch is a little long and perhaps should be split into several patches, > each of which fixes one bug; but see what David thinks. Meh, it's a rewrite, so sure, it's long. I saw a bunch of bugs and rewrote the whole thing rather than going one-by-one with the bugs, which would have produced a series. Personally I'd prefer to keep it like this than spending the time needling away with git and extra commit messages and struggling to make them each build separately. Seems like very very little gain for the time required to do that right. *shrug* Will wait to hear from David. > I should also note, that while there definitely were some inadmissible bugs > here, the support for encrypting big_key's was only added recently, in the v4.7 > kernel. And obviously not encrypting at all is at least as much as a > "vulnerability" as using weak encryption. Yea that's fair. I'm mostly just running around like a headless chicken after seeing ECB and wondering how this got committed in the first place, but nobody really takes chickens seriously. But yes, I suppose one way of considering this is just to say, "big_keys did not use encryption before 4.14, even though the code was really complicated." > I'm also a little skeptical that > people actually care enough about big_key's for it to be worthwhile to mark a > rewrite like this for stable, though I suppose it wouldn't be *too* hard to at > least cherry-pick this to 4.9 if you wanted. (There is a small conflict so > you'd have to send the backport yourself after this goes into mainline.) I imagine the conflicts will be moving back from get_random_bytes_wait to get_random_bytes and maybe the loff_t/kernel_{read,write} thing. Is this what you had in mind too? I should be able to handle those fairly easily and send it to Greg manually after this lands. Thanks again for the review, very appreciated. Regards, Jason From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason@zx2c4.com (Jason A. Donenfeld) Date: Sun, 17 Sep 2017 13:50:40 +0200 Subject: [PATCH v5] security/keys: rewrite all of big_key crypto In-Reply-To: <20170917060457.GB10599@zzz.localdomain> References: <20170916130034.17706-1-Jason@zx2c4.com> <20170916130533.23036-1-Jason@zx2c4.com> <20170917060457.GB10599@zzz.localdomain> Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Sun, Sep 17, 2017 at 8:04 AM, Eric Biggers wrote: > This should jump to 'err_enckey', otherwise it will leak 'enckey'. Yikes, good catch, thanks! > > Otherwise the changes all look good; after fixing the above, feel free to add my > Reviewed-by. Ack. > Yes, AES-GCM is the right choice here. It is, however, almost > certainly the case that if someone can modify your swap partition, they can > already own your system in many other ways, so the "authenticated" portion of > "authenticated encryption" may not actually buy much in this situation :-) True, though this is slightly different from people putting their big_keys on NFS, I guess (but who would do such a thing anyway?). > The patch is a little long and perhaps should be split into several patches, > each of which fixes one bug; but see what David thinks. Meh, it's a rewrite, so sure, it's long. I saw a bunch of bugs and rewrote the whole thing rather than going one-by-one with the bugs, which would have produced a series. Personally I'd prefer to keep it like this than spending the time needling away with git and extra commit messages and struggling to make them each build separately. Seems like very very little gain for the time required to do that right. *shrug* Will wait to hear from David. > I should also note, that while there definitely were some inadmissible bugs > here, the support for encrypting big_key's was only added recently, in the v4.7 > kernel. And obviously not encrypting at all is at least as much as a > "vulnerability" as using weak encryption. Yea that's fair. I'm mostly just running around like a headless chicken after seeing ECB and wondering how this got committed in the first place, but nobody really takes chickens seriously. But yes, I suppose one way of considering this is just to say, "big_keys did not use encryption before 4.14, even though the code was really complicated." > I'm also a little skeptical that > people actually care enough about big_key's for it to be worthwhile to mark a > rewrite like this for stable, though I suppose it wouldn't be *too* hard to at > least cherry-pick this to 4.9 if you wanted. (There is a small conflict so > you'd have to send the backport yourself after this goes into mainline.) I imagine the conflicts will be moving back from get_random_bytes_wait to get_random_bytes and maybe the loff_t/kernel_{read,write} thing. Is this what you had in mind too? I should be able to handle those fairly easily and send it to Greg manually after this lands. Thanks again for the review, very appreciated. Regards, Jason -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 MIME-Version: 1.0 In-Reply-To: <20170917060457.GB10599@zzz.localdomain> References: <20170916130034.17706-1-Jason@zx2c4.com> <20170916130533.23036-1-Jason@zx2c4.com> <20170917060457.GB10599@zzz.localdomain> From: "Jason A. Donenfeld" Date: Sun, 17 Sep 2017 13:50:40 +0200 Message-ID: Content-Type: text/plain; charset="UTF-8" Subject: [kernel-hardening] Re: [PATCH v5] security/keys: rewrite all of big_key crypto To: Eric Biggers Cc: linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, kernel-hardening@lists.openwall.com, LKML , David Howells , Herbert Xu , Kirill Marinushkin , security@kernel.org, stable@vger.kernel.org List-ID: On Sun, Sep 17, 2017 at 8:04 AM, Eric Biggers wrote: > This should jump to 'err_enckey', otherwise it will leak 'enckey'. Yikes, good catch, thanks! > > Otherwise the changes all look good; after fixing the above, feel free to add my > Reviewed-by. Ack. > Yes, AES-GCM is the right choice here. It is, however, almost > certainly the case that if someone can modify your swap partition, they can > already own your system in many other ways, so the "authenticated" portion of > "authenticated encryption" may not actually buy much in this situation :-) True, though this is slightly different from people putting their big_keys on NFS, I guess (but who would do such a thing anyway?). > The patch is a little long and perhaps should be split into several patches, > each of which fixes one bug; but see what David thinks. Meh, it's a rewrite, so sure, it's long. I saw a bunch of bugs and rewrote the whole thing rather than going one-by-one with the bugs, which would have produced a series. Personally I'd prefer to keep it like this than spending the time needling away with git and extra commit messages and struggling to make them each build separately. Seems like very very little gain for the time required to do that right. *shrug* Will wait to hear from David. > I should also note, that while there definitely were some inadmissible bugs > here, the support for encrypting big_key's was only added recently, in the v4.7 > kernel. And obviously not encrypting at all is at least as much as a > "vulnerability" as using weak encryption. Yea that's fair. I'm mostly just running around like a headless chicken after seeing ECB and wondering how this got committed in the first place, but nobody really takes chickens seriously. But yes, I suppose one way of considering this is just to say, "big_keys did not use encryption before 4.14, even though the code was really complicated." > I'm also a little skeptical that > people actually care enough about big_key's for it to be worthwhile to mark a > rewrite like this for stable, though I suppose it wouldn't be *too* hard to at > least cherry-pick this to 4.9 if you wanted. (There is a small conflict so > you'd have to send the backport yourself after this goes into mainline.) I imagine the conflicts will be moving back from get_random_bytes_wait to get_random_bytes and maybe the loff_t/kernel_{read,write} thing. Is this what you had in mind too? I should be able to handle those fairly easily and send it to Greg manually after this lands. Thanks again for the review, very appreciated. Regards, Jason