From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1Z8DPx-00066q-N8 for mharc-grub-devel@gnu.org; Thu, 25 Jun 2015 16:06:37 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56621) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8DPu-00062z-68 for grub-devel@gnu.org; Thu, 25 Jun 2015 16:06:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z8DPo-0000sk-W5 for grub-devel@gnu.org; Thu, 25 Jun 2015 16:06:34 -0400 Received: from johnlane.plus.com ([212.159.104.145]:61186 helo=sodium.amajohn.co.uk) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8DPo-0000sG-Lb for grub-devel@gnu.org; Thu, 25 Jun 2015 16:06:28 -0400 Received: by sodium.amajohn.co.uk (Postfix, from userid 1000) id 8D10828; Thu, 25 Jun 2015 21:06:25 +0100 (BST) Received: from [10.0.200.1] (hydrogen.amajohn.co.uk [10.0.200.1]) by sodium.amajohn.co.uk (Postfix) with ESMTPSA id A533228; Thu, 25 Jun 2015 21:06:24 +0100 (BST) Message-ID: <558C5F3F.4040307@jelmail.com> Date: Thu, 25 Jun 2015 21:06:23 +0100 From: John Lane User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.2 MIME-Version: 1.0 To: Andrei Borzenkov Subject: Re: [PATCH 2/4] Cryptomount support key files References: <1434445875-6846-1-git-send-email-john@lane.uk.net> <1434445875-6846-3-git-send-email-john@lane.uk.net> <20150620075404.1efc15d5@opensuse.site> <5589971C.3070207@jelmail.com> <558A93D3.4070304@jelmail.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Outbound-Checked: Yes X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 212.159.104.145 Cc: The development of GNU GRUB X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jun 2015 20:06:36 -0000 On 24/06/15 13:02, Andrei Borzenkov wrote: > On Wed, Jun 24, 2015 at 2:26 PM, John Lane wrote: >>>>>> + >>>>>> + keyfile_size =3D grub_file_read (keyfile, key, keyfile_size= ); >>>>>> + if (keyfile_size =3D=3D (grub_size_t)-1) >>>>>> + return grub_errno; >>>>> If keyfile size is explicitly given, I expect that short read shoul= d be >>>>> considered an error. Otherwise what is the point of explicitly givi= ng >>>>> size? >>>> A short read is accepted by the upstream "cryptsetup" tool. Its man = page >>>> describes its "--keyfile-size" argument as "Read a maximum of value >>>> bytes from the key file. Default is to read the whole file up to the= >>>> compiled-in maximum. The cryptomount command follows that rule. >>> It is not what my version of cryptsetup says. >>> >>> >From a key file: It will be cropped to the size given by -s. If ther= e >>> is insufficient key material in the key file, cryptsetup will quit >>> with an error. >> This is equivalent to the "-l" or "--keyfile-size" option, not the "-s= " >> or '--key-size' option. >> > Ah, right. Cryptography must be confusing, otherwise it is not secret := ) > >> It isn't the number of bytes in the key; it's the maximum number of >> bytes that is read from the key file. For LUKS the key file contains a= >> passphrase which is then used to derive the key (pbkdf2 function). Thi= s >> argument allows a passphrase length to be given. >> >> My cryptsetup version is 1.6.6 and it says what I wrote above, which i= s >> also reflected in the current upstream head: >> >> https://gitlab.com/cryptsetup/cryptsetup/blob/master/man/cryptsetup.8#= L635 > I do not unterpret it your way. > >>> Which is logical. If I state that I want to have N bytes in a key, >>> getting less is error. >> I can see your logic but I was just following the convention set down = by >> cryptsetup. >> I can make it error if that's preferred but it isn't what cryptsetup d= oes. > Hmm ... > > https://gitlab.com/cryptsetup/cryptsetup/blob/master/lib/utils_crypt.c#= L446 > > if (!unlimited_read && i !=3D keyfile_size_max) { > log_err(cd, _("Cannot read requested amount of data.\n")); > goto out_err; > } > > So yes, it reads as much as it can - unless it is explicitly requested > to read specific amount of data. > >>>>>> - if (!grub_password_get (passphrase, MAX_PASSPHRASE)) >>>>>> + if (keyfile_bytes) >>>>>> { >>>>>> - grub_free (split_key); >>>>>> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not >>>> supplied"); >>>>>> + /* Use bytestring from key file as passphrase */ >>>>>> + passphrase =3D keyfile_bytes; >>>>>> + passphrase_length =3D keyfile_bytes_size; >>>>>> + } >>>>>> + else >>>>> I'm not sure if this should be "else". I think normal behavior of u= ser >>>>> space is to ask for password then. If keyfile fails we cannot conti= nue >>>>> anyway. >>>> Not sure I follow you. This is saying that the key file data should = be >>>> used if given ELSE ask the user for a passphrase. >>> What I mean - if user requested keyfile but keyfile could not be read= , >>> we probably should fallback to interactive password. >>> >>> I mostly think about scenario where keyfile is used to decrypt >>> /boot/grub; in this case we cannot continue if we cannot decrypt it >>> and we are in pre-normal environment where we cannot easily script. S= o >>> asking user for passphrase seems logical here. >>> >> I'll change it so that it falls back to passphrase if it cannot open t= he >> key file. This is in cryptodisk.c. >> Should it also fall back to passphrase on other errors (seek and read)= ? > I do not right now see reasons to differentiate. Either we could read > key or not.=20 OK, I've re-coded the keyfile bit to continue to passphrase on error, and being unable to read a specified keyfile size is considered an error now in addition to failure to open, seek or read. revised patch forthcoming... > Moreover, it pobably should fallback to pasphrase even if > key file is present but verification failed. We may add --silent or > similar if some use case emerges. How about making it so the user gets a number of attempts, so that a bad passphrase results in the passphrase prompt again (a bit like when you log in to a getty). The number of tries could be preset, say to 2. If a keyfile is given then that uses up one try. A bad keyfile would then result in a passphrase prompt. The way the existing code is written would fit that model easily without too much trouble. > >> The behaviour that I copied from elsewhere in the code was to exit wit= h >> an error when these things happen. >> > User using cryptsetup in full blown Unix shell has much better ways to > handle such errors; we do not. I meant in the Grub code ;)