From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 04DD5C433F5 for ; Fri, 10 Dec 2021 00:15:15 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C8A6582F7E; Fri, 10 Dec 2021 01:15:12 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="M7iwlI2u"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E136782F7E; Fri, 10 Dec 2021 01:15:10 +0100 (CET) Received: from mail-ua1-x934.google.com (mail-ua1-x934.google.com [IPv6:2607:f8b0:4864:20::934]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 06C18811D8 for ; Fri, 10 Dec 2021 01:15:05 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-ua1-x934.google.com with SMTP id o1so14011151uap.4 for ; Thu, 09 Dec 2021 16:15:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=uRTqwKtZFvpOWi0v0BBP+fyWGFbRLtO8VyQTODkeJMA=; b=M7iwlI2uDw+o66QUgfTeAh/hqmRNSFT4yyjRiR4K6cmmW+RVBCZxoIlvv7OjNqCLKa tq2gNPyqNU/w5eULsGs9GKcNY9k5ykhM58zQ0ijSF7Ty81yKsk1/+qPnSi2rv3nQ+VYI N+NjIX1F6GTsI2Puyt5ZiCdICbKTiRT2OmSTA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=uRTqwKtZFvpOWi0v0BBP+fyWGFbRLtO8VyQTODkeJMA=; b=FSM900Dn/lgReRwmgCXHies466GF2v/DXz35cOZH+cpzlspqsGRCWr20fczYQ6K7a7 gCOMPLvZ7/BHswV/XiXZwMb6eATAvi42QFA0stGU8g5vGGBuJBaYJy4F+3/30oTGbt1z 4lDyN6Oudm3cLq6u//Tyul+ZvD5gjZ/dz8Y0gf3x9G9sBV/LO/p8BQIBBZQ0RjMk8uBs rf/VHsTqocz69BHCCoF8wTmSqKvzIz8+YGi1sjhh8F7xZIKvU2aw9ZS+sdaMrvLtGjh/ cdu4I4zHa8rLprdd2XDRpg2MBUEkknoj5B4xFkTo6k46kN6GdnIxdXJ2wjNU8NgpHo0m lmtw== X-Gm-Message-State: AOAM533oj03w3hcPzJSXQK0rpe5BM7oRK3Je7tBer0f8qCPfv4bnMulk jJGd89Lcp899MtNmZ/iZwTMeghao9340nPOVCk012Q== X-Google-Smtp-Source: ABdhPJxucNr0F/xYtQbmst5gM2jU8phNC2uYV9lezDEj2fKteUkd5ZBr1Ykyn/D8cO6nj7ICtr5IG2w8fcjRJsYsK/8= X-Received: by 2002:ab0:4465:: with SMTP id m92mr24410757uam.47.1639095304401; Thu, 09 Dec 2021 16:15:04 -0800 (PST) MIME-Version: 1.0 References: <20211117175215.24262-1-philippe.reynes@softathome.com> <20211117175215.24262-9-philippe.reynes@softathome.com> <38d15e93-d7bd-aabb-230e-0de702ca996a@softathome.com> In-Reply-To: <38d15e93-d7bd-aabb-230e-0de702ca996a@softathome.com> From: Simon Glass Date: Thu, 9 Dec 2021 17:14:52 -0700 Message-ID: Subject: Re: [RFC PATCH v3 8/8] tools: gen_pre_load_header.sh: initial import To: Philippe REYNES Cc: Rasmus Villemoes , mr.nuke.me@gmail.com, joel.peshkin@broadcom.com, u-boot@lists.denx.de Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.38 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean Hi, On Wed, 8 Dec 2021 at 11:10, Philippe REYNES wrote: > > Hi Rasmus, > > First, thanks for the feedback. > > > Le 06/12/2021 =C3=A0 09:23, Rasmus Villemoes a =C3=A9crit : > > On 17/11/2021 18.52, Philippe Reynes wrote: > >> This commit adds a script gen_pre_load_header.sh > >> that generate the header used by the image pre-load > >> stage. > >> > >> Signed-off-by: Philippe Reynes > >> --- > >> tools/gen_pre_load_header.sh | 174 +++++++++++++++++++++++++++++++++= ++ > >> 1 file changed, 174 insertions(+) > >> create mode 100755 tools/gen_pre_load_header.sh > >> > >> diff --git a/tools/gen_pre_load_header.sh b/tools/gen_pre_load_header.= sh > >> new file mode 100755 > >> index 0000000000..8256fa80ee > >> --- /dev/null > >> +++ b/tools/gen_pre_load_header.sh > >> @@ -0,0 +1,174 @@ > >> +#!/bin/bash > >> +# SPDX-License-Identifier: GPL-2.0+ > >> + > >> +# > >> +# default value > >> +# > >> +size=3D'4096' > >> +algo=3D'sha256,rsa2048' > >> +padding=3D'pkcs-1.5' > >> +key=3D'' > >> +verbose=3D'false' > >> +input=3D'' > >> +output=3D'' > >> + > >> +usage() { > >> + printf "Usage: $0 -a -k [-p ] [-s ] [= -v] -i -o \n" > >> +} > >> + > >> +# > >> +# parse arguments > >> +# > >> +while getopts 'a:hi:k:o:p:s:v' flag; do > >> + case "${flag}" in > >> + a) algo=3D"${OPTARG}" ;; > >> + h) usage > >> + exit 0 ;; > >> + i) input=3D"${OPTARG}" ;; > >> + k) key=3D"${OPTARG}" ;; > >> + o) output=3D"${OPTARG}" ;; > >> + p) padding=3D"${OPTARG}" ;; > >> + s) size=3D"${OPTARG}" ;; > >> + v) verbose=3D'true' ;; > >> + *) usage > >> + exit 1 ;; > >> + esac > >> +done > >> + > >> +# > >> +# check that mandatory arguments are provided > >> +# > >> +if [ -z "$key" -o -z "$input" -o -z "$output" ] > >> +then > >> + usage > >> + exit 0 > >> +fi > >> + > >> +hash=3D$(echo $algo | cut -d',' -f1) > >> +sign=3D$(echo $algo | cut -d',' -f2) > >> + > >> +echo "status:" > >> +echo "size =3D $size" > >> +echo "algo =3D $algo" > >> +echo "hash =3D $hash" > >> +echo "sign =3D $sign" > >> +echo "padding =3D $padding" > >> +echo "key =3D $key" > >> +echo "verbose =3D $verbose" > >> + > >> +# > >> +# check if input file exist > >> +# > >> +if [ ! -f "$input" ] > >> +then > >> + echo "Error: file '$input' doesn't exist" > >> + exit 1 > >> +fi > >> + > >> +# > >> +# check if output is not empty > >> +# > >> +if [ -z "$output" ] > >> +then > >> + echo "Error: output is empty" > >> + exit 1 > >> +fi > >> + > >> +# > >> +# check that size is bigger than 0 > >> +# > >> +if [ $size -le 0 ] > >> +then > >> + echo "Error: $size lower than 0" > >> + exit 1 > >> +fi > >> + > >> +# > >> +# check if the key file exist > >> +# > >> +if [ ! -f "$key" ] > >> +then > >> + echo "Error: file $key doesn't exist\n" > >> + exit 1 > >> +fi > >> + > >> +# > >> +# check if the hash is valid and supported > >> +# > >> +print_supported_hash() { > >> + echo "Supported hash:" > >> + echo "- sha1" > >> + echo "- sha256" > >> + echo "- sha384" > >> + echo "- sha512" > >> +} > >> + > >> +case "$hash" in > >> + "sha1") hashOption=3D"-sha1" ;; > >> + "sha256") hashOption=3D"-sha256" ;; > >> + "sha384") hashOption=3D"-sha384" ;; > >> + "sha512") hashOption=3D"-sha512" ;; > >> + *) echo "Error: $hash is an invalid hash" > >> + print_supported_hash > >> + exit 1;; > >> +esac > >> + > >> +# > >> +# check if the sign is valid and supported > >> +# > >> +print_supported_sign() { > >> + echo "Supported sign:" > >> + echo "- rsa1024" > >> + echo "- rsa2048" > >> + echo "- rsa4096" > >> +} > >> + > >> +case "$sign" in > >> + "rsa1024") ;; > >> + "rsa2048") ;; > >> + "rsa4096") ;; > >> + *) echo "Error: $sign is an invalid signature type" > >> + print_supported_sign > >> + exit 1;; > >> +esac > >> + > >> +# > >> +# check if the padding is valid and supported > >> +# > >> +print_supported_padding() { > >> + echo "Supported padding:" > >> + echo "- pkcs-1.5" > >> + echo "- pss" > >> +} > >> + > >> +case "$padding" in > >> + "pkcs-1.5") optionPadding=3D'' ;; > >> + "pss") optionPadding=3D'-sigopt rsa_padding_mode:pss -sigopt rsa_= pss_saltlen:-2' ;; > >> + *) echo "Error: $padding is an invalid padding" > >> + print_supported_padding > >> + exit 1;; > >> +esac > >> + > >> + > >> +# > >> +# generate the sigature > >> +# > >> +sig=3D$(openssl dgst $optionHash -sign $key $optionPadding $input | x= xd -p) > >> + > >> +# > >> +# generate the header > >> +# > >> +# 0 =3D magic > >> +# 4 =3D image size > >> +# 8 =3D signature > >> +# > >> +h=3D$(printf "%08x" 0x55425348) > >> +i=3D$(stat --printf=3D"%s" $input) > >> +i=3D$(printf "%08x" $i) > >> + > >> +echo "$h$i$sig" | xxd -r -p > $output > > So this sounds like a completely generic way of prepending a signature > > to an arbitrary blob, whether that is a FIT image, a U-Boot script > > wrapped in a FIT, some firmware blob or whatnot. So it sounds like it > > could be generally useful, and a lot simpler than the complexity > > inherent when trying to add signature data within the signed data > > structure itself. > We plan to use it with FIT, but it is very generic and may be used > with any firmware. > > So, can we perhaps not tie it to bootm as such? It's not a problem if > > bootm learns to recognize 0x55425348 as another image format and then > > automatically knows how to locate the "real" image, and/or automaticall= y > > verifies it. But I'd really like to be able to > > > > fatload $loadaddr blabla && \ > > verify $loadaddr && \ > > source $loadaddr > > > > where fatload can be any random command that gets a bunch of bytes into > > memory at a specific location (tftpboot, mmc read, ubi...). Currently, > > we simply don't have any sane way to verify a boot script, or random > > blobs, AFAICT. > > > Based on this header, it is quite easy to develop a command verify. > It wasn't planned but it is a very good idea. I will add it, in the next > version or in another series a bit after. > > > > To that end, it would be nice if the header was a little more > > self-describing. Something like > > > > 0 =3D magic > > 4 =3D header size (including padding) > > 8 =3D image size > > 12 =3D offset to image signature > > 16 =3D flags (currently enforced to 0) > > 20 =3D reserved (currently enforced to 0) > > 24 =3D signature of first 24 bytes > > > > xx =3D signature of image > > > > Why do I want the image size signed? Because I'd like to be able to > > store the whole thing in a raw partition (or ubi volume, or...), where > > there's no concept of "file size" available. So I'd like to be able to > > read in some fixed size chunk (24+whatever I expect the signature could > > be, so 4096 is certainly enough), and from that compute the whole size = I > > need to read. But I don't want to blindly trust the "image size" field. > > So, for such a case, I'd also like a "verify header $loadaddr" > > subcommand (and "verify image $loadaddr", with "verify $loadaddr" being > > shorthand for doing both). > I understand why you want to add a signature for the header and I agree. > > But if we add a signature for the header, I think that we should > sign everything (even the signature) or include a hash of the > image signature in the header. > > So I would suggest something like: > 0 =3D magic > 4 =3D header size (including padding) > 8 =3D image size > 12 =3D offset to image signature > 16 =3D version of the header > 20 =3D flags (currently enforced to 0) > 24 =3D reserved (currently enforced to 0) > 28 =3D reserved (currently enforced to 0) > 32 =3D sha256 of the signature > 64 =3D signature of the first 64 bytes > .. > xx =3D signature of the image > > Another solution would be to keep the header size in the u-boot device > tree and > add the signature of the header at the end of the header. > It would become something like: > 0 =3D magic > 4 =3D image size > 8 =3D offset to image signature > 12 =3D version of the header > 16 =3D flags (currently enforced to 0) > 20 =3D reserved (currently enforced to 0) > 24 =3D reserved (currently enforced to 0) > .. > xx =3D signature of the image > .. > header_size - sig_size =3D signature of the header (without the header > signature) > > As you can see I also propose to add the header version. > I prefer the second solution. > > > Everybody agrees with this proposal ? So long as this is not a shell script and is done with a binman entry, yes. But why not use struct image_header, if we are going to have this as an ad-hoc thing? Regards, Simon > > > > > > And continuing the wishlist, it could be even better if we had > > > > verify load $loadaddr 'mmc read %l% 0 %s512%' > > > > i.e. we could pass a "parametrized shell command" to verify for it to > > use to read in a bunch of bytes to a given address - with %l% being > > substituted by the address and %s% by the size to load, > > optionally specified in the given unit. > > I agree, it would be nice. But I think it could be done in a second step. > > > > > Rasmus > > > Regards, > Philippe > > > -- This message and any attachments herein are confidential, intended sol= ely for the addressees and are SoftAtHome=E2=80=99s ownership. Any unauthor= ized use or dissemination is prohibited. If you are not the intended addres= see of this message, please cancel it immediately and inform the sender.