From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1Z7iox-0001lm-Vo for mharc-grub-devel@gnu.org; Wed, 24 Jun 2015 07:26:23 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52136) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7iou-0001lF-1P for grub-devel@gnu.org; Wed, 24 Jun 2015 07:26:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7ioq-0002Gz-0W for grub-devel@gnu.org; Wed, 24 Jun 2015 07:26:19 -0400 Received: from johnlane.plus.com ([212.159.104.145]:60447 helo=sodium.amajohn.co.uk) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7iop-0002GT-Li for grub-devel@gnu.org; Wed, 24 Jun 2015 07:26:15 -0400 Received: by sodium.amajohn.co.uk (Postfix, from userid 1000) id B67914A; Wed, 24 Jun 2015 12:26:13 +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 B55AD40; Wed, 24 Jun 2015 12:26:12 +0100 (BST) Message-ID: <558A93D3.4070304@jelmail.com> Date: Wed, 24 Jun 2015 12:26:11 +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> 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: Wed, 24 Jun 2015 11:26:22 -0000 On 24/06/15 07:59, Andrei Borzenkov wrote: > On Tue, Jun 23, 2015 at 8:27 PM, John Lane wrote: >>>> - err =3D cr->recover_key (source, dev, hdr); >>>> + err =3D cr->recover_key (source, dev, hdr, key, keyfile_size); >>> You never clear key variable, so after the first call all subsequent >>> invocations will always use it for any device. Moving to proper >>> callback data would make such errors less likely. >> It is cleared when the arguments are processed, specifically >> "grub_cmd_cryptomount" sets "key =3D NULL". > Right, missed it, sorry. > >>>> + keyfile_size =3D state[7].set ? grub_strtoul (state[7].arg, 0= , >> 0) : \ >>>> + GRUB_CRYPTODISK_MAX_KEYFILE_SI= ZE; >>>> + > This must check keyfile_size, otherwise it meand buffer overwrite. I'll add in a check for this. > >>>> + keyfile =3D grub_file_open (state[4].arg); >>>> + if (!keyfile) >>>> + return grub_errno; >>>> + >>>> + if (grub_file_seek (keyfile, keyfile_offset) =3D=3D (grub_off= _t)-1) >>>> + return grub_errno; >>>> + >>>> + 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 should = be >>> considered an error. Otherwise what is the point of explicitly giving= >>> size? >> A short read is accepted by the upstream "cryptsetup" tool. Its man pa= ge >> 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 there > 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. 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). This argument allows a passphrase length to be given. My cryptsetup version is 1.6.6 and it says what I wrote above, which is also reflected in the current upstream head: https://gitlab.com/cryptsetup/cryptsetup/blob/master/man/cryptsetup.8#L63= 5 > 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 does= =2E >>>> - 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 use= r >>> space is to ask for password then. If keyfile fails we cannot continu= e >>> 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. So > asking user for passphrase seems logical here. > I'll change it so that it falls back to passphrase if it cannot open the key file. This is in cryptodisk.c. Should it also fall back to passphrase on other errors (seek and read) ? The behaviour that I copied from elsewhere in the code was to exit with an error when these things happen. Here's the relevant (revised) snippet: keyfile =3D grub_file_open (state[4].arg); if (keyfile) { =20 if (grub_file_seek (keyfile, keyfile_offset) =3D=3D (grub_off_t= )-1) return grub_errno; key =3D keyfile_buffer; keyfile_size =3D grub_file_read (keyfile, key, keyfile_size); if (keyfile_size =3D=3D (grub_size_t)-1) return grub_errno; } =20 else grub_printf (N_("Unable to open key file %s\n"), state[4].arg); You can see it errors on seek and read but will continue if the open fails. "Continue" means return to the grub prompt. Most commands seem to return to the prompt on error.