Bug 218512

Summary: Geli arbitrarily prevents setting passphrases
Product: Base System Reporter: Frank <fhriley>
Component: kernAssignee: 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
In the geli metadata, there is one field that specifies the pkcs5v2 iterations, which means it used for both keys. Because of this, the code needs to prevent the user from setting a passphrase with a given (or calculated) iterations, and then setting a second passphrase with a different iterations. If it didn't, the first passphrase would get invalidated. The existing geli code does this, but in a naive way that leads to weird failures that, logically, should not fail, and drastically reduce the usability of geli. For example, the current code prevents the following:

  - Set two keys, then set a passphrase on one key
  - Set one key, then set a second key with passphrase using -i
  - Set one passphrase, then change the iterations
  
The first and second ones are especially bad because it means you have to reissue keys if you want to set password on an existing key (FreeNAS does this).

Also, if you set two keys with passphrases, geli will forever think a passphrase is set, even if you replace those two keys without passphrases, because the current code has no way to know if a passphrase is set on a key.

I am submitting a git pull request to fix all of the above.
Comment 1 Frank 2017-04-09 20:00:55 UTC
Git pull request:

https://github.com/freebsd/freebsd/pull/109
Comment 2 Allan Jude freebsd_committer freebsd_triage 2017-04-09 21:50:11 UTC
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.
Comment 3 Frank 2017-04-09 21:55:38 UTC
(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.
Comment 4 Allan Jude freebsd_committer freebsd_triage 2017-04-09 22:01:19 UTC
(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.
Comment 5 Alexander Motin freebsd_committer freebsd_triage 2017-04-10 07:03:25 UTC
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.
Comment 6 commit-hook freebsd_committer freebsd_triage 2017-04-21 07:17:05 UTC
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
Comment 7 Alexander Motin freebsd_committer freebsd_triage 2017-04-21 07:21:33 UTC
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.
Comment 8 Frank 2017-04-21 18:34:44 UTC
(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.)
Comment 9 Alexander Motin freebsd_committer freebsd_triage 2017-04-21 18:57:28 UTC
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.
Comment 10 Frank 2017-04-21 19:45:04 UTC
(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.
Comment 11 Allan Jude freebsd_committer freebsd_triage 2017-04-21 20:06:43 UTC
(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.
Comment 12 Frank 2017-04-21 20:13:22 UTC
(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.
Comment 13 Alexander Motin freebsd_committer freebsd_triage 2017-04-21 20:26:56 UTC
(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.
Comment 14 Frank 2017-04-21 20:38:19 UTC
(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.
Comment 15 Alexander Motin freebsd_committer freebsd_triage 2017-04-22 06:10:18 UTC
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.
Comment 16 Frank 2017-04-22 21:39:57 UTC
(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.
Comment 17 commit-hook freebsd_committer freebsd_triage 2017-05-06 00:51:08 UTC
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