Hi,
On Wed, 8 Dec 2021 at 11:10, Philippe REYNES
<philippe.rey...@softathome.com> wrote:
>
> Hi Rasmus,
>
> First, thanks for the feedback.
>
>
> Le 06/12/2021 à 09:23, Rasmus Villemoes a écrit :
> > 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 <philippe.rey...@softathome.com>
> >> ---
> >> 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='4096'
> >> +algo='sha256,rsa2048'
> >> +padding='pkcs-1.5'
> >> +key=''
> >> +verbose='false'
> >> +input=''
> >> +output=''
> >> +
> >> +usage() {
> >> + printf "Usage: $0 -a <algo> -k <key> [-p <padding>] [-s <size>] [-v]
> >> -i <input> -o <output>\n"
> >> +}
> >> +
> >> +#
> >> +# parse arguments
> >> +#
> >> +while getopts 'a:hi:k:o:p:s:v' flag; do
> >> + case "${flag}" in
> >> + a) algo="${OPTARG}" ;;
> >> + h) usage
> >> + exit 0 ;;
> >> + i) input="${OPTARG}" ;;
> >> + k) key="${OPTARG}" ;;
> >> + o) output="${OPTARG}" ;;
> >> + p) padding="${OPTARG}" ;;
> >> + s) size="${OPTARG}" ;;
> >> + v) verbose='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=$(echo $algo | cut -d',' -f1)
> >> +sign=$(echo $algo | cut -d',' -f2)
> >> +
> >> +echo "status:"
> >> +echo "size = $size"
> >> +echo "algo = $algo"
> >> +echo "hash = $hash"
> >> +echo "sign = $sign"
> >> +echo "padding = $padding"
> >> +echo "key = $key"
> >> +echo "verbose = $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="-sha1" ;;
> >> + "sha256") hashOption="-sha256" ;;
> >> + "sha384") hashOption="-sha384" ;;
> >> + "sha512") hashOption="-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='' ;;
> >> + "pss") optionPadding='-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=$(openssl dgst $optionHash -sign $key $optionPadding $input | xxd -p)
> >> +
> >> +#
> >> +# generate the header
> >> +#
> >> +# 0 = magic
> >> +# 4 = image size
> >> +# 8 = signature
> >> +#
> >> +h=$(printf "%08x" 0x55425348)
> >> +i=$(stat --printf="%s" $input)
> >> +i=$(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 automatically
> > 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 = magic
> > 4 = header size (including padding)
> > 8 = image size
> > 12 = offset to image signature
> > 16 = flags (currently enforced to 0)
> > 20 = reserved (currently enforced to 0)
> > 24 = signature of first 24 bytes
> >
> > xx = 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 = magic
> 4 = header size (including padding)
> 8 = image size
> 12 = offset to image signature
> 16 = version of the header
> 20 = flags (currently enforced to 0)
> 24 = reserved (currently enforced to 0)
> 28 = reserved (currently enforced to 0)
> 32 = sha256 of the signature
> 64 = signature of the first 64 bytes
> ..
> xx = 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 = magic
> 4 = image size
> 8 = offset to image signature
> 12 = version of the header
> 16 = flags (currently enforced to 0)
> 20 = reserved (currently enforced to 0)
> 24 = reserved (currently enforced to 0)
> ..
> xx = signature of the image
> ..
> header_size - sig_size = 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<optional unit>% 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 solely
> for the addressees and are SoftAtHome’s ownership. Any unauthorized use or
> dissemination is prohibited. If you are not the intended addressee of this
> message, please cancel it immediately and inform the sender.
