From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1oRuaN-0008E6-Rt for mharc-grub-devel@gnu.org; Sat, 27 Aug 2022 08:06:47 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:46248) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oRuaN-0008Dt-28 for grub-devel@gnu.org; Sat, 27 Aug 2022 08:06:47 -0400 Received: from mail-40136.proton.ch ([185.70.40.136]:61581) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oRuaI-0007xs-Vr for grub-devel@gnu.org; Sat, 27 Aug 2022 08:06:46 -0400 Date: Sat, 27 Aug 2022 12:06:30 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fomin.one; s=protonmail; t=1661601998; x=1661861198; bh=X1xmNd3zi200gwdzYeQBxVgI1rfA8s5bpWH+rbAVmx8=; h=Date:To:From:Cc:Reply-To:Subject:Message-ID:In-Reply-To: References:Feedback-ID:From:To:Cc:Date:Subject:Reply-To: Feedback-ID:Message-ID; b=EstswgWRY6KL9TS8y0XimFwfWwztt0rMuUMb1XFCPbAbeQvIlV8sxOfbLFZOsUgWY 6WpubKJ0wYIPxOv8V6CZFcHx0z1dT9Cj4LiUgTT/dF4RQVpZcv/7GIoT9cqOVN+5pB 5PU2Tes87ebIfgubYNr8qpXMRKOd3MlpjEMJLp7rR/JovAscXfEEhAeO6iHSd50+Po 7lQf8CYWKeUmLScvUcXLOSVzOQ3t5fvRNyvDC1i488Z0t6ImUB90De1v7lmrISWbZM Nd2Q8NL/PZSU1Iu0GQ+W/Ea5Srkj6iQEo2KhLDwI9cyCshriNMBNyOx8Y1o5xddDoZ vUcJVaBbpRbkA== To: "grub-devel@gnu.org" From: Maxim Fomin Cc: "development@efficientek.com" , "ps@pks.im" Reply-To: Maxim Fomin Subject: Re: [PATCH v6 1/1] plainmount: Support plain encryption mode Message-ID: In-Reply-To: <20220823011417.2598923f@crass-HP-ZBook-15-G2> References: <20220823011417.2598923f@crass-HP-ZBook-15-G2> Feedback-ID: 10594471:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=185.70.40.136; envelope-from=maxim@fomin.one; helo=mail-40136.proton.ch X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 27 Aug 2022 12:06:47 -0000 ------- Original Message ------- On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn wrote: > > +/ Hashes a password into a key and stores it with cipher. / > > +static grub_uint8_t > > +plainmount_configure_password (grub_cryptodisk_t dev, const char *hash= , > > + grub_uint8_t *key_data, grub_size_t key_size) > > > Why does the return type changed from v5? I like it was better before, > and I'm thinking the signature should be more like hash() in > cryptsetup, that is have password and password_size arguments, to get > rid of the non-NULL byte assumption. Moving the password asking code > out of this function is fine though. I changed signature because configure_password() modifies password data sen= t from the caller. It does so in a such way, that new pointer must be returned (that's what I = was thinking when changing function signature). This does not happen with the configure_keyfi= le() because it does not modify the buffer. So, moving the call to setkey() into the main f= unc (out from configure_password() and configure_keyfile()) required changing one of the = function signatures. I will reconsider this issue to make signatures look like hash() in cryptse= tup and also will check the issue with NULL byte. > > > + grub_printf_ (N_("warning: hash is ignored if keyfile is specified\n"= )); > > + > > + /* Warn if -p option is specified with keyfile / > > + if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set) > > + grub_printf_ (N_("warning: password specified with -p option " > > + "is ignored if keyfile is provided\n")); > > + > > + / Warn of -O is provided without keyfile / > > + if (state[OPTION_KEYFILE_OFFSET].set && !state[OPTION_KEYFILE].set) > > + grub_printf_ (N_("warning: keyfile offset option -O " > > + "specified without keyfile option -d\n")); > > + > > + grub_dprintf ("plainmount", "parameters: cipher=3D%s, hash=3D%s, key_= size=3D%" > > + PRIuGRUB_SIZE", keyfile=3D%s, keyfile offset=3D%"PRIuGRUB_SIZE"\n", > > + cipher, hash, key_size, keyfile, keyfile_offset); > > + > > + err =3D plainmount_configure_sectors (dev, disk, sector_size); > > + if (err !=3D GRUB_ERR_NONE) > > + goto exit; > > + > > + / Configure keyfile or password */ > > + if (keyfile !=3D NULL) > > + { > > + err =3D plainmount_configure_keyfile (keyfile, key_data, key_size, ke= yfile_offset); > > + if (err !=3D GRUB_ERR_NONE) > > + goto exit; > > + err =3D plainmount_setkey (dev, key_data, key_size); > > + if (err !=3D GRUB_ERR_NONE) > > + goto exit; > > + } > > + else > > + { > > + hashed_key_data =3D plainmount_configure_password (dev, hash, key_dat= a, key_size); > > > It looks like you're limiting key_data, which could be a string from > -p, to key_size. The cryptsetup code does not appear to do this, so I > think this does not work for passwords longer than the hash length. In one of the old versions of the patch I removed the option which allowed = to set key size from password or keyfile. I considered it is strange to specify key size op= tion (-s 512, for example) and then implicitly specify different key size from password, = hash or keyfile argument. I think it was that version of the patch where you pointed on pos= sible buffer overflow attack because of this feature. Also, I am confused about maximum key data and password size allowed by cry= ptomount.h. It limits GRUB_CRYPTODISK_MAX_KEYLEN to 128, GRUB_CRYPTODISK_MAX_PASSPHRASE to= 256 and GRUB_CRYPTODISK_MAX_KEYFILE_SIZE to 8192. So, it looks like the passphrase = (before hashing) is limited to 256 bytes, when it is hashed - it is limited to hash size, wh= ich cannot be larger than 128 bytes. Why then GRUB_CRYPTODISK_MAX_KEYFILE_SIZE is limited= to 8192 bytes (or bits?)? Also, if password is not hashed (-h plain) then 129 byte passwo= rd becomes illegal, because it is used directly as a key, which is limited to 128 bytes. Am I c= orrect? > > + if (hashed_key_data =3D=3D NULL) > > + goto exit; > > + err =3D plainmount_setkey (dev, hashed_key_data, key_size); > > + if (err !=3D GRUB_ERR_NONE) > > + { > > + grub_free (hashed_key_data); > > + goto exit; > > + } > > + } > > > I was hoping that when moving plainmount_setkey() out of the > plainmount_configure_*() functions that it could be only called once in > the code, instead of twice as done here. Why can't we refactor and have > this code here: > > err =3D plainmount_setkey (dev, key_data, key_size); > if (err !=3D GRUB_ERR_NONE) > goto exit; > > Glenn I will check this in the next version (see my comment above about changing = data buffer in one of configure_*() function explaining why I changed the function sign= ature).