Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
@ 2019-07-05 21:30 David Howells
  2019-07-09  3:15 ` pr-tracker-bot
  2019-07-10 18:35 ` Linus Torvalds
  0 siblings, 2 replies; 8+ messages in thread
From: David Howells @ 2019-07-05 21:30 UTC (permalink / raw)
  To: torvalds
  Cc: dhowells, jmorris, keyrings, netdev, linux-nfs, linux-cifs,
	linux-afs, linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel

Hi Linus,

Here's my fourth block of keyrings changes for the next merge window.  They
change the permissions model used by keys and keyrings to be based on an
internal ACL by the following means:

 (1) Replace the permissions mask internally with an ACL that contains a
     list of ACEs, each with a specific subject with a permissions mask.
     Potted default ACLs are available for new keys and keyrings.

     ACE subjects can be macroised to indicate the UID and GID specified on
     the key (which remain).  Future commits will be able to add additional
     subject types, such as specific UIDs or domain tags/namespaces.

     Also split a number of permissions to give finer control.  Examples
     include splitting the revocation permit from the change-attributes
     permit, thereby allowing someone to be granted permission to revoke a
     key without allowing them to change the owner; also the ability to
     join a keyring is split from the ability to link to it, thereby
     stopping a process accessing a keyring by joining it and thus
     acquiring use of possessor permits.

 (2) Provide a keyctl to allow the granting or denial of one or more
     permits to a specific subject.  Direct access to the ACL is not
     granted, and the ACL cannot be viewed.

David
---
The following changes since commit a58946c158a040068e7c94dc1d58bbd273258068:

  keys: Pass the network namespace into request_key mechanism (2019-06-27 23:02:12 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-acl-20190703

for you to fetch changes up to 7a1ade847596dadc94b37e49f8c03f167fd71748:

  keys: Provide KEYCTL_GRANT_PERMISSION (2019-07-03 13:05:22 +0100)

----------------------------------------------------------------
Keyrings ACL

----------------------------------------------------------------
David Howells (2):
      keys: Replace uid/gid/perm permissions checking with an ACL
      keys: Provide KEYCTL_GRANT_PERMISSION

 Documentation/security/keys/core.rst               | 128 ++++++--
 Documentation/security/keys/request-key.rst        |   9 +-
 certs/blacklist.c                                  |   7 +-
 certs/system_keyring.c                             |  12 +-
 drivers/md/dm-crypt.c                              |   2 +-
 drivers/nvdimm/security.c                          |   2 +-
 fs/afs/security.c                                  |   2 +-
 fs/cifs/cifs_spnego.c                              |  25 +-
 fs/cifs/cifsacl.c                                  |  28 +-
 fs/cifs/connect.c                                  |   4 +-
 fs/crypto/keyinfo.c                                |   2 +-
 fs/ecryptfs/ecryptfs_kernel.h                      |   2 +-
 fs/ecryptfs/keystore.c                             |   2 +-
 fs/fscache/object-list.c                           |   2 +-
 fs/nfs/nfs4idmap.c                                 |  30 +-
 fs/ubifs/auth.c                                    |   2 +-
 include/linux/key.h                                | 121 +++----
 include/uapi/linux/keyctl.h                        |  65 ++++
 lib/digsig.c                                       |   2 +-
 net/ceph/ceph_common.c                             |   2 +-
 net/dns_resolver/dns_key.c                         |  12 +-
 net/dns_resolver/dns_query.c                       |  15 +-
 net/rxrpc/key.c                                    |  19 +-
 net/wireless/reg.c                                 |   6 +-
 security/integrity/digsig.c                        |  31 +-
 security/integrity/digsig_asymmetric.c             |   2 +-
 security/integrity/evm/evm_crypto.c                |   2 +-
 security/integrity/ima/ima_mok.c                   |  13 +-
 security/integrity/integrity.h                     |   6 +-
 .../integrity/platform_certs/platform_keyring.c    |  14 +-
 security/keys/compat.c                             |   2 +
 security/keys/encrypted-keys/encrypted.c           |   2 +-
 security/keys/encrypted-keys/masterkey_trusted.c   |   2 +-
 security/keys/gc.c                                 |   2 +-
 security/keys/internal.h                           |  16 +-
 security/keys/key.c                                |  29 +-
 security/keys/keyctl.c                             | 104 ++++--
 security/keys/keyring.c                            |  27 +-
 security/keys/permission.c                         | 361 +++++++++++++++++++--
 security/keys/persistent.c                         |  27 +-
 security/keys/proc.c                               |  22 +-
 security/keys/process_keys.c                       |  86 +++--
 security/keys/request_key.c                        |  34 +-
 security/keys/request_key_auth.c                   |  15 +-
 security/selinux/hooks.c                           |  16 +-
 security/smack/smack_lsm.c                         |   3 +-
 46 files changed, 992 insertions(+), 325 deletions(-)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
  2019-07-05 21:30 [GIT PULL] Keys: Set 4 - Key ACLs for 5.3 David Howells
@ 2019-07-09  3:15 ` pr-tracker-bot
  2019-07-10 18:35 ` Linus Torvalds
  1 sibling, 0 replies; 8+ messages in thread
From: pr-tracker-bot @ 2019-07-09  3:15 UTC (permalink / raw)
  To: David Howells
  Cc: torvalds, dhowells, jmorris, keyrings, netdev, linux-nfs,
	linux-cifs, linux-afs, linux-fsdevel, linux-integrity,
	linux-security-module, linux-kernel

The pull request you sent on Fri, 05 Jul 2019 22:30:39 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-acl-20190703

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/0f75ef6a9cff49ff612f7ce0578bced9d0b38325

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
  2019-07-05 21:30 [GIT PULL] Keys: Set 4 - Key ACLs for 5.3 David Howells
  2019-07-09  3:15 ` pr-tracker-bot
@ 2019-07-10 18:35 ` Linus Torvalds
  2019-07-10 19:46   ` Eric Biggers
  1 sibling, 1 reply; 8+ messages in thread
From: Linus Torvalds @ 2019-07-10 18:35 UTC (permalink / raw)
  To: David Howells
  Cc: James Morris James Morris, keyrings, Netdev, linux-nfs, CIFS,
	linux-afs, linux-fsdevel, linux-integrity, LSM List,
	Linux List Kernel Mailing

On Fri, Jul 5, 2019 at 2:30 PM David Howells <dhowells@redhat.com> wrote:
>
> Here's my fourth block of keyrings changes for the next merge window.  They
> change the permissions model used by keys and keyrings to be based on an
> internal ACL by the following means:

It turns out that this is broken, and I'll probably have to revert the
merge entirely.

With this merge in place, I can't boot any of the machines that have
an encrypted disk setup. The boot just stops at

  systemd[1]: Started Forward Password Requests to Plymouth Directory Watch.
  systemd[1]: Reached target Paths.

and never gets any further. I never get the prompt for a passphrase
for the disk encryption.

Apparently not a lot of developers are using encrypted volumes for
their development machines.

I'm not sure if the only requirement is an encrypted volume, or if
this is also particular to a F30 install in case you need to be able
to reproduce. But considering that you have a redhat email address,
I'm sure you can find a F30 install somewhere with an encrypted disk.

David, if you can fix this quickly, I'll hold off on the revert of it
all, but I can wait only so long. I've stopped merging stuff since I
noticed my machines don't work (this merge window has not been
pleasant so far - in addition to this issue I had another entirely
unrelated boot failure which made bisecting this one even more fun).

So if I don't see a quick fix, I'll just revert in order to then
continue to do pull requests later today. Because I do not want to do
further pulls with something that I can't boot as a base.

                 Linus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
  2019-07-10 18:35 ` Linus Torvalds
@ 2019-07-10 19:46   ` Eric Biggers
  2019-07-10 20:15     ` Eric Biggers
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Biggers @ 2019-07-10 19:46 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Howells, James Morris James Morris, keyrings, Netdev,
	linux-nfs, CIFS, linux-afs, linux-fsdevel, linux-integrity,
	LSM List, Linux List Kernel Mailing

On Wed, Jul 10, 2019 at 11:35:07AM -0700, Linus Torvalds wrote:
> On Fri, Jul 5, 2019 at 2:30 PM David Howells <dhowells@redhat.com> wrote:
> >
> > Here's my fourth block of keyrings changes for the next merge window.  They
> > change the permissions model used by keys and keyrings to be based on an
> > internal ACL by the following means:
> 
> It turns out that this is broken, and I'll probably have to revert the
> merge entirely.
> 
> With this merge in place, I can't boot any of the machines that have
> an encrypted disk setup. The boot just stops at
> 
>   systemd[1]: Started Forward Password Requests to Plymouth Directory Watch.
>   systemd[1]: Reached target Paths.
> 
> and never gets any further. I never get the prompt for a passphrase
> for the disk encryption.
> 
> Apparently not a lot of developers are using encrypted volumes for
> their development machines.
> 
> I'm not sure if the only requirement is an encrypted volume, or if
> this is also particular to a F30 install in case you need to be able
> to reproduce. But considering that you have a redhat email address,
> I'm sure you can find a F30 install somewhere with an encrypted disk.
> 
> David, if you can fix this quickly, I'll hold off on the revert of it
> all, but I can wait only so long. I've stopped merging stuff since I
> noticed my machines don't work (this merge window has not been
> pleasant so far - in addition to this issue I had another entirely
> unrelated boot failure which made bisecting this one even more fun).
> 
> So if I don't see a quick fix, I'll just revert in order to then
> continue to do pull requests later today. Because I do not want to do
> further pulls with something that I can't boot as a base.
> 
>                  Linus

This also broke 'keyctl new_session' and hence all the fscrypt tests
(https://lkml.kernel.org/lkml/20190710011559.GA7973@sol.localdomain/), and it
also broke loading in-kernel X.509 certificates
(https://lore.kernel.org/lkml/27671.1562384658@turing-police/T/#u).

I'm *guessing* these are all some underlying issue where keyrings aren't being
given all the needed permissions anymore.

But just FYI, David had said he's on vacation with no laptop or email access for
2 weeks starting from Sunday (3 days ago).  So I don't think you can expect a
quick fix from him.

I was planning to look into this to fix the fscrypt tests, but it might be a few
days before I get to it.  And while I'm *guessing* it will be a simple fix, it
might not be.  So I can't speak for David, but personally I'm fine with the
commits being reverted for now.

I'm also unhappy that the new keyctl KEYCTL_GRANT_PERMISSION doesn't have any
documentation or tests.  (Which seems to be a common problem with David's
work...  None of the new mount syscalls in v5.2 have any tests, for example, and
the man pages are still work-in-progress and last sent out for review a year
ago, despite API changes that occurred before the syscalls were merged.)

- Eric

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
  2019-07-10 19:46   ` Eric Biggers
@ 2019-07-10 20:15     ` Eric Biggers
  2019-07-11  1:59       ` Linus Torvalds
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Biggers @ 2019-07-10 20:15 UTC (permalink / raw)
  To: Linus Torvalds, David Howells, James Morris, keyrings, Netdev,
	linux-nfs, CIFS, linux-afs, linux-fsdevel, linux-integrity,
	LSM List, Linux List Kernel Mailing

On Wed, Jul 10, 2019 at 12:46:22PM -0700, Eric Biggers wrote:
> On Wed, Jul 10, 2019 at 11:35:07AM -0700, Linus Torvalds wrote:
> > On Fri, Jul 5, 2019 at 2:30 PM David Howells <dhowells@redhat.com> wrote:
> > >
> > > Here's my fourth block of keyrings changes for the next merge window.  They
> > > change the permissions model used by keys and keyrings to be based on an
> > > internal ACL by the following means:
> > 
> > It turns out that this is broken, and I'll probably have to revert the
> > merge entirely.
> > 
> > With this merge in place, I can't boot any of the machines that have
> > an encrypted disk setup. The boot just stops at
> > 
> >   systemd[1]: Started Forward Password Requests to Plymouth Directory Watch.
> >   systemd[1]: Reached target Paths.
> > 
> > and never gets any further. I never get the prompt for a passphrase
> > for the disk encryption.
> > 
> > Apparently not a lot of developers are using encrypted volumes for
> > their development machines.
> > 
> > I'm not sure if the only requirement is an encrypted volume, or if
> > this is also particular to a F30 install in case you need to be able
> > to reproduce. But considering that you have a redhat email address,
> > I'm sure you can find a F30 install somewhere with an encrypted disk.
> > 
> > David, if you can fix this quickly, I'll hold off on the revert of it
> > all, but I can wait only so long. I've stopped merging stuff since I
> > noticed my machines don't work (this merge window has not been
> > pleasant so far - in addition to this issue I had another entirely
> > unrelated boot failure which made bisecting this one even more fun).
> > 
> > So if I don't see a quick fix, I'll just revert in order to then
> > continue to do pull requests later today. Because I do not want to do
> > further pulls with something that I can't boot as a base.
> > 
> >                  Linus
> 
> This also broke 'keyctl new_session' and hence all the fscrypt tests
> (https://lkml.kernel.org/lkml/20190710011559.GA7973@sol.localdomain/), and it
> also broke loading in-kernel X.509 certificates
> (https://lore.kernel.org/lkml/27671.1562384658@turing-police/T/#u).
> 
> I'm *guessing* these are all some underlying issue where keyrings aren't being
> given all the needed permissions anymore.
> 
> But just FYI, David had said he's on vacation with no laptop or email access for
> 2 weeks starting from Sunday (3 days ago).  So I don't think you can expect a
> quick fix from him.
> 
> I was planning to look into this to fix the fscrypt tests, but it might be a few
> days before I get to it.  And while I'm *guessing* it will be a simple fix, it
> might not be.  So I can't speak for David, but personally I'm fine with the
> commits being reverted for now.
> 
> I'm also unhappy that the new keyctl KEYCTL_GRANT_PERMISSION doesn't have any
> documentation or tests.  (Which seems to be a common problem with David's
> work...  None of the new mount syscalls in v5.2 have any tests, for example, and
> the man pages are still work-in-progress and last sent out for review a year
> ago, despite API changes that occurred before the syscalls were merged.)
> 

Also worth noting that the key ACL patches were only in linux-next for 9 days
before the pull request was sent.  The X.509 certificate loading bug (which
might be the same underlying bug) was reported on July 6 by someone testing
linux-next, but the pull request had already been sent on July 5.  I suspect
these bug(s) would have been fixed if they had been in linux-next for longer.

- Eric

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
  2019-07-10 20:15     ` Eric Biggers
@ 2019-07-11  1:59       ` Linus Torvalds
  2019-07-11  3:07         ` Mimi Zohar
  2019-08-16 13:36         ` David Howells
  0 siblings, 2 replies; 8+ messages in thread
From: Linus Torvalds @ 2019-07-11  1:59 UTC (permalink / raw)
  To: Linus Torvalds, David Howells, James Morris, keyrings, Netdev,
	linux-nfs, CIFS, linux-afs, linux-fsdevel, linux-integrity,
	LSM List, Linux List Kernel Mailing

On Wed, Jul 10, 2019 at 1:15 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> Also worth noting that the key ACL patches were only in linux-next for 9 days
> before the pull request was sent.

Yes. I was not entirely happy with the whole key subsystem situation.
See my concerns in

  https://lore.kernel.org/lkml/CAHk-=wjEowdfG7v_4ttu3xhf9gqopj1+q1nGG86+mGfGDTEBBg@mail.gmail.com/

for more. That was before I realized it was buggy.

So it really would be good to have more people involved, and more
structure to the keys development (and, I suspect, much else under
security/)

Anyway, since it does seem like David is offline, I've just reverted
this from my tree, and will be continuing my normal merge window pull
requests (the other issues I have seen have fixes in their respective
trees).

                 Linus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
  2019-07-11  1:59       ` Linus Torvalds
@ 2019-07-11  3:07         ` Mimi Zohar
  2019-08-16 13:36         ` David Howells
  1 sibling, 0 replies; 8+ messages in thread
From: Mimi Zohar @ 2019-07-11  3:07 UTC (permalink / raw)
  To: Linus Torvalds, David Howells, James Morris, keyrings, Netdev,
	linux-nfs, CIFS, linux-afs, linux-fsdevel, linux-integrity,
	LSM List, Linux List Kernel Mailing

Hi Linus,

On Wed, 2019-07-10 at 18:59 -0700, Linus Torvalds wrote:
> Anyway, since it does seem like David is offline, I've just reverted
> this from my tree, and will be continuing my normal merge window pull
> requests (the other issues I have seen have fixes in their respective
> trees).

Sorry for the delay.  An exception is needed for loading builtin keys
"KEY_ALLOC_BUILT_IN" onto a keyring that is not writable by userspace.
 The following works, but probably is not how David would handle the
exception.

diff --git a/security/keys/key.c b/security/keys/key.c
index 519211a996e7..a99332c1e014 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -896,7 +896,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
        /* if we're going to allocate a new key, we're going to have
         * to modify the keyring */
        ret = key_permission(keyring_ref, KEY_NEED_WRITE);
-       if (ret < 0) {
+       if (ret < 0 && !(flags & KEY_ALLOC_BUILT_IN)) {
                key_ref = ERR_PTR(ret);
                goto error_link_end;
        }

Mimi


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
  2019-07-11  1:59       ` Linus Torvalds
  2019-07-11  3:07         ` Mimi Zohar
@ 2019-08-16 13:36         ` David Howells
  1 sibling, 0 replies; 8+ messages in thread
From: David Howells @ 2019-08-16 13:36 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: dhowells, Linus Torvalds, James Morris, keyrings, Netdev,
	linux-nfs, CIFS, linux-afs, linux-fsdevel, linux-integrity,
	LSM List, Linux List Kernel Mailing

Mimi Zohar <zohar@linux.ibm.com> wrote:

> Sorry for the delay.  An exception is needed for loading builtin keys
> "KEY_ALLOC_BUILT_IN" onto a keyring that is not writable by userspace.
>  The following works, but probably is not how David would handle the
> exception.

I think the attached is the right way to fix it.

load_system_certificate_list(), for example, when it creates keys does this:

	key = key_create_or_update(make_key_ref(builtin_trusted_keys, 1),

marking the keyring as "possessed" in make_key_ref().  This allows the
possessor permits to be used - and that's the *only* way to use them for
internal keyrings like this because you can't link to them and you can't join
them.

David
---
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 57be78b5fdfc..1f8f26f7bb05 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -99,7 +99,7 @@ static __init int system_trusted_keyring_init(void)
 	builtin_trusted_keys =
 		keyring_alloc(".builtin_trusted_keys",
 			      KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
-			      &internal_key_acl, KEY_ALLOC_NOT_IN_QUOTA,
+			      &internal_keyring_acl, KEY_ALLOC_NOT_IN_QUOTA,
 			      NULL, NULL);
 	if (IS_ERR(builtin_trusted_keys))
 		panic("Can't allocate builtin trusted keyring\n");
diff --git a/security/keys/permission.c b/security/keys/permission.c
index fc84d9ef6239..86efd3eaf083 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -47,7 +47,7 @@ struct key_acl internal_keyring_acl = {
 	.usage	= REFCOUNT_INIT(1),
 	.nr_ace	= 2,
 	.aces = {
-		KEY_POSSESSOR_ACE(KEY_ACE_SEARCH),
+		KEY_POSSESSOR_ACE(KEY_ACE_SEARCH | KEY_ACE_WRITE),
 		KEY_OWNER_ACE(KEY_ACE_VIEW | KEY_ACE_READ | KEY_ACE_SEARCH),
 	}
 };

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-05 21:30 [GIT PULL] Keys: Set 4 - Key ACLs for 5.3 David Howells
2019-07-09  3:15 ` pr-tracker-bot
2019-07-10 18:35 ` Linus Torvalds
2019-07-10 19:46   ` Eric Biggers
2019-07-10 20:15     ` Eric Biggers
2019-07-11  1:59       ` Linus Torvalds
2019-07-11  3:07         ` Mimi Zohar
2019-08-16 13:36         ` David Howells

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org linux-cifs@archiver.kernel.org
	public-inbox-index linux-cifs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox