Re: Why the auxiliary cipher in gss_krb5_crypto.c?
On Fri, Dec 04, 2020 at 02:59:35PM +, David Howells wrote: > Hi Chuck, Bruce, > > Why is gss_krb5_crypto.c using an auxiliary cipher? For reference, the > gss_krb5_aes_encrypt() code looks like the attached. > > From what I can tell, in AES mode, the difference between the main cipher and > the auxiliary cipher is that the latter is "cbc(aes)" whereas the former is > "cts(cbc(aes))" - but they have the same key. > > Reading up on CTS, I'm guessing the reason it's like this is that CTS is the > same as the non-CTS, except for the last two blocks, but the non-CTS one is > more efficient. The reason to use CTS is if you don't want to expand the size of the cipher text to the cipher block size. e.g., if you have a 53 byte plaintext, and you can't afford to let the ciphertext be 56 bytes, the cryptographic engineer will reach for CTS instead of CBC. So that probably explains the explanation to use CTS (and it's required by the spec in any case). As far as why CBC is being used instead of CTS, the only reason I can think of is the one you posted. Perhaps there was some hardware or software configureation where cbc(aes) was hardware accelerated, and cts(cbc(aes)) would not be? In any case, using cbc(aes) for all but the last two blocks, and using cts(cbc(aes)) for the last two blocks, is identical to using cts(cbc(aes)) for the whole encryption. So the only reason to do this in the more complex way would be because for performance reasons. - Ted
Re: drivers/char/random.c needs a (new) maintainer
On Mon, Nov 30, 2020 at 04:15:23PM +0100, Jason A. Donenfeld wrote: > I am willing to maintain random.c and have intentions to have a > formally verified RNG. I've mentioned this to Ted before. > > But I think Ted's reluctance to not accept the recent patches sent to > this list is mostly justified, and I have no desire to see us rush > into replacing random.c with something suboptimal or FIPSy. Being a maintainer is not about *accepting* patches, it's about *reviewing* them. I do plan to make time to catch up on reviewing patches this cycle. One thing that would help me is if folks (especially Jason, if you would) could start with a detailed review of Nicolai's patches. His incremental approach is I believe the best one from a review perspective, and certainly his cleanup patches are ones which I would expect are no-brainers. - Ted
Re: [DISCUSSION PATCH 00/41] random: possible ways towards NIST SP800-90B compliance
On Fri, Oct 02, 2020 at 03:39:35PM +, Van Leeuwen, Pascal wrote: > > Then your company can not contribute in Linux kernel development, as > > this is obviously not allowed by such a footer. > > > Interesting, this has never been raised as a problem until today ... > Going back through my mail archive, it looks like they started automatically > adding that some > 3 months ago. Not that they informed anyone about that, it just silently > happened. So use a private e-mail address (e.g., at fastmail.fm if you don't want to run your mail server) and then tunnel out SMTP requests using ssh. It's not hard. :-) I've worked a multiple $BIG_COMPANY's, and I've been doing this for decades. It's also helpful when I need to send e-mails from conference networks from my laptop - Ted
Re: [PATCH] random: initialize ChaCha20 constants with correct endianness
On Tue, Sep 15, 2020 at 09:50:13PM -0700, Eric Biggers wrote: > From: Eric Biggers > > On big endian CPUs, the ChaCha20-based CRNG is using the wrong > endianness for the ChaCha20 constants. > > This doesn't matter cryptographically, but technically it means it's not > ChaCha20 anymore. Fix it to always use the standard constants. I'll note that we're not technically ChaCha20 in terms of how we handle the IV. ChaCha20 is defined as having a 96 bit IV and a 32-bit counter. The counter is "usually initialized to be zero or one" (per RFC 7539) and the counter is defined to be Little Endian. We're currently not bothering to deal with Endian conversions with the counter, and we're using a 64-bit counter, instead of a 32-bit counter. We also twiddle 32-bits of the state (crng->state[14]) by XOR'ing it with RDRAND if available at each round, which is also a spec violation. WE also initialize the counter to be a random value, using the input_pool or the primary crng state (if we are initializing the secondary state), but given that the specification says _usually_ zero or one, that's not an out-and-out spec violation. As far as the other deviations / "spec violations" from ChaCha-20 are concerned... I'm "sorry not sorry". :-) I have no objections to changing things so that the first 4 words of the crng state are more ChaCha-20-like, on the theory that most of the cryptoanlysis work (both positive and negative) have been done with the little-endian version of "expand 32-byte k". I don't think it really makes a difference, either positively or negatively. But technically we'd *still* not be using ChaCha20. We could say that we're using the ChaCha20 block function, regardless. Cheers, - Ted
Re: [PATCH] fscrypt: add support for IV_INO_LBLK_32 policies
On Fri, May 15, 2020 at 01:41:41PM -0700, Eric Biggers wrote: > From: Eric Biggers > > The eMMC inline crypto standard will only specify 32 DUN bits (a.k.a. IV > bits), unlike UFS's 64. IV_INO_LBLK_64 is therefore not applicable, but > an encryption format which uses one key per policy and permits the > moving of encrypted file contents (as f2fs's garbage collector requires) > is still desirable. > > To support such hardware, add a new encryption format IV_INO_LBLK_32 > that makes the best use of the 32 bits: the IV is set to > 'SipHash-2-4(inode_number) + file_logical_block_number mod 2^32', where > the SipHash key is derived from the fscrypt master key. We hash only > the inode number and not also the block number, because we need to > maintain contiguity of DUNs to merge bios. > > Unlike with IV_INO_LBLK_64, with this format IV reuse is possible; this > is unavoidable given the size of the DUN. This means this format should > only be used where the requirements of the first paragraph apply. > However, the hash spreads out the IVs in the whole usable range, and the > use of a keyed hash makes it difficult for an attacker to determine > which files use which IVs. > > Besides the above differences, this flag works like IV_INO_LBLK_64 in > that on ext4 it is only allowed if the stable_inodes feature has been > enabled to prevent inode numbers and the filesystem UUID from changing. > > Signed-off-by: Eric Biggers Reviewed-by: Theodore Ts'o I kind of wish we had Kunit tests with test vectors, but that's for another commit I think. - Ted
Re: [PATCH] hw_random: don't wait on add_early_randomness()
On Tue, Sep 17, 2019 at 11:54:50AM +0200, Laurent Vivier wrote: > add_early_randomness() is called by hwrng_register() when the > hardware is added. If this hardware and its module are present > at boot, and if there is no data available the boot hangs until > data are available and can't be interrupted. > > To avoid that, call rng_get_data() in non-blocking mode (wait=0) > from add_early_randomness(). > > Signed-off-by: Laurent Vivier Looks good, you can add: Reviewed-by: Theodore Ts'o
Re: [PATCH 5/9] block: support diskcipher
On Tue, Aug 27, 2019 at 05:33:33PM +0900, boojin.kim wrote: > > Dear Satya. > Keyslot manager is a good solution for ICE. And probably no issue for FMP. > But, I think it's complicated for FMP because FMP doesn't need > any keyslot control. Hi Boojin, I think the important thing to realize here is that there are a large number of hardware devices for which the keyslot manager *is* needed. And from the upstream kernel's perspective, supporting two different schemes for supporting the inline encryption feature is more complexity than just supporting one which is general enough to support a wider variety of hardware devices. If you want somethig which is only good for the hardware platform you are charged to support, that's fine if it's only going to be in a Samsung-specific kernel. But if your goal is to get something that works upstream, especially if it requires changes in core layers of the kernel, it's important that it's general enough to support most, if not all, if the hardware devices in the industry. Regards, - Ted
Re: [PATCH v8 20/20] fscrypt: document the new ioctls and policy version
On Mon, Aug 05, 2019 at 09:25:21AM -0700, Eric Biggers wrote: > From: Eric Biggers > > Update the fscrypt documentation file to catch up to all the latest > changes, including the new ioctls to manage master encryption keys in > the filesystem-level keyring and the support for v2 encryption policies. > > Signed-off-by: Eric Biggers Looks good, thanks! Feel free to add: Reviewed-by: Theodore Ts'o
Re: [PATCH v8 13/20] fscrypt: v2 encryption policy support
On Mon, Aug 05, 2019 at 09:25:14AM -0700, Eric Biggers wrote: > From: Eric Biggers > > Add a new fscrypt policy version, "v2". It has the following changes > from the original policy version, which we call "v1" (*): > > - Master keys (the user-provided encryption keys) are only ever used as > input to HKDF-SHA512. This is more flexible and less error-prone, and > it avoids the quirks and limitations of the AES-128-ECB based KDF. > Three classes of cryptographically isolated subkeys are defined: > > - Per-file keys, like used in v1 policies except for the new KDF. > > - Per-mode keys. These implement the semantics of the DIRECT_KEY > flag, which for v1 policies made the master key be used directly. > These are also planned to be used for inline encryption when > support for it is added. > > - Key identifiers (see below). > > - Each master key is identified by a 16-byte master_key_identifier, > which is derived from the key itself using HKDF-SHA512. This prevents > users from associating the wrong key with an encrypted file or > directory. This was easily possible with v1 policies, which > identified the key by an arbitrary 8-byte master_key_descriptor. > > - The key must be provided in the filesystem-level keyring, not in a > process-subscribed keyring. > > The following UAPI additions are made: > > - The existing ioctl FS_IOC_SET_ENCRYPTION_POLICY can now be passed a > fscrypt_policy_v2 to set a v2 encryption policy. It's disambiguated > from fscrypt_policy/fscrypt_policy_v1 by the version code prefix. > > - A new ioctl FS_IOC_GET_ENCRYPTION_POLICY_EX is added. It allows > getting the v1 or v2 encryption policy of an encrypted file or > directory. The existing FS_IOC_GET_ENCRYPTION_POLICY ioctl could not > be used because it did not have a way for userspace to indicate which > policy structure is expected. The new ioctl includes a size field, so > it is extensible to future fscrypt policy versions. > > - The ioctls FS_IOC_ADD_ENCRYPTION_KEY, FS_IOC_REMOVE_ENCRYPTION_KEY, > and FS_IOC_GET_ENCRYPTION_KEY_STATUS now support managing keys for v2 > encryption policies. Such keys are kept logically separate from keys > for v1 encryption policies, and are identified by 'identifier' rather > than by 'descriptor'. The 'identifier' need not be provided when > adding a key, since the kernel will calculate it anyway. > > This patch temporarily keeps adding/removing v2 policy keys behind the > same permission check done for adding/removing v1 policy keys: > capable(CAP_SYS_ADMIN). However, the next patch will carefully take > advantage of the cryptographically secure master_key_identifier to allow > non-root users to add/remove v2 policy keys, thus providing a full > replacement for v1 policies. > > (*) Actually, in the API fscrypt_policy::version is 0 while on-disk > fscrypt_context::format is 1. But I believe it makes the most sense > to advance both to '2' to have them be in sync, and to consider the > numbering to start at 1 except for the API quirk. > > Signed-off-by: Eric Biggers Looks good, feel free to add: Reviewed-by: Theodore Ts'o
Re: [PATCH v8 15/20] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS ioctl
On Mon, Aug 05, 2019 at 09:25:16AM -0700, Eric Biggers wrote: > From: Eric Biggers > > Add a root-only variant of the FS_IOC_REMOVE_ENCRYPTION_KEY ioctl which > removes all users' claims of the key, not just the current user's claim. > I.e., it always removes the key itself, no matter how many users have > added it. > > This is useful for forcing a directory to be locked, without having to > figure out which user ID(s) the key was added under. This is planned to > be used by a command like 'sudo fscrypt lock DIR --all-users' in the > fscrypt userspace tool (http://github.com/google/fscrypt). > > Signed-off-by: Eric Biggers Looks good, thanks. Feel free to add: Reviewed-by: Theodore Ts'o
Re: [PATCH v8 14/20] fscrypt: allow unprivileged users to add/remove keys for v2 policies
On Mon, Aug 05, 2019 at 09:25:15AM -0700, Eric Biggers wrote: > From: Eric Biggers > > Allow the FS_IOC_ADD_ENCRYPTION_KEY and FS_IOC_REMOVE_ENCRYPTION_KEY > ioctls to be used by non-root users to add and remove encryption keys > from the filesystem-level crypto keyrings, subject to limitations. > > Motivation: while privileged fscrypt key management is sufficient for > some users (e.g. Android and Chromium OS, where a privileged process > manages all keys), the old API by design also allows non-root users to > set up and use encrypted directories, and we don't want to regress on > that. Especially, we don't want to force users to continue using the > old API, running into the visibility mismatch between files and keyrings > and being unable to "lock" encrypted directories. > > Intuitively, the ioctls have to be privileged since they manipulate > filesystem-level state. However, it's actually safe to make them > unprivileged if we very carefully enforce some specific limitations. > > First, each key must be identified by a cryptographic hash so that a > user can't add the wrong key for another user's files. For v2 > encryption policies, we use the key_identifier for this. v1 policies > don't have this, so managing keys for them remains privileged. > > Second, each key a user adds is charged to their quota for the keyrings > service. Thus, a user can't exhaust memory by adding a huge number of > keys. By default each non-root user is allowed up to 200 keys; this can > be changed using the existing sysctl 'kernel.keys.maxkeys'. > > Third, if multiple users add the same key, we keep track of those users > of the key (of which there remains a single copy), and won't really > remove the key, i.e. "lock" the encrypted files, until all those users > have removed it. This prevents denial of service attacks that would be > possible under simpler schemes, such allowing the first user who added a > key to remove it -- since that could be a malicious user who has > compromised the key. Of course, encryption keys should be kept secret, > but the idea is that using encryption should never be *less* secure than > not using encryption, even if your key was compromised. > > We tolerate that a user will be unable to really remove a key, i.e. > unable to "lock" their encrypted files, if another user has added the > same key. But in a sense, this is actually a good thing because it will > avoid providing a false notion of security where a key appears to have > been removed when actually it's still in memory, available to any > attacker who compromises the operating system kernel. > > Signed-off-by: Eric Biggers Looks good. I'd probably would have used either "mk_secret_sem" or "mk->mk_secret_sem" in the comments, instead of "->mk_securet_sem", but that's just a personal style preference. Since you consistently used the latter, I assume that's a deliberate choice, which is fine. Feel free to add: Reviewed-by: Theodore Ts'o
Re: [PATCH v8 10/20] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl
> + /* Some inodes still reference this key; try to evict them. */ > + if (try_to_lock_encrypted_files(sb, mk) != 0) > + status_flags |= > + FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY; > + } try_to_lock_encrypted_files() can return other errors besides -EBUSY; in particular sync_filesystem() can return other errors, such as -EIO or -EFSCORUPTED. In that case, I think we're better off returning the relevant status code back to the user. We will have already wiped the master key, but this situation will only happen in exceptional conditions (e.g., user has ejected the sdcard, etc.), so it's not worth it to try to undo the master key wipe to try to restore things to the pre-ioctl execution state. So I think we should capture the return code from try_to_lock_encrypted_files, and if it is EBUSY, we can set FILES_BUSY flag and return success. Otherwise, we should return the error. If you agree, please fix that up and then feel free to add: Reviewed-by: Theodore Ts'o - Ted
Re: [PATCH v8 08/20] fscrypt: rename keyinfo.c to keysetup.c
On Mon, Aug 05, 2019 at 09:25:09AM -0700, Eric Biggers wrote: > From: Eric Biggers > > Rename keyinfo.c to keysetup.c since this better describes what the file > does (sets up the key), and it matches the new file keysetup_v1.c. > > Signed-off-by: Eric Biggers Looks good, you can add: Reviewed-by: Theodore Ts'o
Re: [PATCH v8 07/20] fscrypt: move v1 policy key setup to keysetup_v1.c
On Mon, Aug 05, 2019 at 09:25:08AM -0700, Eric Biggers wrote: > From: Eric Biggers > > In preparation for introducing v2 encryption policies which will find > and derive encryption keys differently from the current v1 encryption > policies, move the v1 policy-specific key setup code from keyinfo.c into > keysetup_v1.c. > > Signed-off-by: Eric Biggers Looks good, you can add Reviewed-by: Theodore Ts'o
Re: [PATCH v8 06/20] fscrypt: refactor key setup code in preparation for v2 policies
On Mon, Aug 05, 2019 at 09:25:07AM -0700, Eric Biggers wrote: > From: Eric Biggers > > Do some more refactoring of the key setup code, in preparation for > introducing a filesystem-level keyring and v2 encryption policies: > > - Now that ci_inode exists, don't pass around the inode unnecessarily. > > - Define a function setup_file_encryption_key() which handles the crypto > key setup given an under-construction fscrypt_info. Don't pass the > fscrypt_context, since everything is in the fscrypt_info. > [This will be extended for v2 policies and the fs-level keyring.] > > - Define a function fscrypt_set_derived_key() which sets the per-file > key, without depending on anything specific to v1 policies. > [This will also be used for v2 policies.] > > - Define a function fscrypt_setup_v1_file_key() which takes the raw > master key, thus separating finding the key from using it. > [This will also be used if the key is found in the fs-level keyring.] > > Signed-off-by: Eric Biggers Looks good, you can add: Reviewed-by: Theodore Ts'o - Ted
Re: [PATCH v8 05/20] fscrypt: rename fscrypt_master_key to fscrypt_direct_key
On Mon, Aug 05, 2019 at 09:25:06AM -0700, Eric Biggers wrote: > From: Eric Biggers > > In preparation for introducing a filesystem-level keyring which will > contain fscrypt master keys, rename the existing 'struct > fscrypt_master_key' to 'struct fscrypt_direct_key'. This is the > structure in the existing table of master keys that's maintained to > deduplicate the crypto transforms for v1 DIRECT_KEY policies. > > I've chosen to keep this table as-is rather than make it automagically > add/remove the keys to/from the filesystem-level keyring, since that > would add a lot of extra complexity to the filesystem-level keyring. > > Signed-off-by: Eric Biggers Looks good. You can add: Reviewed-by: Theodore Ts'o - Ted
Re: [PATCH v7 07/16] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl
On Thu, Aug 01, 2019 at 09:38:27PM -0700, Eric Biggers wrote:
>
> Here's a slightly updated version (I missed removing some stale text):
Apologies for the delaying in getting back. Thanks, this looks great.
- Ted
>
> Removing keys
> -
>
> Two ioctls are available for removing a key that was added by
> `FS_IOC_ADD_ENCRYPTION_KEY`_:
>
> - `FS_IOC_REMOVE_ENCRYPTION_KEY`_
> - `FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS`_
>
> These two ioctls differ only in cases where v2 policy keys are added
> or removed by non-root users.
>
> These ioctls don't work on keys that were added via the legacy
> process-subscribed keyrings mechanism.
>
> Before using these ioctls, read the `Kernel memory compromise`_
> section for a discussion of the security goals and limitations of
> these ioctls.
>
> FS_IOC_REMOVE_ENCRYPTION_KEY
>
>
> The FS_IOC_REMOVE_ENCRYPTION_KEY ioctl removes a claim to a master
> encryption key from the filesystem, and possibly removes the key
> itself. It can be executed on any file or directory on the target
> filesystem, but using the filesystem's root directory is recommended.
> It takes in a pointer to a :c:type:`struct fscrypt_remove_key_arg`,
> defined as follows::
>
> struct fscrypt_remove_key_arg {
> struct fscrypt_key_specifier key_spec;
> #define FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY 0x0001
> #define FSCRYPT_KEY_REMOVAL_STATUS_FLAG_OTHER_USERS 0x0002
> __u32 removal_status_flags; /* output */
> __u32 __reserved[5];
> };
>
> This structure must be zeroed, then initialized as follows:
>
> - The key to remove is specified by ``key_spec``:
>
> - To remove a key used by v1 encryption policies, set
> ``key_spec.type`` to FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR and fill
> in ``key_spec.u.descriptor``. To remove this type of key, the
> calling process must have the CAP_SYS_ADMIN capability in the
> initial user namespace.
>
> - To remove a key used by v2 encryption policies, set
> ``key_spec.type`` to FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER and fill
> in ``key_spec.u.identifier``.
>
> For v2 policy keys, this ioctl is usable by non-root users. However,
> to make this possible, it actually just removes the current user's
> claim to the key, undoing a single call to FS_IOC_ADD_ENCRYPTION_KEY.
> Only after all claims are removed is the key really removed.
>
> For example, if FS_IOC_ADD_ENCRYPTION_KEY was called with uid 1000,
> then the key will be "claimed" by uid 1000, and
> FS_IOC_REMOVE_ENCRYPTION_KEY will only succeed as uid 1000. Or, if
> both uids 1000 and 2000 added the key, then for each uid
> FS_IOC_REMOVE_ENCRYPTION_KEY will only remove their own claim. Only
> once *both* are removed is the key really removed. (Think of it like
> unlinking a file that may have hard links.)
>
> If FS_IOC_REMOVE_ENCRYPTION_KEY really removes the key, it will also
> try to "lock" all files that had been unlocked with the key. It won't
> lock files that are still in-use, so this ioctl is expected to be used
> in cooperation with userspace ensuring that none of the files are
> still open. However, if necessary, the ioctl can be executed again
> later to retry locking any remaining files.
>
> FS_IOC_REMOVE_ENCRYPTION_KEY returns 0 if either the key was removed
> (but may still have files remaining to be locked), the user's claim to
> the key was removed, or the key was already removed but had files
> remaining to be the locked so the ioctl retried locking them. In any
> of these cases, ``removal_status_flags`` is filled in with the
> following informational status flags:
>
> - ``FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY``: set if some file(s)
> are still in-use. Not guaranteed to be set in the case where only
> the user's claim to the key was removed.
> - ``FSCRYPT_KEY_REMOVAL_STATUS_FLAG_OTHER_USERS``: set if only the
> user's claim to the key was removed, not the key itself
>
> FS_IOC_REMOVE_ENCRYPTION_KEY can fail with the following errors:
>
> - ``EACCES``: The FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR key specifier type
> was specified, but the caller does not have the CAP_SYS_ADMIN
> capability in the initial user namespace
> - ``EINVAL``: invalid key specifier type, or reserved bits were set
> - ``ENOKEY``: the key object was not found at all, i.e. it was never
> added in the first place or was already fully removed including all
> files locked; or, the user does not have a claim to the key.
> - ``ENOTTY``: this type of filesystem does not implement encryption
> - ``EOPNOTSUPP``: the kernel was not configured with encryption
> support for this filesystem, or the filesystem superblock has not
> had encryption enabled on it
>
> FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS
> ~~
>
> FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS is exactly the same as
> `FS_IO
Re: [f2fs-dev] [PATCH v7 07/16] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl
On Wed, Jul 31, 2019 at 06:11:40PM -0700, Eric Biggers wrote: > > Well, it's either > > 1a. Remove the user's handle. > OR > 1b. Remove all users' handles. (FSCRYPT_REMOVE_KEY_FLAG_ALL_USERS) > > Then > > 2. If no handles remain, try to evict all inodes that use the key. > > By "purge all keys" do you mean step (2)? Note that it doesn't require root > by > itself; root is only required to remove other users' handles (1b). No, I was talking about 1b. I'd argue that 1a and 1b should be different ioctl. 1b requires root, and 1a doesn't. And 1a should just mean, "I don't need to use the encrypted files any more". In the PAM passphrase case, when you are just logging out, 1a is what's needed, and success is just fine. pam_session won't *care* whether or not there are other users keeping the key in use. The problem with "fscrypt lock" is that the non-privileged user sort of wants to do REMOVE_FLAG_KEY_FLAG_FOR_ALL_USERS, but they doesn't have the privileges to do it, and they are hoping that removing their own key removes it the key from the system. That to me seems to be the fundamental disconnect. The "fscrypt unlock" and "fscrypt lock" commands comes from the v1 key management, and requires root. It's the translation to unprivileged mode where "fscrypt lock" seems to have problems. > > What about having "fscrypt lock" call FS_IOC_GET_ENCRYPTION_KEY_STATUS > > and print a warning message saying, "we can't lock it because N other > > users who have registered a key". I'd argue fscrypt should do this > > regardless of whether or not FS_IOC_REMOVE_ENCRYPTION_KEY returns > > EUSERS or not. > > Shouldn't "fscrypt lock" still remove the user's handle, as opposed to refuse > to > do anything, though? So it would still need to callh > FS_IOC_REMOVE_ENCRYPTION_KEY, and could get the status from it rather than > also > needing to call FS_IOC_GET_ENCRYPTION_KEY_STATUS. > > Though, FS_IOC_GET_ENCRYPTION_KEY_STATUS would be needed if we wanted to show > the specific count of other users. So my perspective is that the ioctl's should have very clean semantics, and errors should be consistent with how the Unix system calls and error reporting. If we need to make "fscrypt lock" and "fscrypt unlock" have semantics that are more consistent with previous user interface choices, then fscrypt can use FS_IOC_GET_ENCRYPTION_KEY_STATUS to print the warning before it calls FS_IOC_REMOVE_ENCRYPTION_KEY --- with "fscrypt purge_keys" calling something like FS_IOC_REMOVE_ALL_USER_ENCRYPTION_KEYS. > > It seems to me that the EBUSY and EUSERS errors should be status bits > > which gets returned to the user in a bitfield --- and if the key has > > been removed, or the user's claim on the key's existence has been > > removed, the ioctl returns success. > > > > That way we don't have to deal with the semantic disconnect where some > > errors don't actually change system state, and other errors that *do* > > change system state (as in, the key gets removed, or the user's claim > > on the key gets removed), but still returns than error. > > > > Do you mean use a positive return value, or do you mean add an output field to > the struct passed to the ioctl? I meant adding an output field. I see EBUSY and EUSERS as status bits which *some* use cases might find useful. Other use cases, such as in the pam_keys session logout code, we won't care at *all* about those status reporting (or error codes). So if EBUSY and EUSERS are returned as errors, then it adds to complexity of those programs whichd don't care. (And even for those that do, it's still a bit more complex since they has to distinguish between EBUSY, EUSERS, or other errors --- in fact, *all* programs which use FS_IOC_REMOVE_ENCRYPTION_KEY will *have* to check for EBUSY and ESUSERS whether they care or not.) > Either way note that it doesn't really need to be a bitfield, since you can't > have both statuses at the same time. I.e. if there are still other users, we > couldn't have even gotten to checking for in-use files. That's actually an implementation detail, though, right? In theory, we could check to see if there are any in-use files, independently of whether there are any users or not. - Ted
Re: [PATCH v7 07/16] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl
On Wed, Jul 31, 2019 at 11:38:02AM -0700, Eric Biggers wrote: > > This is perhaps different from what users expect from unlink(). It's well > known > that unlink() just deletes the filename, not the file itself if it's still > open > or has other links. And unlink() by itself isn't meant for use cases where > the > file absolutely must be securely erased. But FS_IOC_REMOVE_ENCRYPTION_KEY > really is meant primarily for that sort of thing. Seems to me that part of the confusion is FS_IOC_REMOVE_ENCRYPTION_KEY does two things. One is "remove the user's handle on the key". The other is "purge all keys" (which requires root). So it does two different things with one ioctl. > To give a concrete example: my patch for the userspace tool > https://github.com/google/fscrypt adds a command 'fscrypt lock' which locks an > encrypted directory. If, say, someone runs 'fscrypt unlock' as uid 0 and then > 'fscrypt lock' as uid 1000, then FS_IOC_REMOVE_ENCRYPTION_KEY can't actually > remove the key. I need to make the tool show a proper error message in this > case. To do so, it would help to get a unique error code (e.g. EUSERS) from > FS_IOC_REMOVE_ENCRYPTION_KEY, rather than get the ambiguous error code ENOKEY > and have to call FS_IOC_GET_ENCRYPTION_KEY_STATUS to get the real status. What about having "fscrypt lock" call FS_IOC_GET_ENCRYPTION_KEY_STATUS and print a warning message saying, "we can't lock it because N other users who have registered a key". I'd argue fscrypt should do this regardless of whether or not FS_IOC_REMOVE_ENCRYPTION_KEY returns EUSERS or not. > Also, we already have the EBUSY case. This means that the ioctl removed the > master key secret itself; however, some files were still in-use, so the key > remains in the "incompletely removed" state. If we were actually going for > unlink() semantics, then for consistency this case really ought to return 0 > and > unlink the key object, and people who care about in-use files would need to > use > FS_IOC_GET_ENCRYPTION_KEY_STATUS. But most people *will* care about this, and > may even want to retry the ioctl later, which isn't something youh can do with > pure unlink() semantics. It seems to me that the EBUSY and EUSERS errors should be status bits which gets returned to the user in a bitfield --- and if the key has been removed, or the user's claim on the key's existence has been removed, the ioctl returns success. That way we don't have to deal with the semantic disconnect where some errors don't actually change system state, and other errors that *do* change system state (as in, the key gets removed, or the user's claim on the key gets removed), but still returns than error. We could also add a flag which indicates where if there are files that are still busy, or there are other users keeping a key in use, the ioctl fails hard and returns an error. At least that way we keep consistency where an error means, "nothing has changed". - Ted P.S. BTW, one of the comments which I didn't make was the documentation didn't adequately explain which error codes means, "success but with a caveat", and which errors means, "we failed and didn't do anything". But since I was arguing for changing the behavior, I decided not to complain about the documentation.
Re: [PATCH v7 15/16] ubifs: wire up new fscrypt ioctls
On Fri, Jul 26, 2019 at 03:41:40PM -0700, Eric Biggers wrote: > From: Eric Biggers > > Wire up the new ioctls for adding and removing fscrypt keys to/from the > filesystem, and the new ioctl for retrieving v2 encryption policies. > > FS_IOC_REMOVE_ENCRYPTION_KEY also required making UBIFS use > fscrypt_drop_inode(). > > For more details see Documentation/filesystems/fscrypt.rst and the > fscrypt patches that added the implementation of these ioctls. > > Signed-off-by: Eric Biggers Looks good: Reviewed-by: Theodore Ts'o - Ted
Re: [PATCH v7 06/16] fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl
On Mon, Jul 29, 2019 at 12:46:45PM -0700, Eric Biggers wrote: > > For that matter, we could just add a new ioctl which returns the file > > system's keyring id. That way an application program won't have to > > try to figure out what a file's underlying sb->s_id happens to be. > > (Especially if things like overlayfs are involved.) > > Keep in mind that the new ioctls (FS_IOC_ADD_ENCRYPTION_KEY, > FS_IOC_REMOVE_ENCRYPTION_KEY, FS_IOC_GET_ENCRYPTION_KEY_STATUS) don't take the > keyring ID as a parameter, since it's already known from the filesystem the > ioctl is executed on. So there actually isn't much that can be done with the > keyring ID. But sure, if it's needed later we can add an API to get it. Yeah, I was thinking about for testing/debugging purposes so that we could use keyctl to examine the per-file system keyring and see what keys are attached to a file system. This is only going to be usable by root, so I guess we can just try to figure it out by going through /proc/keys and searching by sb->s_id. If there are ambiguities that make this hard to do, we can add an interface to make this easier. - Ted
Re: [PATCH v7 16/16] fscrypt: document the new ioctls and policy version
On Fri, Jul 26, 2019 at 03:41:41PM -0700, Eric Biggers wrote: > +- The kernel cannot magically wipe copies of the master key(s) that > + userspace might have as well. Therefore, userspace must wipe all > + copies of the master key(s) it makes as well. Naturally, the same > + also applies to all higher levels in the key hierarchy. Userspace > + should also follow other security precautions such as mlock()ing > + memory containing keys to prevent it from being swapped out. Normally, shouldn't userspace have wiped all copies of the master key after they have called ADD_KEY? Why should they be left hanging around? Waiting until REMOVE_KEY to remove other copies of the master key seems late. > +- In general, decrypted contents and filenames in the kernel VFS > + caches are freed but not wiped. Therefore, portions thereof may be > + recoverable from freed memory, even after the corresponding key(s) > + were wiped. To partially solve this, you can set > + CONFIG_PAGE_POISONING=y in your kernel config and add page_poison=1 > + to your kernel command line. However, this has a performance cost. ... and even this won't help if you have swap configured > +v1 encryption policies have some weaknesses with respect to online > +attacks: > + > +- There is no verification that the provided master key is correct. > + Consequently, malicious users can associate the wrong key with > + encrypted files, even files to which they have only read-only > + access. Yes, but they won't be able to trick other users into using that incorrect key. With the old interface, it gets written into the user's session keyring, which won't get used by another user. And with the newer interface, only root is allowed to set v1 key. > +Master keys should be pseudorandom, i.e. indistinguishable from random > +bytestrings of the same length. This implies that users **must not** > +directly use a password as a master key, zero-pad a shorter key, or > +repeat a shorter key. These paragraphs starts a bit funny, since we first say "should" in the first sentence, and then it's followed up by "**must not**" in the second sentence. Basically, they *could* do this, but it would just weaken the security of the system significantly. At the very least, we should explain the basis of the recommendation. > +The KDF used for a particular master key differs depending on whether > +the key is used for v1 encryption policies or for v2 encryption > +policies. Users **must not** use the same key for both v1 and v2 > +encryption policies. "Must not" seems a bit strong. If they do, and a v1 per-file key and nonce leaks out, then the encryption key will be compromised. So the strength of the key will be limited by the weaknesses of the v1 scheme. But it's not like using a that was originally meant for v1, and then using it for v2, causes any additional weakness. Right? - Ted
Re: [PATCH v7 13/16] ext4: wire up new fscrypt ioctls
On Fri, Jul 26, 2019 at 03:41:38PM -0700, Eric Biggers wrote: > From: Eric Biggers > > Wire up the new ioctls for adding and removing fscrypt keys to/from the > filesystem, and the new ioctl for retrieving v2 encryption policies. > > FS_IOC_REMOVE_ENCRYPTION_KEY also required making ext4_drop_inode() call > fscrypt_drop_inode(). > > For more details see Documentation/filesystems/fscrypt.rst and the > fscrypt patches that added the implementation of these ioctls. > > Signed-off-by: Eric Biggers Looks good, feel free to add: Reviewed-by: Theodore Ts'o - Ted
Re: [PATCH v7 12/16] fscrypt: require that key be added when setting a v2 encryption policy
On Fri, Jul 26, 2019 at 03:41:37PM -0700, Eric Biggers wrote: > From: Eric Biggers > > By looking up the master keys in a filesystem-level keyring rather than > in the calling processes' key hierarchy, it becomes possible for a user > to set an encryption policy which refers to some key they don't actually > know, then encrypt their files using that key. Cryptographically this > isn't much of a problem, but the semantics of this would be a bit weird. > Thus, enforce that a v2 encryption policy can only be set if the user > has previously added the key, or has capable(CAP_FOWNER). > > We tolerate that this problem will continue to exist for v1 encryption > policies, however; there is no way around that. > > Signed-off-by: Eric Biggers Looks good, feel free to add: Reviewed-by: Theodore Ts'o - Ted
Re: [PATCH v7 11/16] fscrypt: allow unprivileged users to add/remove keys for v2 policies
On Fri, Jul 26, 2019 at 03:41:36PM -0700, Eric Biggers wrote:
> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> index 56e085c2ed8c6..307533d4d7c51 100644
> --- a/fs/crypto/keyring.c
> +++ b/fs/crypto/keyring.c
> + if (mk->mk_users->keys.nr_leaves_on_tree != 0) {
> + /*
> + * Other users have still added the key too. We removed
> + * the current user's usage of the key if there was one,
> + * but we still can't remove the key itself.
> + */
> + err = -EUSERS;
> + up_write(&key->sem);
> + goto out_put_key;
I commented about this on an earlier patch, but I'm not convinced we
should be returning EUSERS here. Returning success might be a better
choice.
- Ted
Re: [PATCH v7 10/16] fscrypt: v2 encryption policy support
On Fri, Jul 26, 2019 at 03:41:35PM -0700, Eric Biggers wrote:
> @@ -319,6 +329,31 @@ int fscrypt_ioctl_add_key(struct file *filp, void __user
> *_uarg)
> if (!capable(CAP_SYS_ADMIN))
> goto out_wipe_secret;
>
> + if (arg.key_spec.type != FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR) {
This should be "== FSCRYPT_KEY_SPEC_TYPE_INDENTIFIER" instead. That's
because you use the identifier part of the union:
> + /* Calculate the key identifier and return it to userspace. */
> + err = fscrypt_hkdf_expand(&secret.hkdf,
> + HKDF_CONTEXT_KEY_IDENTIFIER,
> + NULL, 0, arg.key_spec.u.identifier,
If we ever add a new key specifier type, and alternative in the union,
this is going to come back to bite us.
> + if (policy->version == FSCRYPT_POLICY_V1) {
> + /*
> + * The original encryption policy version provided no way of
> + * verifying that the correct master key was supplied, which was
> + * insecure in scenarios where multiple users have access to the
> + * same encrypted files (even just read-only access).
Which scenario do you have in mind? With read-only access, Alice can
fetch the encryption policy for a directory, and introduce a key with
the same descriptor, but the "wrong" key, but that's only going to
affect Alice's use of the key. It won't affect what key is used by
Bob, since Alice doesn't have write access to Bob's keyrings.
If what you mean is the risk when there is a single global
filesystem-specific keyring, where Alice could introduce a "wrong" key
identified with a specific descriptor, then sure, Alice could trick
Bob into encrypting his data with the wrong key (one known to Alice).
But we don't allow keys usable by V1 policies to be used in the
filesystem-specific keyring, do we?
- Ted
Re: [PATCH v7 09/16] fscrypt: add an HKDF-SHA512 implementation
On Fri, Jul 26, 2019 at 03:41:34PM -0700, Eric Biggers wrote: > From: Eric Biggers > > Add an implementation of HKDF (RFC 5869) to fscrypt, for the purpose of > deriving additional key material from the fscrypt master keys for v2 > encryption policies. HKDF is a key derivation function built on top of > HMAC. We choose SHA-512 for the underlying unkeyed hash, and use an > "hmac(sha512)" transform allocated from the crypto API. > > We'll be using this to replace the AES-ECB based KDF currently used to > derive the per-file encryption keys. While the AES-ECB based KDF is > believed to meet the original security requirements, it is nonstandard > and has problems that don't exist in modern KDFs such as HKDF: > > 1. It's reversible. Given a derived key and nonce, an attacker can >easily compute the master key. This is okay if the master key and >derived keys are equally hard to compromise, but now we'd like to be >more robust against threats such as a derived key being compromised >through a timing attack, or a derived key for an in-use file being >compromised after the master key has already been removed. > > 2. It doesn't evenly distribute the entropy from the master key; each 16 >input bytes only affects the corresponding 16 output bytes. > > 3. It isn't easily extensible to deriving other values or keys, such as >a public hash for securely identifying the key, or per-mode keys. >Per-mode keys will be immediately useful for Adiantum encryption, for >which fscrypt currently uses the master key directly, introducing >unnecessary usage constraints. Per-mode keys will also be useful for >hardware inline encryption, which is currently being worked on. > > HKDF solves all the above problems. > > Signed-off-by: Eric Biggers Unless I missed something there's nothing here which is fscrypt specific. Granted that it's somewhat unlikely that someone would want to implement (the very bloated) IKE from IPSEC in the kernel, I wonder if there might be other users of HKDF, and whether this would be better placed in lib/ or crypto/ instead of fs/crypto? Other than that, looks good. Feel free to add: Reviewed-by: Theodore Ts'o
Re: [PATCH v7 08/16] fscrypt: add FS_IOC_GET_ENCRYPTION_KEY_STATUS ioctl
On Fri, Jul 26, 2019 at 03:41:33PM -0700, Eric Biggers wrote: > From: Eric Biggers > > Add a new fscrypt ioctl, FS_IOC_GET_ENCRYPTION_KEY_STATUS. Given a key > specified by 'struct fscrypt_key_specifier' (the same way a key is > specified for the other fscrypt key management ioctls), it returns > status information in a 'struct fscrypt_get_key_status_arg'. > > The main motivation for this is that applications need to be able to > check whether an encrypted directory is "unlocked" or not, so that they > can add the key if it is not, and avoid adding the key (which may > involve prompting the user for a passphrase) if it already is. > > It's possible to use some workarounds such as checking whether opening a > regular file fails with ENOKEY, or checking whether the filenames "look > like gibberish" or not. However, no workaround is usable in all cases. > > Like the other key management ioctls, the keyrings syscalls may seem at > first to be a good fit for this. Unfortunately, they are not. Even if > we exposed the keyring ID of the ->s_master_keys keyring and gave > everyone Search permission on it (note: currently the keyrings > permission system would also allow everyone to "invalidate" the keyring > too), the fscrypt keys have an additional state that doesn't map cleanly > to the keyrings API: the secret can be removed, but we can be still > tracking the files that were using the key, and the removal can be > re-attempted or the secret added again. > > After later patches, some applications will also need a way to determine > whether a key was added by the current user vs. by some other user. > Reserved fields are included in fscrypt_get_key_status_arg for this and > other future extensions. > > Signed-off-by: Eric Biggers Looks good, feel free to add: Reviewed-by: Theodore Ts'o
Re: [PATCH v7 07/16] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl
On Fri, Jul 26, 2019 at 03:41:32PM -0700, Eric Biggers wrote: > + fscrypt_warn(NULL, > + "%s: %zu inodes still busy after removing key with > description %*phN, including ino %lu (%s)", nit: s/inodes/inode(s)/ > + > +/* > + * Try to remove an fscrypt master encryption key. If other users have also > + * added the key, we'll remove the current user's usage of the key, then > return > + * -EUSERS. Otherwise we'll continue on and try to actually remove the key. Nit: this should be moved to patch #11 Also, perror(EUSERS) will display "Too many users" which is going to be confusing. I understand why you chose this; we would like to distinguish between there are still inodes using this key, and there are other users using this key. Do we really need to return EUSERS in this case? It's actually not an *error* that other users are using the key. After all, the unlink(2) system call doesn't return an advisory error when you delete a file which has other hard links. And an application which does care about this detail can always call FS_IOC_ENCRYPTION_KEY_STATUS() and check user_count. Other than these nits, looks good. Feel free to add: Reviewed-by: Theodore Ts'o - Ted
Re: [PATCH v7 06/16] fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl
On Fri, Jul 26, 2019 at 03:41:31PM -0700, Eric Biggers wrote:
> From: Eric Biggers
>
> Add a new fscrypt ioctl, FS_IOC_ADD_ENCRYPTION_KEY. This ioctl adds an
> encryption key to the filesystem's fscrypt keyring ->s_master_keys,
> making any files encrypted with that key appear "unlocked".
Note: it think it's going to be useful to make the keyring id
available someplace like /sys/fs///keyring, or preferably
in the new fsinfo system call. Yes, the system administrator can paw
through /proc/keys and try to figure it out, but it will be nicer if
there's a direct way to do that.
For that matter, we could just add a new ioctl which returns the file
system's keyring id. That way an application program won't have to
try to figure out what a file's underlying sb->s_id happens to be.
(Especially if things like overlayfs are involved.)
> diff --git a/include/uapi/linux/fscrypt.h b/include/uapi/linux/fscrypt.h
> index 29a945d165def..93d6eabaa7de4 100644
> --- a/include/uapi/linux/fscrypt.h
> +++ b/include/uapi/linux/fscrypt.h
> +
> +struct fscrypt_key_specifier {
> +#define FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR 1
> + __u32 type;
> + __u32 __reserved;
Can you move the definition of FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR
outside of the structure definition, and then add a comment about what
is a "descriptor" key spec? (And then in a later patch, please add a
comment about what is an "identifier" key type.) There's an
explanation in Documentation/filesystems/fscrypt.rst, I know, but a
one or two line comment plus a pointer to
Documentation/filesystems/fscrypt.rst in the header file would be
really helpful.
Otherwise, it looks good. Feel free to add:
Reviewed-by: Theodore Ts'o
- Ted
Re: [PATCH v7 05/16] fscrypt: refactor v1 policy key setup into keysetup_legacy.c
On Fri, Jul 26, 2019 at 03:41:30PM -0700, Eric Biggers wrote: > From: Eric Biggers > > In preparation for introducing v2 encryption policies which will find > and derive encryption keys differently from the current v1 encryption > policies, refactor the v1 policy-specific key setup code from keyinfo.c > into keysetup_legacy.c. Then rename keyinfo.c to keysetup.c. I'd use keysetup_v1.c, myself. We can hope that we've gotten it right with v2 and we'll never need to do another version, but *something* is going to come up eventually which will require a v3 keysetup , whether it's post-quantuum cryptography or something else we can't anticipate right now. For an example of the confusion that can result, one good example is in the fs/quota subsystem, where QFMT_VFS_OLD, QFMT_VFS_V0, and QFMT_VFS_V1 maps to quota_v1 and quota_v2 in an amusing and non-obvious way. (Go ahead, try to guess before you go look at the code. :-) Other than that, looks good. We can always move code around or rename files in the future, so I'm not going to insist on doing it now (but it would be my preference). Reviewed-by: Theodore Ts'o - Ted
Re: [PATCH v7 04/16] fscrypt: add ->ci_inode to fscrypt_info
On Fri, Jul 26, 2019 at 03:41:29PM -0700, Eric Biggers wrote: > From: Eric Biggers > > Add an inode back-pointer to 'struct fscrypt_info', such that > inode->i_crypt_info->ci_inode == inode. > > This will be useful for: > > 1. Evicting the inodes when a fscrypt key is removed, since we'll track >the inodes using a given key by linking their fscrypt_infos together, >rather than the inodes directly. This avoids bloating 'struct inode' >with a new list_head. > > 2. Simplifying the per-file key setup, since the inode pointer won't >have to be passed around everywhere just in case something goes wrong >and it's needed for fscrypt_warn(). > > Signed-off-by: Eric Biggers Looks good, feel free to add: Reviewed-by: Theodore Ts'o
Re: [PATCH v7 01/16] fs, fscrypt: move uapi definitions to new header
On Fri, Jul 26, 2019 at 03:41:26PM -0700, Eric Biggers wrote: > From: Eric Biggers > > More fscrypt definitions are being added, and we shouldn't use a > disproportionate amount of space in for fscrypt stuff. > So move the fscrypt definitions to a new header . > > For source compatibility with existing userspace programs, > still includes the new header. > > Signed-off-by: Eric Biggers Looks good, feel free to add: Reviewed-by: Theodore Ts'o
Re: [PATCH v2] Allow hwrng to initialize crng.
On Thu, Dec 13, 2018 at 10:48:07AM +0100, Ard Biesheuvel wrote:
> > @@ -64,13 +65,19 @@ static size_t rng_buffer_size(void)
> > static void add_early_randomness(struct hwrng *rng)
> > {
> > int bytes_read;
> > - size_t size = min_t(size_t, 16, rng_buffer_size());
> > + /* Read enough to initialize crng. */
> > + size_t size = min_t(size_t,
> > + 2*CHACHA20_KEY_SIZE,
>
> This should be as symbolic constant that retains its meaning even if
> we move away from ChaCha20 or modify the current implementation
Also, rng_buffer_size() could be less than 2*hCHACHA20_KEY_SIZE, at
which point your goal wouldn't be realized. What I'd recommend is to
keep the line:
size_t size = min_t(size_t, 16, rng_buffer_size());
But to loop until rng_is_initialized() returns true or bytes_read is
0. If you want to be paranoid, you could also break out of the loop
it isn't initialized after, say, 8 times through the loop.
Cheers,
- Ted
Re: [PATCH] fscrypt: remove CRYPTO_CTR dependency
On Tue, Dec 04, 2018 at 03:45:07PM -0800, Eric Biggers wrote:
> On Thu, Sep 06, 2018 at 12:43:41PM +0200, Ard Biesheuvel wrote:
> > On 5 September 2018 at 21:24, Eric Biggers wrote:
> > > From: Eric Biggers
> > >
> > > fscrypt doesn't use the CTR mode of operation for anything, so there's
> > > no need to select CRYPTO_CTR. It was added by commit 71dea01ea2ed
> > > ("ext4 crypto: require CONFIG_CRYPTO_CTR if ext4 encryption is
> > > enabled"). But, I've been unable to identify the arm64 crypto bug it
> > > was supposedly working around.
> > >
> > > I suspect the issue was seen only on some old Android device kernel
> > > (circa 3.10?). So if the fix wasn't mistaken, the real bug is probably
> > > already fixed. Or maybe it was actually a bug in a non-upstream crypto
> > > driver.
> > >
> > > So, remove the dependency. If it turns out there's actually still a
> > > bug, we'll fix it properly.
> > >
> > > Signed-off-by: Eric Biggers
> >
> > Acked-by: Ard Biesheuvel
Thanks, applied.
- Ted
Re: [PATCH v4] fscrypt: add Adiantum support
On Mon, Nov 26, 2018 at 11:27:37AM -0800, Eric Biggers wrote:
> From: Eric Biggers
>
> Add support for the Adiantum encryption mode to fscrypt. Adiantum is a
> tweakable, length-preserving encryption mode with security provably
> reducible to that of XChaCha12 and AES-256, subject to a security bound.
> It's also a true wide-block mode, unlike XTS. See the paper
> "Adiantum: length-preserving encryption for entry-level processors"
> (https://eprint.iacr.org/2018/720.pdf) for more details. Also see
> commit 059c2a4d8e16 ("crypto: adiantum - add Adiantum support").
Thanks, applied.
- Ted
Re: [RFC PATCH v2 0/4] Exporting existing crypto API code through zinc
On Tue, Nov 20, 2018 at 05:24:41PM +0100, Jason A. Donenfeld wrote: > On Tue, Nov 20, 2018 at 3:19 PM Herbert Xu > wrote: > > Yes. In fact it's used for FIPS certification testing. > > Sure, nobody sane should be doing it. But when it comes to > > government certification... :) > > The kernel does not aim toward any FIPS certification, and we're not > going to start bloating our designs to fulfill this. It's never been a > goal. Maybe ask Ted to add a FIPS mode to random.c and see what > happens... When you start arguing "because FIPS!" as your > justification, you really hit a head scratcher. There are crazy people who go for FIPS certification for the kernel. That's why crypto/drbg.c exists. There is a crazy fips mode in drivers/char/random.c which causes the kernel to panic with a 1 in 2**80 probability each time _extract_entropy is called. It's not the default, and I have no idea if any one uses it, or if it's like the NIST OSI mandate, which forced everyone to buy an OSI networking stack --- and then put it on the shelf and use TCP/IP instead. Press release from March 2018: https://www.redhat.com/en/about/press-releases/red-hat-completes-fips-140-2-re-certification-red-hat-enterprise-linux-7 - Ted
Re: [PATCH net-next v8 28/28] net: WireGuard secure network tunnel
On Fri, Oct 26, 2018 at 01:47:21AM +0200, Jason A. Donenfeld wrote: > when it goes to sleep (screen blanking, wakelocks, etc). The Android > model of Linux revolves around this, and hence the suspend semantics > for WireGuard respect this model and adjust accordingly, using the > appropriate CONFIG_ANDROID to determine which model we're operating > under. This is not a bandaid, and it doesn't have to do with forks of > the Linux kernel. If that's what you are trying to conditionalize, why don't use CONFIG_PM_AUTOSLEEP? That way if there are other systems that want to use the Android wakelocks style of suspend management, your code will DTRT, as opposed to depending on CONFIG_ANDROID. - Ted
Re: [PATCH 3/3] crypto: qce: ice: Add support for Inline Crypto Engine
On Thu, Oct 25, 2018 at 09:55:48AM -0500, Rob Herring wrote: > > +Introduction: > > += > > +Storage encryption has been one of the most required feature from security > > +point of view. QTI based storage encryption solution uses general purpose > > +crypto engine. While this kind of solution provide a decent amount of > > +performance, it falls short as storage speed is improving significantly > > +continuously. To overcome performance degradation, newer chips are going to > > +have Inline Crypto Engine (ICE) embedded into storage device. ICE is > > supposed > > +to meet the line speed of storage devices. > > Is ICE part of the storage device or part of the host as the binding > suggests? My understanding is that for this particular instantiation, the Inline Crypto Engine is located on the SOC. However, from the perspective of generic kernel support, the inline crypto support could be implemented on the SOC, or in the host bus adaptor, or as a "bump in the wire", or on the storage device. And whatever abstract interface in the block layer should be able to support all of these cases. I do not believe it would be wise to assume that inline crypto will forever be a mobile-only thing. I could easily see use cases in the data center; for example, if you believe that Nation State Actors might be trying to create "implants" that attack hard drive firmware, per some of the Snowden leaks, creating an open design ICE engine with auditable firmware and a trusted secure key store, and which sits between the host CPU and the storage device might be one way to mitigate against this threat. - Ted
Re: [PATCH 3/3] crypto: qce: ice: Add support for Inline Crypto Engine
First, thanks for the effort for working on getting the core ICE driver support into upstreamable patches. On Wed, Oct 17, 2018 at 08:47:56PM +0530, AnilKumar Chimata wrote: > +2) Per File Encryption (PFE) > +Per File Manager(PFM) calls QSEECom api to create the key. PFM has a peer > comp- > +onent(PFT) at kernel layer which gets the corresponding key index from PFM. > ... > +Further Improvements > + > +Currently, Due to PFE use case, ICE driver is dependent upon dm-req-crypt to > +provide the keys as part of request structure. This couples ICE driver with > +dm-req-crypt based solution. It is under discussion to expose an IOCTL based > +and registration based interface APIs from ICE driver. ICE driver would use > +these two interfaces to find out if any key exists for current request. If > +yes, choose the right key index received from IOCTL or registration based > +APIs. If not, dont set any crypto parameter in the request. However, this documentation is referencing components which are not in the mainline kernel. In the long term, what I'd like to see for per-file key support is a clean solution which interfaces with the in-kernel fscrypt-enabled file systems (e.g., f2fs and ext4). What I think we need to do is to add a field in the bio structure which references a key id, and then define a bdi-specific interface which allows the file system to register a struct key and get a key id. Use of the key id will be refcounted, so the device driver knows how many I/O operations are in flight using a particular key --- since each ICE hardware will have a different number of active keys that it can support. Until that's there, at least for the upstream documentation, I think it would be better to drop mention of code that is not yet upstream --- and which may have problems ever going upstream in their current form. (I haven't checked in a while, but last time I looked the code in question blindly dereferenced private pointers assuming that the only two file systems that could ever use ICE were ext4 and f2fs not that the code used by Google handsets were _that_ much cleaner, but at least we dropped in a crypto key into the struct bio, instead of playing unnatural games with private pointers from the wrong abstraction layer. :-) - Ted
Re: random(4) and VMs
On Tue, Sep 18, 2018 at 01:00:31PM -0400, Sandy Harris wrote: > Solutions have been proposed by various people. If I understand them > right, Ted Ts'o suggests modifying the boot loader to provide some > entropy & John Denker suggests that every machine should be > provisioned with some entropy in the kernel image at install time. > Both are general solutions, but I think both would require updating > the entropy store later. As far as I know, neither has yet been > implemented as accepted patches > > Is a fix that only deals with a subset of the problem worth > considering? Just patch the VM support code so that any time a VM is > either booted or re-started after a save, the host system drops in > some entropy, This looks relatively easy to do, at least for Linux > VMs, and some of the code might be the same as what the more general > approaches would need. That already exists. It's called virtio-rng. On the host side, using kvm/qemu as an example, you just add the qemu options: -object rng-random,filename=/dev/urandom,id=rng0 \ -device virtio-rng-pci,rng=rng0 On the guest kernel, enable CONFIG_HW_RANDOM_VIRTIO. - Ted
Re: rng_dev_read: Kernel memory exposure attempt detected from SLUB object 'kmalloc-64'
On Mon, Sep 10, 2018 at 10:02:38PM +0200, Ard Biesheuvel wrote: > >> [146535.257274] tpm tpm0: A TPM error (379) occurred attempting get random > >> [146535.257304] usercopy: Kernel memory exposure attempt detected from > >> SLUB object 'kmalloc-64' (offset 0, size 379)! > > The TPM return code '379' is returned from rng_get_data(), and > interpreted as a byte count rather than an error code. So there are two bugs here. Once is in the TPM hw_random driver; it shouldn't be returning the TPM error code. The second is that rng_dev_read() should be more suspicious and validate the number of bytes returned from the low-level hw_random driver for sanity. - Ted
Re: rng_dev_read: Kernel memory exposure attempt detected from SLUB object 'kmalloc-64'
On Mon, Sep 10, 2018 at 08:08:51PM +0300, Meelis Roos wrote: > This is weekend's 4.19.0-rc2-00246-gd7b686ebf704 on a Thinkad T460s. > There seems to be a usercopy warning from rng_dev read (full dmesg > below). Looking at rng_dev_head(), which is in drivers/char/hw_random.c, it looks like this was probably caused by a problem in the specific hardware random number generator being used. Can you tell us which one was in use? Thanks!! - Ted > [0.00] microcode: microcode updated early to revision 0xc6, date = > 2018-04-17 > [0.00] Linux version 4.19.0-rc2-00246-gd7b686ebf704 (mroos@t460s) > (gcc version 8.2.0 (Debian 8.2.0-5)) #36 SMP Sat Sep 8 16:27:54 EEST 2018 > [0.00] Command line: > BOOT_IMAGE=/vmlinuz-4.19.0-rc2-00246-gd7b686ebf704 root=/dev/mapper/TP-ROOT ro > [0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point > registers' > [0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers' > [0.00] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers' > [0.00] x86/fpu: Supporting XSAVE feature 0x008: 'MPX bounds registers' > [0.00] x86/fpu: Supporting XSAVE feature 0x010: 'MPX CSR' > [0.00] x86/fpu: xstate_offset[2]: 576, xstate_sizes[2]: 256 > [0.00] x86/fpu: xstate_offset[3]: 832, xstate_sizes[3]: 64 > [0.00] x86/fpu: xstate_offset[4]: 896, xstate_sizes[4]: 64 > [0.00] x86/fpu: Enabled xstate features 0x1f, context size is 960 > bytes, using 'compacted' format. > [0.00] BIOS-provided physical RAM map: > [0.00] BIOS-e820: [mem 0x-0x0009cfff] usable > [0.00] BIOS-e820: [mem 0x0009d000-0x0009] reserved > [0.00] BIOS-e820: [mem 0x000e-0x000f] reserved > [0.00] BIOS-e820: [mem 0x0010-0xb100afff] usable > [0.00] BIOS-e820: [mem 0xb100b000-0xc3ed5fff] reserved > [0.00] BIOS-e820: [mem 0xc3ed6000-0xc3ed6fff] ACPI NVS > [0.00] BIOS-e820: [mem 0xc3ed7000-0xcff75fff] reserved > [0.00] BIOS-e820: [mem 0xcff76000-0xcff77fff] ACPI NVS > [0.00] BIOS-e820: [mem 0xcff78000-0xcff78fff] reserved > [0.00] BIOS-e820: [mem 0xcff79000-0xcffc5fff] ACPI NVS > [0.00] BIOS-e820: [mem 0xcffc6000-0xcfffdfff] ACPI > data > [0.00] BIOS-e820: [mem 0xcfffe000-0xd7ff] reserved > [0.00] BIOS-e820: [mem 0xd860-0xdc7f] reserved > [0.00] BIOS-e820: [mem 0xf800-0xfbff] reserved > [0.00] BIOS-e820: [mem 0xfd00-0xfe7f] reserved > [0.00] BIOS-e820: [mem 0xfec0-0xfec00fff] reserved > [0.00] BIOS-e820: [mem 0xfed0-0xfed00fff] reserved > [0.00] BIOS-e820: [mem 0xfed1-0xfed19fff] reserved > [0.00] BIOS-e820: [mem 0xfed84000-0xfed84fff] reserved > [0.00] BIOS-e820: [mem 0xfee0-0xfee00fff] reserved > [0.00] BIOS-e820: [mem 0xff80-0x] reserved > [0.00] BIOS-e820: [mem 0x0001-0x0003227f] usable > [0.00] NX (Execute Disable) protection: active > [0.00] SMBIOS 2.8 present. > [0.00] DMI: LENOVO 20F9003SMS/20F9003SMS, BIOS N1CET65W (1.33 ) > 02/16/2018 > [0.00] tsc: Detected 2400.000 MHz processor > [0.002224] e820: update [mem 0x-0x0fff] usable ==> reserved > [0.002226] e820: remove [mem 0x000a-0x000f] usable > [0.002234] last_pfn = 0x322800 max_arch_pfn = 0x4 > [0.002238] MTRR default type: write-back > [0.002239] MTRR fixed ranges enabled: > [0.002240] 0-9 write-back > [0.002241] A-B uncachable > [0.002242] C-F write-protect > [0.002242] MTRR variable ranges enabled: > [0.002244] 0 base 00E000 mask 7FE000 uncachable > [0.002245] 1 base 00DC00 mask 7FFC00 uncachable > [0.002246] 2 base 00DA00 mask 7FFE00 uncachable > [0.002246] 3 disabled > [0.002246] 4 disabled > [0.002247] 5 disabled > [0.002247] 6 disabled > [0.002248] 7 disabled > [0.002248] 8 disabled > [0.002248] 9 disabled > [0.003223] x86/PAT: Configuration [0-7]: WB WC UC- UC WB WP UC- WT > [0.003726] last_pfn = 0xb100b max_arch_pfn = 0x4 > [0.011684] Scanning 1 areas for low memory corruption > [0.011688] Base memory trampoline at [(ptrval)] 97000 size 24576 > [0.011691] Using GB pages for direct mapping > [0.011693] BRK [0x2422f6000, 0x2422f6fff] PGTABLE > [0.011695] BRK [0x2422f7000, 0x2422f7fff] PGTABLE > [0.011696] BRK [0x2422f8000, 0x2422f
Re: [PATCH 0/4] arm64: wire CRC32 instructions into core crc32 routines
On Mon, Aug 27, 2018 at 01:02:41PM +0200, Ard Biesheuvel wrote: > While this is not known to cause performance issues, calling a table based > time variant implementation with a non-negligible D-cache footprint (8 KB) > is wasteful in any case, and now that the crc32 instructions have been made > mandatory in the architecture, let's wire them up into the core crc routines. Stupid question --- are there any arm64 SOC's out there which do *not* have the crc32 instructions? Presumably there won't be in the future, because it's now mandatory --- but where there any in the past? - Ted
[GIT PULL] random updates for 4.19
The following changes since commit 81e69df38e2911b642ec121dec319fad2a4782f3: random: mix rdrand with entropy sent in from userspace (2018-07-17 21:32:36 -0400) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/tytso/random.git tags/random_for_linus for you to fetch changes up to 9a47249d444d344051c7c0e909fad0e88515a5c2: random: Make crng state queryable (2018-08-02 17:33:06 -0400) Some changes to trust cpu-based hwrng (such as RDRAND) for initializing hashed pointers and (optionally, controlled by a config option) to initialize the CRNG to avoid boot hangs. Ingo Molnar (1): random: remove preempt disabled region Jason A. Donenfeld (1): random: Make crng state queryable Theodore Ts'o (1): random: add a config option to trust the CPU's hwrng Tobin C. Harding (4): random: Fix whitespace pre random-bytes work random: Return nbytes filled from hw RNG vsprintf: Use hw RNG for ptr_key vsprintf: Add command line option debug_boot_weak_hash Documentation/admin-guide/kernel-parameters.txt | 8 drivers/char/Kconfig| 14 ++ drivers/char/random.c | 49 +++-- include/linux/random.h | 3 ++- lib/vsprintf.c | 27 ++- 5 files changed, 85 insertions(+), 16 deletions(-)
Re: random: ensure use of aligned buffers with ChaCha20
On Fri, Aug 10, 2018 at 08:20:51AM +0200, Stephan Mueller wrote:
> > while (nbytes >= CHACHA20_BLOCK_SIZE) {
> > int adjust = (unsigned long)buf & (sizeof(tmp[0]) - 1);
> >
> > extract_crng(buf);
>
> Why this line?
>
> > buf += CHACHA20_BLOCK_SIZE;
Sorry, the above two lines should be removed, of course.
> > if (likely(adjust == 0)) {
> > extract_crng(buf);
> > buf += CHACHA20_BLOCK_SIZE;
> > nbytes -= CHACHA20_BLOCK_SIZE;
> > } else {
> > extract_crng(tmp);
> > memcpy(buf, tmp, CHACHA20_BLOCK_SIZE - adjust);
> > buf += CHACHA20_BLOCK_SIZE - adjust;
> > nbytes -= CHACHA20_BLOCK_SIZE - adjust;
>
> Sure, why not.
>
> > }
> >
> > }
> >
> > This may be a hyper optimization, though --- it's not clear how often,
> > say the kernel would be calling get_random_bytes with size >> 64 at
> > all, never mind with an unaligned buffer.
>
> I agree it is not likely that we have unaligned buffers. But in case we have,
> we have the potential to overwrite memory that does not belong to us with
> unknown consequences.
Sure, faire enough. The potential wouldn't be overwriting memory,
though. It would be a kernel panic when the CPU trapped a non-aligned
pointer dereference.
- Ted
Re: random: ensure use of aligned buffers with ChaCha20
On Thu, Aug 09, 2018 at 08:38:56PM +0200, Stephan Müller wrote:
> The function extract_crng invokes the ChaCha20 block operation directly
> on the user-provided buffer. The block operation operates on u32 words.
> Thus the extract_crng function expects the buffer to be aligned to u32
> as it is visible with the parameter type of extract_crng. However,
> get_random_bytes uses a void pointer which may or may not be aligned.
> Thus, an alignment check is necessary and the temporary buffer must be
> used if the alignment to u32 is not ensured.
I'm wondering whether we have kernel code that actually tries to
extract more than 64 bytes, so I'm not sure how often we enter the
while loop at all. Out of curiosity, did you find this from code
inspection or via a kernel fault on some architecture that doesn't
allow unaligned u32 memory accesses?
> while (nbytes >= CHACHA20_BLOCK_SIZE) {
> - extract_crng(buf);
> - buf += CHACHA20_BLOCK_SIZE;
> + if (likely((unsigned long)buf & (sizeof(tmp[0]) - 1))) {
> + extract_crng(buf);
> + buf += CHACHA20_BLOCK_SIZE;
> + } else {
> + extract_crng(tmp);
> + memcpy(buf, tmp, CHACHA20_BLOCK_SIZE);
> + }
> +
That should be
if (likely(((unsigned long)buf & (sizeof(tmp[0]) - 1)) == 0) {
shouldn't it?
If we *did* have callers who were calling get_random_bytes with nbytes
significantly larger than 64 bytes (CHACHA20_BLOCK_SIZE), I wonder if
we should be doing something like this:
while (nbytes >= CHACHA20_BLOCK_SIZE) {
int adjust = (unsigned long)buf & (sizeof(tmp[0]) - 1);
extract_crng(buf);
buf += CHACHA20_BLOCK_SIZE;
if (likely(adjust == 0)) {
extract_crng(buf);
buf += CHACHA20_BLOCK_SIZE;
nbytes -= CHACHA20_BLOCK_SIZE;
} else {
extract_crng(tmp);
memcpy(buf, tmp, CHACHA20_BLOCK_SIZE - adjust);
buf += CHACHA20_BLOCK_SIZE - adjust;
nbytes -= CHACHA20_BLOCK_SIZE - adjust;
}
}
This may be a hyper optimization, though --- it's not clear how often,
say the kernel would be calling get_random_bytes with size >> 64 at
all, never mind with an unaligned buffer.
- Ted
Re: [PATCH] crypto: remove speck
On Mon, Aug 06, 2018 at 08:12:38PM -0700, Eric Biggers wrote: > I mention this because people are naturally going to be curious about that, > e.g. > speculating that Google found a "backdoor" -- remember that we do have some > good > cryptographers! I'm just stating what we know, out of honesty and openness; I > don't really intend to be arguing for Speck with this statement, and in any > case > we already made the decision to not use Speck. Let's be clear --- the arguments about whether or not to use Speck, and whether or not to remove Speck from the kernel, are purely political --- not techinical. - Ted
Re: [PATCH] random: add a config option to trust the CPU's hwrng
On Sat, Aug 04, 2018 at 08:25:14PM -0400, Theodore Y. Ts'o wrote: > Depending on your hardware, no mouse motion might be necessary at all. > On my laptop (a Dell XPS 13 model 9370) using an dm-crypt protected > root disk, and running a Debian testing userspace, with a 4.18-rc6 > based kernel, the "CRNG is initialized" message is printed *before* > the root file system is mounted. Sorry, correction. It's printed *before* the root file system is remounted read/write. (Which means before we can generate long-term public keys and save them to the file system.) - Ted
Re: [PATCH] random: add a config option to trust the CPU's hwrng
On Sat, Aug 04, 2018 at 11:52:10PM +0200, Pavel Machek wrote: > > However, enabling config option means that the CRNG will be > > initialized with potentially information available to the CPU > > manufacturer and/or Nation States, and this persists *after* > > initialization / early boot. So to say, "we're perfectly safe after > > we leave initialization / early boot" is not true. > > This should really be explained in the help text. > > I assume that after 10 seconds of moving mouse, user is safe even when > rdrand is backoored? You'll hate this answer, but "it depends". Suppose someone is using an init script which generates ssh keys upon first boot if they are missing. If this is the case, *and* RDRAND is backdoored, then the keys will be generated in such a way that they *might* be succeptible to being guessed by a nation state. Moving your mouse around for 1000 or 10,000 seconds won't help if the host's ssh keys has already been generated. Depending on your hardware, no mouse motion might be necessary at all. On my laptop (a Dell XPS 13 model 9370) using an dm-crypt protected root disk, and running a Debian testing userspace, with a 4.18-rc6 based kernel, the "CRNG is initialized" message is printed *before* the root file system is mounted. On other systems, where the hardware does not issue as many interrupts, the mouse motion might be extremely important in order to get the "CRNG is initialized" message. > (Plus, I'd say this should be kernel command line option, not config > option...?) Agreed, there should be a command line option as well. I just haven't gotten around to it yet, and in the meantime, having a config option is better than nothing. Patches gratefully accepted... :-) - Ted
Re: Does /dev/urandom now block until initialised ?
On Mon, Jul 23, 2018 at 12:11:12PM -0400, Jeffrey Walton wrote: > > I believe Stephan Mueller wrote up the weakness a couple of years ago. > He's the one who explained the interactions to me. Mueller was even > cited at https://github.com/systemd/systemd/issues/4167. Stephan had a lot of complaints about the existing random driver. That's because he has a replacement driver that he has been pushing, and instead of giving explicit complaints with specific patches to fix those specific issues, he have a generalized blast of complaints, plus a "big bang rewrite". I've reviewed his lrng doc, and this specific issue was not among his complaints. Quite a while ago, I had gone through his document, and had specifically addressed each of his complaints. As far as I have been able determine, all of the specific technical complaints (as opposed to personal preference issues) have been addressed. His complaint is a text book complaint about how *not* to file a bug report. That being said, we try to take bug reports from as many sources as possible even if they aren't well formed or submitted in the ideal place. (I'm reminded of Linux's networking scalability limitations which Microsoft filed in the Wall Street Journal 15+ years ago --- and which only applied if you had 4 CPU's and four 10 megabit networking cards; if you had four CPU's and a 100 megabit networking card, Linux would grind Microsoft into the dust; still it was a bug, and we appreciated the report and we fixed it, even if it wasn't filed in the ideal forum. :-) > It is too bad he Mueller not receive credit for it in the CVE database. As near as I can tell, he doesn't deserve it for this particular issue. It's all Jann Horn and Google's Project Zero. (And his writeup is a textbook example of how to report this sort of issue with great specifity and analysis.) - Ted
Re: Does /dev/urandom now block until initialised ?
On Mon, Jul 23, 2018 at 04:43:01AM +0100, Ken Moffat wrote: > Ted, > > last week you proposed an rfc patch to gather entropy from the CPU's > hwrng, and I was pleased - until I discovered one of my stalling > desktop machines does not have a hwrng. At that point I thought that > the problem was only from reading /dev/random, so I went away to look > at persuading the immediate consumer (unbound) to use /dev/urandom. > > Did that, no change. Ran strace from the bootscript, confirmed that > only /dev/urandom was being used, and that it seemed to be blocking. > Thought maybe this was the olnl problematic bootscript, tried moving > it to later, but hit the same problem on chronyd (again, seems to use > urandom). And yes, I probably should have started chronyd first > anyway, but that's irrelevant to this problem. Nope, /dev/urandom still doesn't block. Are you sure it isn't caused by something calling getrandom(2) --- which *will* block? We intentionally left /dev/urandom non-blocking, because of backwards compatibility. > BUT: I'm not sure if I've correctly understood what is happening. > It seems to me that the fix for CVE-2018-1108 (4.17-rc1, 4.16.4) > means /dev/urandom will now block until fully initialised. > > Is that correct and intentional ? No, that's not right. What the fix does is more accurately account for the entropy accounting before getrandom(2) would become non-blocking. There were a bunch of things we were doing wrong, including assuming that 100% of the bytes being sent via add_device_entropy() were random --- when some of the things that were feeding into it was the (fixed) information you would get from running dmidecode (e.g., the fixed results from the BIOS configuration data). Some of those bytes might not be known to an external adversary (such as your CPU mainboard's serial number), but it's not exactly *Secret*. > If so, to get the affected desktop machines to boot I seem to have > some choices... Well, this probably isn't going to be popular, but the other thing that might help is you could switch distro's. I'm guessing you run a Red Hat distro, probably Fedora, right? The problem which most people are seeing turns out to be a terrible interaction between dracut-fips, systemd and a Red Hat specific patch to libgcrypt for FIPS/FEDRAMP compliance: https://src.fedoraproject.org/rpms/libgcrypt/blob/master/f/libgcrypt-1.6.2-fips-ctor.patch#_23 Uninstalling dracut-fips and recreating the initramfs might also help. One of the reasons why I didn't see the problem when I was developing the remediation patch for CVE-2018-1108 is because I run Debian testing, which doesn't have this particular Red Hat patch. > The latter certainly lets it boot in a reasonable time, but people > who understand this seem to regard it as untrustworthy. For users > of /dev/urandom that is no big deal, but does it not mean that the > values from /dev/random will be similarly untrustworthy and > therefore I should not use this machine for generating long-lived > secure keys ? This really depends on how paranoid / careful you are. Remember, your keyboard controller was almost certainly built in Shenzhen, China, and Matt Blaze published a paper on the Jitterbug in 2006: http://www.crypto.com/papers/jbug-Usenix06-final.pdf In practice, after 30 minutes of operation, especially if you are using the keyboard, the entropy pool *will* be sufficiently randomized, whether or not it was sufficientl randomized at boot. The real danger of CVE-2018-1108 was always long-term keys generated at first boot. That was the problem that was discussed in the "Mining your p's and q's: Detection of Widespread Weak Keys in Network Devices" (see https://factorable.net). So generating long-lived keys means (a) you need to be sure you trust all of the software on the system --- some very paranoid people such as Bruce Schneier used a freshly installed machine from CD-ROM that was never attached to the network before examining materials from Edward Snowden, and (b) making sure the entropy pool is initialized. Remember we are constantly feeding input from the hardware sources into the entropy pool; it doesn't stop the moment we think the entropy pool is initialized. And you can always mix extra "stuff" into the entropy pool by echoing the results of say, taking series of dice rolls, aond sending it via the "cat" or "echo" command into /dev/urhandom. So it should be possible to use the machine for generated long lived keys; you might just need to be a bit more careful before you do it. It's really keys generated automatically at boot that are most at risk --- and you can always regenerate the host SSH keys after a fresh install. In fact, what I have done in the past when I first login to a freshly created Cloud VM system is to run command like "dd if=/dev/urandom count=1 bs=256 | od -x", then login to VM, and then run "cat > /dev/urandom", and cut and paste the results of the od -x output into the guest VM
Re: [PATCH] random: addu a config option to trust the CPU's hwrng
On Wed, Jul 18, 2018 at 04:22:35PM -0400, Sandy Harris wrote: > > Yes & one of those can also solve any difficulty with random(4) at > startup. Another alternative, perhaps easier on some systems, is > Denker's Turbid trng: > https://www.av8n.com/turbid/paper/turbid.htm In the link above I saw breadboards with resistors plugged in, alligator clips to TRG plugs, shield boxes with aluminum foil, and doing calibrations using voltmeters. While that induced a pleasant flashback to my junior high days when I experimented with electronics, I'm not entirely sure most sysadmins would call that "easy". :-) - Ted
Re: [PATCH] random: add a config option to trust the CPU's hwrng
On Wed, Jul 18, 2018 at 05:29:58PM +0200, Yann Droneaud wrote:
> Sure, but, AFAICT, RDRAND is already in use through arch_get_random_*()
> functions when CONFIG_ARCH_RANDOM is enabled.
>
> From an outside PoV, there's a conflict: why one would want its kernel
> to use CPU hwrng if one has purposely disabled CONFIG_RANDOM_TRUST_CPU
> ?
Yes, but we use it to mix in RDRAND into the entropy pool. So we're
not depending solely on RDRAND's output. The trust model that we're
using is this. The presumption is that (at least for US-based CPU
manfacturers) the amount of effort needed to add a blatent backdoor
to, say, the instruction scheduler and register management file is
such that it couldn't be done by a single engineer, or even a very
small set of engineers. Enough people would need to know about it, or
would be able to figure out something untowards was happening, or it
would be obvious through various regression tests, that it would be
obvious if there was a generic back door in the CPU itself. This is a
good thing, because ultimately we *have* to trust the general purpose
CPU. If the CPU is actively conspiring against you, there really is
no hope.
However, the RDRAND unit is a small, self-contained thing, which is
*documented* to use an AES whitener (e.g., it does an AES encryption
as its last step). So presumably, a change to make the RDRAND unit
effectively be:
AES_ENCRYPT(NSA_KEY, COUNTER++)
Is much easier to hide or introduce.
So that's why people are comfortable using RDRAND mixed into the
output of the entropy pools. Yes, in theory, if the CPU has
backdoored the XOR instruction if it sees an RDRAND just before it,
you're sunk. But in if you don't trust the CPU to that level, you
should simply not be using that CPU at all. Period.
So personally, I probably would never chose to use a CPU that was
manufactured by a company owned or controlled by a PLA general or one
of Putin's Oligarchs. But I'm not going to tell other people what to
do; they should make their own decisions.
Now, there is one exception to this, and that is the CPU has RDRAND
support, it will use that exclusively for get_random_{u32, u64, int, long}.
But kernel code shouldn't be using this for cryptographic purposes. If you
need to generate a random key, you should be using get_random_bytes().
get_random_u32, et. al, are designed for things like stack canaries or
TCP sequence numbers.
Regards,
- Ted
Re: [PATCH] random: addu a config option to trust the CPU's hwrng
On Wed, Jul 18, 2018 at 11:14:20AM -0400, Sandy Harris wrote: > Instead, I had a compile-time option to choose a number 0-32 > for how much entropy to assume a 32-bit value from the HWRNG > contains. Default was something less than 32. I debated values > in the 24-30 range, don't recall what I chose & don't think it > Matters hugely. > > Is that a better approach than the binary choice? This patch is only affecting the initialization of the CRNG. It doesn't do anything about the entropy estimator, so it doesn't change the behavior of /dev/random, for example. In practice I doubt most people would be able to deal with a numerical dial, and I'm trying to ecourage people to use getrandom(2). I view /dev/random as a legacy interface, and for most people a CRNG is quite sufficient. For those people who are super paranoid and want a "true random number generator" (and the meaning of that is hazy) because a CRNG is Not Enough, my recommendation these days is that they get something like an open hardware RNG solution, such as ChaosKey from Altus Metrum[1]. [1] https://altusmetrum.org/ChaosKey/ - Ted
Re: [PATCH] random: add a config option to trust the CPU's hwrng
On Wed, Jul 18, 2018 at 09:22:13AM +0200, Yann Droneaud wrote: > > The text message should explain this is only relevant during > initialization / early boot. > > The config option name should state this. There are other workarounds for hangs that happen after initialization / early boot, yes. They are of varying levels of quality / safely, but that's neither here nor there. However, enabling config option means that the CRNG will be initialized with potentially information available to the CPU manufacturer and/or Nation States, and this persists *after* initialization / early boot. So to say, "we're perfectly safe after we leave initialization / early boot" is not true. So I'd much rather make it clear that we are trusting the CPU manufacturer far more than just during early boot. Cheers, - Ted
Re: [PATCH] random: add a config option to trust the CPU's hwrng
On Tue, Jul 17, 2018 at 09:43:44PM -0400, Theodore Ts'o wrote:
> This gives the user building their own kernel (or a Linux
> distribution) the option of deciding whether or not to trust the CPU's
> hardware random number generator (e.g., RDRAND for x86 CPU's) as being
> correctly implemented and not having a back door introduced (perhaps
> courtesy of a Nation State's law enforcement or intelligence
> agencies).
>
> This will prevent getrandom(2) from blocking, if there is a
> willingness to trust the CPU manufacturer.
>
> Signed-off-by: Theodore Ts'o
Note, I had meant to tag this with an RFC. I'm not sure I really want
to push this to Linus yet. If you have an opinion, let me know.
Thanks!
- Ted
> ---
>
> I'm not sure Linux distro's will thank us for this. The problem is
> trusting the CPU manfuacturer can be an emotional / political issue.
>
> For example, assume that China has decided that as a result of the
> "death sentence" that the US government threatened to impose on ZTE
> after they were caught introducing privacy violating malware on US
> comsumers, that they needed to be self-sufficient in their technology
> sector, and so they decided the needed to produce their own CPU.
>
> Even if I were convinced that Intel hadn't backdoored RDRAND (or an
> NSA agent backdoored RDRAND for them) such that the NSA had a NOBUS
> (nobody but us) capability to crack RDRAND generated numbers, if we
> made a change to unconditionally trust RDRAND, then I didn't want the
> upstream kernel developers to have to answer the question, "why are
> you willing to trust Intel, but you aren't willing to trust a company
> owned and controlled by a PLA general?" (Or a company owned and
> controlled by one of Putin's Oligarchs, if that makes you feel
> better.)
>
> With this patch, we don't put ourselves in this position --- but we
> do put the Linux distro's in this position intead. The upside is it
> gives the choice to each person building their own Linux kernel to
> decide whether trusting RDRAND is worth it to avoid hangs due to
> userspace trying to get cryptographic-grade entropy early in the boot
> process. (Note: I trust RDRAND more than I do Jitter Entropy.)
>
> drivers/char/Kconfig | 14 ++
> drivers/char/random.c | 11 ++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 212f447938ae..fe2930c4ecf0 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -554,3 +554,17 @@ config ADI
>
> endmenu
>
> +config RANDOM_TRUST_CPU
> + bool "Trust the CPU manufacturer to initialize Linux's CRNG"
> + depends on (X86 || X86_64 || X86_32 || S390 || PPC)
> + default n
> + help
> + Assume that CPU manufacurer (e.g., Intel or AMD for RDSEED or
> + RDRAND, IBM for the S390 and Power PC architectures) is trustworthy
> + for the purposes of initializing Linux's CRNG. Since this is not
> + something that can be indepedently audited, this amounts to trusting
> + that CPU manufacturer (perhaps with the insistance or requirement
> + of a Nation State's intelligence or law enforcement agencies)
> + has not installed a hidden back door to compromise the CPU's
> + random number generation facilities.
> +
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 34ddfd57419b..f4013b8a711b 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -782,6 +782,7 @@ static void invalidate_batched_entropy(void);
> static void crng_initialize(struct crng_state *crng)
> {
> int i;
> + int arch_init = 1;
> unsigned long rv;
>
> memcpy(&crng->state[0], "expand 32-byte k", 16);
> @@ -792,10 +793,18 @@ static void crng_initialize(struct crng_state *crng)
> _get_random_bytes(&crng->state[4], sizeof(__u32) * 12);
> for (i = 4; i < 16; i++) {
> if (!arch_get_random_seed_long(&rv) &&
> - !arch_get_random_long(&rv))
> + !arch_get_random_long(&rv)) {
> rv = random_get_entropy();
> + arch_init = 0;
> + }
> crng->state[i] ^= rv;
> }
> +#ifdef CONFIG_RANDOM_TRUST_CPU
> + if (arch_init) {
> + crng_init = 2;
> + pr_notice("random: crng done (trusting CPU's manufacturer)\n");
> + }
> +#endif
> crng->init_time = jiffies - CRNG_RESEED_INTERVAL - 1;
> }
>
> --
> 2.18.0.rc0
>
Re: Q for a new API for the random device driver
On Wed, Jun 06, 2018 at 04:58:29PM +0200, Harald Freudenberger wrote: > Had a short glimpse to the mentioned add_hwgenerator_randomness() > and this looks in fact like the API I am looking for :-) > Thanks Stephan, I'll write some code and check this out. The more convenient interface would be structure things as a driver under drivers/char/hw_random. The virtio_rng.c driver is a relatively simple one that you can use as a model. That way you don't have "push" randomness at the random pool. Instead, a kernel thread, implemnted in drivers/char/hw_random/core.c, will automatically "pull" randomness from your hardware random number generator when the entropy estimate in the entropy pool falls below the low watermark, and stop when it goes above the high watermark. The arch_get_random/arch_get_random_seed interfaces are intended for this CPU's that have a high-efficiency RNG where getting entropy is "free". But if it is not free, then you only want to draw on the hardware RNG when it is needed and that's what the hw_random infrastructure will do automatically for you. - Ted
Re: PBKDF2 support in the linux kernel
On Sun, May 27, 2018 at 01:22:05PM +0200, Rafael J. Wysocki wrote: > Again, the PBKDF2 user would be hibernation. It could either take a key from > user space, which would require a key-generating user-space component to be > present in the initramfs (I guess no issue for a regular distro, but I can > imagine cases when it may be a difficulty), or take a passphrase from user > space and generate a key by itself (that's what we would like to use PBKDF2 > for). Right, but are you going to get the passphrase from user space? You have to prompt from user space anyway, so running PBPDF2 from userspace isn't that big of deal. Feel free to grab the implementation from e2fsprogs; it's not hard. :-) - Ted
Re: PBKDF2 support in the linux kernel
On Sat, May 26, 2018 at 03:36:37PM +0200, Stephan Mueller wrote: > - security related code should be vetted (which arguably is the case when the > discussed PBKDF is part of the kernel) > > > > If he/she were to add their own userland code then he would surely be > > criticized for rolling his own implementation. If one were to copy and paste cryptographic algorithms, that's surely not "rolling your own implementation" --- you're using someone else's implemntation. The argument for why code reuse by copy and paste is not a good thing is that you won't pick up bug fixes that might show up later. But for a cryptographic algorithm (e.g., sHA256, CHACHA20, etc.) with test vectors, in general there aren't cases where there are bug fixes that come later. An argument which makes sense for, say, an implementation of TLS, is not really applicable here. Given that there are other ways to address using well-vetted and reviewed code, I would argue that "run code at with least privleges" is clearly the far more important consideration here. - Ted
Re: PBKDF2 support in the linux kernel
On Fri, May 25, 2018 at 12:07:06PM +0200, Tomas Mraz wrote: > > Because having millions of copies of SHA1, MD5, and SHA2 and in > millions of applications is the best thing. > > Now that's something I would call laziness - just copy the code and do > not care about doing the proper decision which crypto library to use. These algorithms are static and have test vectors. If you don't need hardware acceleration for your use case, and portability and reducing external dependencies are a priority, it's a very realistic engineering tradeoff. libext2fs has been ABI backwards compatible for 19 years (since the move from a.out to ELF shared libraries). OpenSSL can't keep ABI compatibility from one relase to another. You can't build ABI compatibility on top of shifting sands, so that's a really good reason for a library not to depend on OpenSSL (if you care about backwards compatibility, anyway). Also consider that sha512.o is only 4735 bytes. libxml2 has a size of 1.75 megabytes, so having my own version of sha512 is equivalent to 0.26% of libxml2. Using my own copy of sha512? 2.5 milli-libxml2's. Shared library ABI backwards compatibility? Priceless. (And I won't even get into the bloat-o-rama which is GNOME2) - Ted
Re: PBKDF2 support in the linux kernel
One more thought about why userspace using AF_ALG is a bad idea --- there is no guarantee that all kernels will have all of the crypto algorithms you need built into the kernel. People who build custom kernels very often only include those kernel modules they need. So by default I don't normally build the more exotic crypto algorithms into my kernel --- and some people might not the crypto algorithms _you_ care about built into the kernel. (Not every one uses distro kernels.) So if you want your program to work everywhere, you're going to have to provide fallback crypto algorithms anyway. Which is why arguably it was a Really Bad Idea that AF_ALG provides access to software-only crypto implementations in the kernel. It led userspace programmers down the primrose path into making programs that are fragile with respect to users with custom-built kernels. - Ted
Re: PBKDF2 support in the linux kernel
On Thu, May 24, 2018 at 07:09:27PM -0500, Denis Kenzior wrote: > > But seriously, how is it a fault of the 'random person on the mailing list' > that AF_ALG exists and is being used for its (seemingly intended) purpose? > > I'm not really here to criticize or judge the past. AF_ALG exists now. It > is being used. Can we just make it better? Or are we going to whinge at > every user that tries to use (and improve) kernel features that (some) > people disagree with because it can 'compromise' kernel security? Another point of view is that it was arguably a mistake, and we shouldn't make it worse. > > Also, if speed isn't a worry, why not just a single software-only > > implementation of SHA1, and be done with it? It's what I did in > > e2fsprogs for e4crypt. > > If things were that simple, we would definitely not be having this exchange. > Lets just say we use just about every feature that crypto subsystem provides > in some way. What I'm saying here is if you need to code PBPDF2 in user-space, it _really_ isn't hard. I've done it. It's less than 7k of source code to implement SHA512: https://ghit.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/lib/ext2fs/sha256.c and then less than 50 lines of code to implement PBPDF2 (I didn't even bother putting in a library such as libext2fs): https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/misc/e4crypt.c#n405 This is all you would need to do if we don't put PBPDF2 in the kernel. Is it really that onerous? If you don't want to use some crypto library, I don't blame you --- I just grabbed the code and dropped it into e2fsprogs; so I understand that POV. And so if you want to keep using AF_ALG, we made a mistake, created an attractive nuisance, and we have to support it forever. Fine. But we don't have to add more _stuff_ into the kernel - Ted
Re: PBKDF2 support in the linux kernel
On Thu, May 24, 2018 at 05:08:41PM -0500, Denis Kenzior wrote: > Actually for the use case we have, speed is something pretty low on the > priority list; not having to deal with userspace crypto library dependencies > was a goal in and of itself. Each one has its own issues and you can never > support just one. Using AF_ALG has been rather... liberating. Which is probably why Eric used the word, "laziness". You might use a different word, but the decisoin was one that was more driven by convenience than kernel security Also, if speed isn't a worry, why not just a single software-only implementation of SHA1, and be done with it? It's what I did in e2fsprogs for e4crypt. - Ted
Re: PBKDF2 support in the linux kernel
On Thu, May 24, 2018 at 12:11:35PM -0500, Denis Kenzior wrote: > > Well, I'm not sure where the laziness comment is coming from. We have at > least two user-space implementations that implement PBKDF on top of AF_ALG. > But a typical invocation of PBKDF runs a couple of thousand iterations. > That is a lot of system call overhead. Would it not be better to fix things > to be more efficient rather than worry about how 'mistakes were made'? Even where there is hardware acceleration, I suspect that it might be more efficient (as in, result in a faster implementation) if the user PBKDF application was changed to use its own in-userspace software implementation. Many/most hardware implementations are optimzied for throughput (e.g., bulk data operations), and it's not obvious to me that once you had the syscall overhead, it's actually faster to use the hardware accleration. Has anyone actually done the experiment and verified that it was in fact a win to use AF_ALG on at least _some_ platform? What about the common cast for most platforms, even those that had some kind of hardware accleration that could only be accessed by the kernel? (Amusing war story: the hardware where we first experimented with ext4 encryption, the hardware "acceleration" offered by the ARM core in question was *slower* than a well-tuned software-only implementation on the same ARM CPU! :-) - Ted
Re: [PATCH v2] fscrypt: log the crypto algorithm implementations
On Fri, May 18, 2018 at 10:58:14AM -0700, Eric Biggers wrote: > Log the crypto algorithm driver name for each fscrypt encryption mode on > its first use, also showing a friendly name for the mode. > > This will help people determine whether the expected implementations are > being used. In some cases we've seen people do benchmarks and reject > using encryption for performance reasons, when in fact they used a much > slower implementation of AES-XTS than was possible on the hardware. It > can make an enormous difference; e.g., AES-XTS on ARM is about 10x > faster with the crypto extensions (AES instructions) than without. > > This also makes it more obvious which modes are being used, now that > fscrypt supports multiple combinations of modes. > > Example messages (with default modes, on x86_64): > > [ 35.492057] fscrypt: AES-256-CTS-CBC using implementation > "cts(cbc-aes-aesni)" > [ 35.492171] fscrypt: AES-256-XTS using implementation "xts-aes-aesni" > > Note: algorithms can be dynamically added to the crypto API, which can > result in different implementations being used at different times. But > this is rare; for most users, showing the first will be good enough. > > Signed-off-by: Eric Biggers Applied, thanks. - Ted
Re: [PATCH v2] fscrypt: add Speck128/256 support
On Mon, May 07, 2018 at 05:22:08PM -0700, Eric Biggers wrote: > fscrypt currently only supports AES encryption. However, many low-end > mobile devices have older CPUs that don't have AES instructions, e.g. > the ARMv8 Cryptography Extensions. Currently, user data on such devices > is not encrypted at rest because AES is too slow, even when the NEON > bit-sliced implementation of AES is used. Unfortunately, it is > infeasible to encrypt these devices at all when AES is the only option. > > Therefore, this patch updates fscrypt to support the Speck block cipher, > which was recently added to the crypto API. The C implementation of > Speck is not especially fast, but Speck can be implemented very > efficiently with general-purpose vector instructions, e.g. ARM NEON. > For example, on an ARMv7 processor, we measured the NEON-accelerated > Speck128/256-XTS at 69 MB/s for both encryption and decryption, while > AES-256-XTS with the NEON bit-sliced implementation was only 22 MB/s > encryption and 19 MB/s decryption. > > There are multiple variants of Speck. This patch only adds support for > Speck128/256, which is the variant with a 128-bit block size and 256-bit > key size -- the same as AES-256. This is believed to be the most secure > variant of Speck, and it's only about 6% slower than Speck128/128. > Speck64/128 would be at least 20% faster because it has 20% rounds, and > it can be even faster on CPUs that can't efficiently do the 64-bit > operations needed for Speck128. However, Speck64's 64-bit block size is > not preferred security-wise. ARM NEON also supports the needed 64-bit > operations even on 32-bit CPUs, resulting in Speck128 being fast enough > for our targeted use cases so far. > > The chosen modes of operation are XTS for contents and CTS-CBC for > filenames. These are the same modes of operation that fscrypt defaults > to for AES. Note that as with the other fscrypt modes, Speck will not > be used unless userspace chooses to use it. Nor are any of the existing > modes (which are all AES-based) being removed, of course. > > We intentionally don't make CONFIG_FS_ENCRYPTION select > CONFIG_CRYPTO_SPECK, so people will have to enable Speck support > themselves if they need it. This is because we shouldn't bloat the > FS_ENCRYPTION dependencies with every new cipher, especially ones that > aren't recommended for most users. Moreover, CRYPTO_SPECK is just the > generic implementation, which won't be fast enough for many users; in > practice, they'll need to enable CRYPTO_SPECK_NEON to get acceptable > performance. > > More details about our choice of Speck can be found in our patches that > added Speck to the crypto API, and the follow-on discussion threads. > We're planning a publication that explains the choice in more detail. > But briefly, we can't use ChaCha20 as we previously proposed, since it > would be insecure to use a stream cipher in this context, with potential > IV reuse during writes on f2fs and/or on wear-leveling flash storage. > > We also evaluated many other lightweight and/or ARX-based block ciphers > such as Chaskey-LTS, RC5, LEA, CHAM, Threefish, RC6, NOEKEON, SPARX, and > XTEA. However, all had disadvantages vs. Speck, such as insufficient > performance with NEON, much less published cryptanalysis, or an > insufficient security level. Various design choices in Speck make it > perform better with NEON than competing ciphers while still having a > security margin similar to AES, and in the case of Speck128 also the > same available security levels. Unfortunately, Speck does have some > political baggage attached -- it's an NSA designed cipher, and was > rejected from an ISO standard (though for context, as far as I know none > of the above-mentioned alternatives are ISO standards either). > Nevertheless, we believe it is a good solution to the problem from a > technical perspective. > > Certain algorithms constructed from ChaCha or the ChaCha permutation, > such as MEM (Masked Even-Mansour) or HPolyC, may also meet our > performance requirements. However, these are new constructions that > need more time to receive the cryptographic review and acceptance needed > to be confident in their security. HPolyC hasn't been published yet, > and we are concerned that MEM makes stronger assumptions about the > underlying permutation than the ChaCha stream cipher does. In contrast, > the XTS mode of operation is relatively well accepted, and Speck has > over 70 cryptanalysis papers. Of course, these ChaCha-based algorithms > can still be added later if they become ready. > > The best known attack on Speck128/256 is a differential cryptanalysis > attack on 25 of 34 rounds with 2^253 time complexity and 2^125 chosen > plaintexts, i.e. only marginally faster than brute force. There is no > known attack on the full 34 rounds. > > Signed-off-by: Eric Biggers Applied, thanks. - Ted
Re: [PATCH 1/5] random: fix crng_ready() test
On Thu, May 17, 2018 at 08:01:04AM +0200, Christophe LEROY wrote: > > On a powerpc embedded board which has an mpc8xx processor running at 133Mhz, > I now get the startup done in more than 7 minutes instead of 30 seconds. > This is due to the webserver blocking on read on /dev/random until we get > 'random: crng init done': > > [0.00] Linux version 4.17.0-rc4-00415-gd2f75d40072d (root@localhost) > (gcc version 5.4.0 (GCC)) #203 PREEMPT Wed May 16 16:32:02 CEST 2018 > [0.295453] random: get_random_u32 called from > bucket_table_alloc+0x84/0x1bc with crng_init=0 > [1.030472] device: 'random': device_add > [1.031279] device: 'urandom': device_add > [1.420069] device: 'hw_random': device_add > [2.156853] random: fast init done > [ 462.007776] random: crng init done > > This has become really critical, is there anything that can be done ? Figure out why the webserver needs to read /dev/random and is it for a security critical purpose? A kernel patch which makes the kernel do a "lalalalala I'm secure" when it really isn't secure is a simple "solution", but would it really make you happy? - Ted
Re: [PATCH 1/5] random: fix crng_ready() test
On Wed, May 16, 2018 at 05:07:08PM -0700, Srivatsa S. Bhat wrote: > > On a Photon OS VM running on VMware ESXi, this patch causes a boot speed > regression of 5 minutes :-( [ The VM doesn't have haveged or rng-tools > (rngd) installed. ] > > [1.420246] EXT4-fs (sda2): re-mounted. Opts: barrier,noacl,data=ordered > [1.469722] tsc: Refined TSC clocksource calibration: 1900.002 MHz > [1.470707] clocksource: tsc: mask: 0x max_cycles: > 0x36c65c1a9e1, max_idle_ns: 881590695311 ns > [1.474249] clocksource: Switched to clocksource tsc > [1.584427] systemd-journald[216]: Received request to flush runtime > journal from PID 1 > [ 346.620718] random: crng init done > > Interestingly, the boot delay is exacerbated on VMs with large amounts > of RAM. For example, the delay is not so noticeable (< 30 seconds) on a > VM with 2GB memory, but goes up to 5 minutes on an 8GB VM. > > Also, cloud-init-local.service seems to be the one blocking for entropy > here. So the first thing I'd ask you to investigate is what the heck cloud-init-local.service is doing, and why it really needs cryptographic grade random numbers? > It would be great if this CVE can be fixed somehow without causing boot speed > to spuike from ~20 seconds to 5 minutes, as that makes the system pretty much > unusable. I can workaround this by installing haveged, but ideally an > in-kernel > fix would be better. If you need any other info about my setup or if you have > a patch that I can test, please let me know! So the question is why is strong random numbers needed by cloud-init-local, and how much do you trust either haveged and/or RDRAND (which is what you will be depending upon if you install rng-tools). If you believe that Intel and/or the NSA hasn't backdoored RDRAND[1], or you believe that because Intel processor's internal cache architecture isn't publically documented, and it's S complicated that no one can figure it out (which is what you will be depending upon if you if you choose haveged), be my guest. I personally consider the latter to be "security via obscu7rity", but others have different opinions. [1] As an aside, read the best paper from the 37th IEEE Symposium on Security and Privacy and weep: https://www.computerworld.com/article/3079417/security/researchers-built-devious-undetectable-hardware-level-backdoor-in-computer-chips.html If it turns out that the security if your VM is critically dependent on what cloud-init-local.service is doing, and you don't like making those assumptions, then you may need to ask VMWare ESXi to make virtio-rng available to guest OS's, and then make Photon OS depend on a secure RNG available to the host OS. Best regards, - Ted
[GIT PULL] /dev/random fixes for 4.17-rc3
The following changes since commit d848e5f8e1ebdb227d045db55fe4f825e82965fa: random: add new ioctl RNDRESEEDCRNG (2018-04-14 11:59:31 -0400) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/tytso/random.git tags/random_for_linus_stable for you to fetch changes up to 4e00b339e264802851aff8e73cde7d24b57b18ce: random: rate limit unseeded randomness warnings (2018-04-25 02:41:39 -0400) Fix a regression on NUMA kernels and suppress excess unseeded entropy pool warnings. Theodore Ts'o (2): random: fix possible sleeping allocation from irq context random: rate limit unseeded randomness warnings drivers/char/random.c | 48 ++-- 1 file changed, 42 insertions(+), 6 deletions(-)
Re: [PATCH v2 0/5] crypto: Speck support
On Tue, Apr 24, 2018 at 10:58:35PM +0200, Jason A. Donenfeld wrote: > > Note that Linux > > doesn't bow down to any particular standards organization, and it offers > > algorithms that were specified in various places, even some with no more > > than a > > publication by the author. In fact, support for SM4 was just added too, > > which > > is a Chinese government standard. Are you going to send a patch to remove > > that > > too, or is it just NSA designed algorithms that are not okay? > > No need to be belittling; I have much less tinfoil strapped around my > head than perhaps you think. I'm not blindly opposed to > government-designed algorithms. Take SHA2, for example -- built by the > NSA. > > But I do care quite a bit about using ciphers that have acceptance of > the academic community and a large body of literature documenting its > design decisions and analyzing it. So where is the large body of literature documenting the design decisions of SM2? Has it received as much analysis as Speck? And if not, why aren't you gunning after it? - Ted
Re: Question on random.c add_interrupt_randomness function
On Tue, Apr 24, 2018 at 03:24:55PM +0200, Harald Freudenberger wrote: > The condition is true and terminates the function > when the count value of the cpu fast pool is below 64 > AND the time since last mix of the pool is lower than > HZ (so lower than 1s). > This means the code following this condition is runu > when the count value is > 64 or the last mix is more > than 1s old. > As the fast_mix() function does a fast_pool->count++ > effectively every 64 invocations this condition is > false and the rest of the function is executed. > > Is this the intention? Yes, that's the intention. Originally we required sampling the IP and/or architecture's cycle counter (if available) and mixing it using fast_mix before we transfer it to the input_pool and credit it with a bit of entropy. The problem is that on some architectures interrupts happen are much less frequently. So what we did is to assume that one second's worth of interrupts will be sufficient to derive a single bit of entropy. - Ted
Re: [PATCH 1/5] random: fix crng_ready() test
On Fri, Apr 13, 2018 at 03:05:01PM +0200, Stephan Mueller wrote:
>
> What I would like to point out that more and more folks change to
> getrandom(2). As this call will now unblock much later in the boot cycle,
> these systems see a significant departure from the current system behavior.
>
> E.g. an sshd using getrandom(2) would be ready shortly after the boot
> finishes
> as of now. Now it can be a matter minutes before it responds. Thus, is such
> change in the kernel behavior something for stable?
It will have some change on the kernel behavior, but not as much as
you might think. That's because in older kernels, we were *already*
blocking until crng_init > 2 --- if the getrandom(2) call happened
while crng_init was in state 0.
Even before this patch series, we didn't wake up a process blocked on
crng_init_wait until crng_init state 2 is reached:
static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
{
...
if (crng == &primary_crng && crng_init < 2) {
invalidate_batched_entropy();
crng_init = 2;
process_random_ready_list();
wake_up_interruptible(&crng_init_wait);
pr_notice("random: crng init done\n");
}
}
This is the reason why there are reports like this: "Boot delayed for
about 90 seconds until 'random: crng init done'"[1]
[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1685794
So we have the problem already. There will be more cases of this
after this patch series is applied, true. But what we have already is
an inconsistent state where if you call getrandom(2) while the kernel
is in crng_init state 0, you will block until crng_init state 2, but
if you are in crng_init state 1, you will assume the CRNG is fully
initialized.
Given the documentation of how getrandom(2) works what its documented
guarantees are, I think it does justify making its behavior both more
consistent with itself, and more consistent what the security
guarantees we have promised people.
I was a little worried that on VM's this could end up causing things
to block for a long time, but an experiment on a GCE VM shows that
isn't a problem:
[0.00] Linux version 4.16.0-rc3-ext4-9-gf6b302ebca85 (tytso@cwcc)
(gcc version 7.3.0 (Debian 7.3.0-15)) #16 SMP Thu Apr 12 16:57:17 EDT 2018
[1.282220] random: fast init done
[3.987092] random: crng init done
[4.376787] EXT4-fs (sda1): re-mounted. Opts: (null)
There are some desktops where the "crng_init done" report doesn't
happen until 45-90 seconds into the boot. I don't think I've seen
reports where it takes _minutes_ however. Can you give me some
examples of such cases?
- Ted
P.S. Of course, in a VM environment, if the host supports virtio-rng,
the boot delay problem is completely not an issue. You just have to
enable virtio-rng in the guest kernel, which I believe is already the
case for most distro kernels.
BTW, for KVM, it's fairly simple to set it the host-side support for
virtio-rng. Just add to the kvm command-line options:
-object rng-random,filename=/dev/urandom,id=rng0 \
-device virtio-rng-pci,rng=rng0
Re: [PATCH 1/5] random: fix crng_ready() test
On Fri, Apr 13, 2018 at 07:38:30AM +0200, Stephan Mueller wrote: > > The crng_init variable has three states: > > > > 0: The CRNG is not initialized at all > > 1: The CRNG has a small amount of entropy, hopefully good enough for > >early-boot, non-cryptographical use cases > > 2: The CRNG is fully initialized and we are sure it is safe for > >cryptographic use cases. > > > > The crng_ready() function should only return true once we are in the > > last state. > > > Do I see that correctly that getrandom(2) will now unblock after the > input_pool has obtained 128 bits of entropy? Similarly for > get_random_bytes_wait. Correct. > As this seems to be the only real use case for crng_ready (apart from > logging), what is the purpose of crng_init == 1? Immediately after boot (crng_init state 0), we funnel the harvested entropy from add_interrupt_entropy() directly to the CRNG pool. The goal is to get at least some amount of entropy into the CRNG is it's not blatently predictable. When we have accomplished this, by mixing in 2 * CHACHA20_KEY_SIZE bytes of input from add_input_randomness, we enter state crng_init state 1. In crng_init state 1, we start funnelling the harvested entropy to the input_pool. Once we have accumulated 128 bits of entropy in the input pool, we then reseed the CRNG from the input_pool, and we consider the CRNG to be fully intialized. During crng_init state 1, the CRNG is basically in a "good enough for government work" state. It's going to get used by things like initial TCP sequence numbers, and UUID's by udev, et. al, but I wouldn't want to use it for anything that has strong cryptographic use cases. It would be preferable if nothing in the kernel and early systemd startup used get_random_bytes() or /dev/urandom or getrandom(2) w/ GRND_NONBLOCK. Unfortunately, (a) there is a lot of legacy code which still uses /dev/urandom and not getrandom(2) and (b) in some cases, such as initialization of the Stack Canary, the kernel simply can't wait for the CRNG to be fully initialized. Or if the kernel needs enough of the networking stack to mount an NFS root file system, for example. This was always the original design intent, but I screwed up and no one noticed until Jann reached out to be and said, "Hey this doesn't seem to make much sense". Regards, - Ted
Re: [PATCH] crypto: DRBG - guard uninstantion by lock
On Sun, Apr 08, 2018 at 09:07:03PM +0200, Stephan Müller wrote: > Can you please check whether the attached patch fixes the issue? > Stephan, FYI, if you incude in your e-mail "#syz test " as the first line of your patch and the syzbot e-mail is cc'ed, the syzbot will automatically apply the patch in the e-mail against the git tree/branch specified in the "#syz test" line, and then try to see if the problem it discovered still reproduces --- and then send you e-mail one way or another. So the syzbot will run while the patch goes through the normal e-mail review process, which is kind of neat. :-) Cheers, - Ted
