Summary: | Geli arbitrarily prevents setting passphrases | ||
---|---|---|---|
Product: | Base System | Reporter: | Frank <fhriley> |
Component: | kern | Assignee: | freebsd-geom (Nobody) <geom> |
Status: | Closed FIXED | ||
Severity: | Affects Many People | CC: | allanjude, cem, mav |
Priority: | --- | ||
Version: | CURRENT | ||
Hardware: | Any | ||
OS: | Any | ||
See Also: |
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=134113 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=196834 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=208305 |
Description
Frank
2017-04-09 19:39:36 UTC
Git pull request: https://github.com/freebsd/freebsd/pull/109 I discussed this recently with pjd@ (original author of GELI). Our current plan is to extend the metadata for GELI, in order to enable a number of other new features, and make a separate slot for the iterations # for each of the passphrases. However, that won't be ready in time for 11.1, and this might be, so I'll start reviewing this as a workaround until we can extend the GELI metadata. (In reply to Allan Jude from comment #2) Agreed that extending the metadata and having iterations per slot is the correct solution. I'd be interested in the design if it is shared somewhere. (In reply to Frank from comment #3) We are still in the process of coming up with the design. There is a working group about it at the BSDCan FreeBSD Developers Summit (June 7-8 in Ottawa, Canada). I plan to have something resembling an initial design doc well ahead of that so people can start thinking about it and we can have meaningful discussions at the devsummit. Over the weekend I created alternative much more simple patch for this issue: https://reviews.freebsd.org/D10338 It does not provide all the same flexibility for changing number of iterations when two keys are present, but honestly I don't think it is so much needed. The main issue fixed by this patch is failing implicit set of number iteration, when -i parameters was not even specified. If this area is going to be rewritten at some point, I think my much more simple and non-invasive patch can be more applicable meanwhile. A commit references this bug: Author: mav Date: Fri Apr 21 07:16:08 UTC 2017 New revision: 317246 URL: https://svnweb.freebsd.org/changeset/base/317246 Log: Always allow setting number of iterations for the first time. Before this change it was impossible to set number of PKCS#5v2 iterations, required to set passphrase, if it has two keys and never had any passphrase. Due to present metadata format limitations there are still cases when number of iterations can not be changed, but now it works in cases when it can. PR: 218512 MFC after: 2 weeks Sponsored by: iXsystems, Inc. Differential Revision: https://reviews.freebsd.org/D10338 Changes: head/sbin/geom/class/eli/geom_eli.c head/sys/geom/eli/g_eli_ctl.c With the lack of feedback I decided to commit my change as much more simple and sufficient for most cases. If there is still wish to improve metadata format to make ELI more functional -- welcome, but that is a separate and IMO less critical task now. (In reply to Alexander Motin from comment #7) Note that the committed solution can lead to non-deterministic failures since when -i is not given by the user, geli will compute the iterations. The computed iterations will depend on the state of the system (busy, not busy, etc.) The committed solution has nothing to do with "non-deterministic failures". It just fixes the present code as it supposed to work. For those "paranoic" who need full determinism, last moment modification I made should allow specifying -i value which is already set, and it won't be an error. Just choose your ove favourite value and specify it every time, if you wish. (In reply to Alexander Motin from comment #9) It's not paranoia at all. If you don't specify -i, the iterations are not set to -1, they are computed and passed in. When setting passphrases on a set of disks, freenas serializes that process, but what if I wanted to parallelize it? geli is happily computing iterations on the cpus when another task happens to come in. One or more of the iterations end up getting computed differently that what it is currently set to and subsequently causing one or more passphrase changes to fail. This is but one of many scenarios of how this could really happen. Of course, I thought of all this when I made my changes, and I added tests to test those changes. But I suppose it's the state of software these days to make the easy fix for the specific use case I care about. This will be my last comment on this as I am not a committer to freebsd, but if I were, I would label the commit as incomplete. (In reply to Frank from comment #10) We will be working on extended the GELI metadata to be able to support holding two different numbers of iterations, to properly solve this problem going forward. I am not sure what to do in the mean time, if that means blocking a set key operation with an explicit -i, as there may be an existing key using the already stored number of iterations, and changing the value in the metadata will make the existing key unusable. Or if we allow the new key to be set, and do not store the number of iterations, they key is unusable. (In reply to Allan Jude from comment #11) In my pull request here https://github.com/freebsd/freebsd/pull/109, I fixed all of the problems. The only things left that it prevents the user from doing are changing the iterations if both keys have passphrases or setting a second passphrase with different iterations than the first passphrase. Everything else works. I definitely agree that a redesign of the metadata with iterations per slot is the correct solution, but in the meantime my goal was to make it as usable as possible with the existing metadata until that redesign happens. (In reply to Frank from comment #10) "One or more of the iterations end up getting computed differently that what it is currently set to and subsequently causing one or more passphrase changes to fail." -- Sorry, I don't see logic in this statement. You several times mentioned this scenario as source of problems, but I don't see any. Multiple processes running same time indeed likely cause different number of iterations to be set on different disks. And so what? The only thing number of iterations affect is a strength of password protection against brute force attack. And it should in no way cause any errors reported, since number of iterations is generated only once and then used forever, unless forced later with -i key, which IMO you should not do without very good reason. (In reply to Alexander Motin from comment #13) "Multiple processes running same time indeed likely cause different number of iterations to be set on different disks." If you recognize that, you should see the problem. Say we have three disks, you set passphrases on them and they end up with the following iterations: 1: 10 2: 11 3: 10 Now you come along and want to change the passphrase (without using -i), and this time it computes the iterations as so: 1: 11 2: 10 3: 10 With the current code and your commit, disks 1 & 2 will fail because it sees you trying to change the iterations. Here is your if statement: if (*valp != -1 && md.md_iterations == -1) { md.md_iterations = *valp; } else if (*valp != -1 && *valp != md.md_iterations) { /* fail */ } For disk 1 above in setting the passphrase the second time: *valp == 11 md_md_iterations == 10 This is fixed in my pull request. Changing passphrase without -i should not change number of iterations, so there should be no problem: /* * Field md_iterations equal to -1 means "choose some sane * value for me". */ if (md->md_iterations == -1) { assert(new); if (verbose) printf("Calculating number of iterations...\n"); md->md_iterations = pkcs5v2_calculate(2000000); assert(md->md_iterations > 0); if (verbose) { printf("Done, using %d iterations.\n", md->md_iterations); } } If md_iterations was ever set, the same value should be used for that disk forever. (In reply to Alexander Motin from comment #15) I stand corrected, you are correct. For some reason I was thinking it would be -1 there. I am ok with closing this bug as mostly fixed. It should be sufficient until the metadata redesign fixes the remaining cases. A commit references this bug: Author: mav Date: Sat May 6 00:50:25 UTC 2017 New revision: 317858 URL: https://svnweb.freebsd.org/changeset/base/317858 Log: MFC r317246: Always allow setting number of iterations for the first time. Before this change it was impossible to set number of PKCS#5v2 iterations, required to set passphrase, if it has two keys and never had any passphrase. Due to present metadata format limitations there are still cases when number of iterations can not be changed, but now it works in cases when it can. PR: 218512 Sponsored by: iXsystems, Inc. Changes: _U stable/11/ stable/11/sbin/geom/class/eli/geom_eli.c stable/11/sys/geom/eli/g_eli_ctl.c |