Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link user keychain to session keychain #3246

Closed
wants to merge 1 commit into from

Conversation

smerrill
Copy link
Contributor

This prevents odd "mount(2)" errors with mount-chroot.

This prevents odd "mount(2)" errors with mount-chroot.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@smerrill
Copy link
Contributor Author

I could not mount my encrypted chroot on a Lenovo Thinkpad 13 running Version 59.0.3071.71 (Official Build) beta (64-bit) without this fix.

@smerrill
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@dnschneid
Copy link
Owner

Is this overkill, with potentially bad side-effects?

Would it be better to just make sure the decrypt key ends up in the appropriate keyring to begin with?

@drinkcat
Copy link
Collaborator

I don't really know about these keyrings, but linking the user keyring in the session keyring seems ok.

On samus-release/R58-9334.72.0, I see this right after login:

$ sudo keyctl list @s
1 key in keyring:
 92994838: --alswrv     0 65534 keyring: _uid.0
$ sudo keyctl list @u
2 keys in keyring:
286101033: --alswrv     0     0 user: a5aeddcf6261f807
905769048: --alswrv     0     0 user: d224878099ec22d9

While on samus-release/R61-9647.0.0 (ext4 crypto) the link is not setup (there is a dircrypt keyring instead):

$ sudo keyctl list @u
keyring is empty
$ sudo keyctl list @s
1 key in keyring:
308501995: --alswrv     0     0 keyring: dircrypt

But after linking it looks like this, which seems to make sense.

$ sudo keyctl list @s
2 keys in keyring:
991469042: ---ls-rv     0     0 keyring: dircrypt
259439550: ---lswrv     0 65534 keyring: _uid.0

@dnschneid
Copy link
Owner

My concern is that they may be unlinked for a reason (ARC++ or multi-profile is usually a good guess).

What shows up if you print out the keyrings right after unwrapping the keys for the chroot? Can we somehow only make those keys accessible where they need to be?

@drinkcat
Copy link
Collaborator

Without this patch:

$ sudo keyctl list @u
2 keys in keyring:
890533281: --alswrv     0     0 user: 8c7159f15c183071
772187242: --alswrv     0     0 user: d3f79ffd75f42358
$ sudo keyctl list @s
1 key in keyring:
308501995: --alswrv     0     0 keyring: dircrypt

(then mount fails and keys are immediately cleared from @u)

With this patch:

$ sudo keyctl list @u
2 keys in keyring:
338803567: --alswrv     0     0 user: 8c7159f15c183071
1044269498: --alswrv     0     0 user: d3f79ffd75f42358
$ sudo keyctl list @s
2 keys in keyring:
308501995: --alswrv     0     0 keyring: dircrypt
346063147: --alswrv     0 65534 keyring: _uid.0

Then, on umount, @u is cleared, but the link in @s stays.

@hshmt
Copy link

hshmt commented Jun 15, 2017

Thank you for the patch!

To use ext4 encryption on Chrome OS, we are applying a local patch to upstart to share a single session keyring among all user processes.
https://chromium-review.googlesource.com/c/331340/
This means, linking the root's user keyring to the session keyring results in exposing it to non-root users too, and I think it's unsafe.

So, instead of linking the root's user keyring to the current session keyring, how about setting up a new session keyring?

keyctl session # Set up a new session keyring
keyctl link @u @s # Link the user keyring to the new session keyring (this is needed because ecryptfs util functions add keys to the user keyring)
keyctl link <the original session keyring> @s # Link the original session keyring to the new session keyring (this is needed to access user's data on new devices if it's encrypted with ext4 encryption instead of ecryptfs)

The resulting keyring will look like this:

# keyctl show
Session Keyring
 860524043 --alswrv      0     0  keyring: _ses
1065162552 --alswrv      0 65534   \_ keyring: _uid.0
 798736045 --alswrv      0     0   \_ keyring: _ses
 220661875 --alswrv      0     0       \_ keyring: dircrypt

@@ -247,6 +247,8 @@ $passphrase" | ecryptfs-rewrap-passphrase "$wrappedfnek" -
error 1 "Failed to decrypt $NAME."
fi

keyctl link @u @s
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add keyctl new_session >/dev/null before this line, and I think that'll solve the concern. No need to link the current keyring here (we don't need it in this process).

@drinkcat
Copy link
Collaborator

@hshmt Thanks for the insight!

From offline discussion, we don't even need to link the old _ses keyring in the new session (mount-chroot just needs to have the keys for ecryptfs). Also, keyctl new_session is what we want here, as keyctl session spawns a new subshell.

drinkcat added a commit to drinkcat/chroagh that referenced this pull request Jun 16, 2017
See dnschneid#3246 for a more complete explanation of the issue.
@dnschneid
Copy link
Owner

@smerrill thanks for the initial investigation and fix! Continuing in #3278

@dnschneid dnschneid closed this Jun 16, 2017
drinkcat added a commit that referenced this pull request Jun 17, 2017
See #3246 for a more complete explanation of the issue.
@kapilhp
Copy link

kapilhp commented Nov 23, 2017

@drinkcat

From offline discussion, we don't even need to link the old _ses keyring in the new session (mount-chroot just needs to have the keys for ecryptfs). Also, keyctl new_session is what we want here, as keyctl session spawns a new subshell.

I am not sure that this solution is the right one for the reasons given below:

  1. keyctl new_session does link the old _ses keyring in the new session. Thus the crouton chroot does have access to the old keyring (including ext4 encryption keys) as can be seen from the output of keyctl show @s.

  2. It is more complicated for the user of the chroot to add keys to the keyring. For this reason if (say) a user (within crouton) tries to create an encryptfs directory on an external device, then that user needs to jump through hoops as explained in my 21 Nov update to my message on 19 Oct in Encryption Broken with ChromeOS Update #3461.

I will try to find a "better" fix once I find time to debug the process!

Kapil.

Update: First of all let me eat humble pie and admit that the solution implemented does indeed seem to be correct.

  1. The point (1) above is not correct. The keyctl new_session does not link the old keyring in the new session. The earlier keyring becomes visible because this new session is closed when mount-chroot exits, thus we get back the earlier session and its keys. (One can find this out by inserting keyctl show @s at various points in the scripts.)

  2. The point (2) above continues to be a niggle. However, this may be due to the fact that the crouton chroot does not initiate a session the same way as pam does. In particular, the stuff carried out by pam_keyinit.so is not done by crouton. Why it worked in the past is still a mystery to me (and will probably remain unresolved since I have no way of running ChromiumOS ver 59 to check!).

@dnschneid
Copy link
Owner

I'm really glad this all makes sense to someone here. Thanks for looking into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants