From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Biggers Date: Thu, 21 Sep 2017 23:34:16 +0000 Subject: Re: [PATCH] KEYS: prevent KEYCTL_READ on negative key Message-Id: <20170921233416.GC89627@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit List-Id: References: <20170918183723.114253-1-ebiggers3@gmail.com> <30124.1505837342@warthog.procyon.org.uk> In-Reply-To: <30124.1505837342@warthog.procyon.org.uk> To: linux-security-module@vger.kernel.org On Tue, Sep 19, 2017 at 05:09:02PM +0100, David Howells wrote: > Eric Biggers wrote: > > > + if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) { > > + ret = -ENOKEY; > > + goto error2; > > + } > > It might be better to do this check in key_validate(). Also, it should > perhaps take the error from key->reject_error. > > David Putting the check in key_validate() would make lookups with KEY_LOOKUP_PARTIAL stop returning negative keys, which would break keyctl_describe(), keyctl_chown(), keyctl_setperm(), keyctl_set_timeout(), keyctl_get_security() on negative keys. I presume those are supposed to work? Another solution would be to remove the special case from lookup_user_key() where it can return a negative/revoked/invalidated/expired key if KEY_LOOKUP_PARTIAL is not specified and the 'perm' mask is 0. The only callers it would affect are the case in question here which is clearly a bug, and the root-only exceptions for keyctl_invalidate() and keyctl_clear(). And I suspect the latter two are unintentional as well. (Is root *supposed* to be able to invalidate a negative/revoked/invalidated/expired key, or clear a revoked/invalidated/expired keyring?) Eric From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiggers3@gmail.com (Eric Biggers) Date: Thu, 21 Sep 2017 16:34:16 -0700 Subject: [PATCH] KEYS: prevent KEYCTL_READ on negative key In-Reply-To: <30124.1505837342@warthog.procyon.org.uk> References: <20170918183723.114253-1-ebiggers3@gmail.com> <30124.1505837342@warthog.procyon.org.uk> Message-ID: <20170921233416.GC89627@gmail.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Tue, Sep 19, 2017 at 05:09:02PM +0100, David Howells wrote: > Eric Biggers wrote: > > > + if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) { > > + ret = -ENOKEY; > > + goto error2; > > + } > > It might be better to do this check in key_validate(). Also, it should > perhaps take the error from key->reject_error. > > David Putting the check in key_validate() would make lookups with KEY_LOOKUP_PARTIAL stop returning negative keys, which would break keyctl_describe(), keyctl_chown(), keyctl_setperm(), keyctl_set_timeout(), keyctl_get_security() on negative keys. I presume those are supposed to work? Another solution would be to remove the special case from lookup_user_key() where it can return a negative/revoked/invalidated/expired key if KEY_LOOKUP_PARTIAL is not specified and the 'perm' mask is 0. The only callers it would affect are the case in question here which is clearly a bug, and the root-only exceptions for keyctl_invalidate() and keyctl_clear(). And I suspect the latter two are unintentional as well. (Is root *supposed* to be able to invalidate a negative/revoked/invalidated/expired key, or clear a revoked/invalidated/expired keyring?) Eric -- 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 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751921AbdIUXeW (ORCPT ); Thu, 21 Sep 2017 19:34:22 -0400 Received: from mail-pf0-f180.google.com ([209.85.192.180]:49272 "EHLO mail-pf0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751728AbdIUXeU (ORCPT ); Thu, 21 Sep 2017 19:34:20 -0400 X-Google-Smtp-Source: AOwi7QC6ON7YBdBIv6hQ1Ux7ti7UyJoiJuGcnn/C4LQqz6ZjTmaqb7Scgfg0EcpvgUuGpnfaqV0TXQ== Date: Thu, 21 Sep 2017 16:34:16 -0700 From: Eric Biggers To: David Howells Cc: keyrings@vger.kernel.org, Michael Halcrow , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Eric Biggers , stable@vger.kernel.org Subject: Re: [PATCH] KEYS: prevent KEYCTL_READ on negative key Message-ID: <20170921233416.GC89627@gmail.com> References: <20170918183723.114253-1-ebiggers3@gmail.com> <30124.1505837342@warthog.procyon.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <30124.1505837342@warthog.procyon.org.uk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 19, 2017 at 05:09:02PM +0100, David Howells wrote: > Eric Biggers wrote: > > > + if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) { > > + ret = -ENOKEY; > > + goto error2; > > + } > > It might be better to do this check in key_validate(). Also, it should > perhaps take the error from key->reject_error. > > David Putting the check in key_validate() would make lookups with KEY_LOOKUP_PARTIAL stop returning negative keys, which would break keyctl_describe(), keyctl_chown(), keyctl_setperm(), keyctl_set_timeout(), keyctl_get_security() on negative keys. I presume those are supposed to work? Another solution would be to remove the special case from lookup_user_key() where it can return a negative/revoked/invalidated/expired key if KEY_LOOKUP_PARTIAL is not specified and the 'perm' mask is 0. The only callers it would affect are the case in question here which is clearly a bug, and the root-only exceptions for keyctl_invalidate() and keyctl_clear(). And I suspect the latter two are unintentional as well. (Is root *supposed* to be able to invalidate a negative/revoked/invalidated/expired key, or clear a revoked/invalidated/expired keyring?) Eric