Neha,
On 01/06/2022 12:48, Neha Malcom Francis wrote:
> Hi Roger,
>
> On 01/06/22 14:54, Roger Quadros wrote:
>>
>>
>> On 01/06/2022 09:02, Neha Malcom Francis wrote:
>>> Hi Roger,
>>>
>>> On 31/05/22 14:50, Roger Quadros wrote:
>>>>
>>>>
>>>> On 06/05/2022 07:37, Neha Malcom Francis wrote:
>>>>> K3 devices x509 certificate added to certain binaries that allows ROM to
>>>>
>>>> what binaries?
>>>>
>>>>> validate the integrity of the image. Etype that generates an x509
>>>>> certificate depending on boot flow added.
>>>>
>>>> Could you please explain in more detail as to what exactly is happening
>>>> here.
>>>>
>>>> What do you mean by "depending on boot flow"?
>>>>
>>>
>>> I will reformat the commit messages accordingly.
>>>>>
>>>>> Signed-off-by: Neha Malcom Francis <n-fran...@ti.com>
>>>>> ---
>>>>> tools/binman/entries.rst | 15 ++
>>>>> tools/binman/etype/x509_cert.py | 248 ++++++++++++++++++++++++++++
>>>>> tools/binman/ftest.py | 7 +
>>>>> tools/binman/test/232_x509_cert.dts | 18 ++
>>>>> tools/k3_gen_x509_cert.sh | 10 +-
>>>>> 5 files changed, 293 insertions(+), 5 deletions(-)
>>>>> create mode 100644 tools/binman/etype/x509_cert.py
>>>>> create mode 100644 tools/binman/test/232_x509_cert.dts
>>>>>
>>>>> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
>>>>> index 0c6d82fce8..dfa281e49f 100644
>>>>> --- a/tools/binman/entries.rst
>>>>> +++ b/tools/binman/entries.rst
>>>>> @@ -1890,6 +1890,21 @@ and kernel are genuine.
>>>>> +Entry: x509cert: x509 certificate for K3 devices
>>>>> +------------------------------------------------
>>>>> +
>>>>
>>>> x509 is a generic standard. Can this be made usable by other vendors as
>>>> well or
>>>> is it very specific to TI?
>>>> If this is TI specific then I'd suggest a "ti-" prefix to the entry name.
>>>>
>>>>> +Properties / Entry arguments:
>>>>> + - content: Phandle of binary to sign
>>>>> + - output: Name of the final output file
>>>>
>>>> why do you need output property?
>>>>
>>>
>>> That is not required, I had later changed it to always using
>>> certificate.bin. Will make the necessary changes.
>>>
>>>>> + - key_file: File with key inside it. If not provided, script
>>>>> generates RSA degenerate key
>>>>> + - core: Target core ID on which image would be running
>>>>> + - load: Target load address of the binary in hex
>>>>> +
>>>>> + Output files:
>>>>> + - certificate.bin: Signed certificate binary
>>>>> +
>>>>> +
>>>>> +
>>>>> Entry: x86-reset16: x86 16-bit reset code for U-Boot
>>>>> ----------------------------------------------------
>>>>> diff --git a/tools/binman/etype/x509_cert.py
>>>>> b/tools/binman/etype/x509_cert.py
>>>>> new file mode 100644
>>>>> index 0000000000..0009973155
>>>>> --- /dev/null
>>>>> +++ b/tools/binman/etype/x509_cert.py
>>>>> @@ -0,0 +1,248 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>>> +# Copyright (c) 2018 Google, Inc
>>>>> +# Written by Simon Glass <s...@chromium.org>
>>>>> +#
>>>>> +
>>>>> +# Support for a x509 certificate for signing K3 devices
>>>>> +
>>>>> +import os
>>>>> +from collections import OrderedDict
>>>>> +from subprocess import Popen, PIPE
>>>>> +from sys import stderr, stdout
>>>>> +
>>>>> +import asn1
>>>>> +from Crypto.PublicKey import RSA
>>>>> +from cryptography.hazmat.backends import default_backend
>>>>> +from cryptography.hazmat.primitives import serialization
>>>>> +
>>>>> +from binman.etype.collection import Entry_collection
>>>>> +from dtoc import fdt_util
>>>>> +from patman import tools
>>>>> +
>>>>> +temp_x509 = "x509-temp.cert"
>>>>> +cert = "certificate.bin"
>>>>> +rand_key = "eckey.pem"
>>>>> +bootcore_opts = 0
>>>>> +bootcore = 0
>>>>> +debug_type = 0
>>>>> +
>>>>> +
>>>>> +class Entry_x509_cert(Entry_collection):
>>>>> + """ An entry which contains a x509 certificate
>>>>> +
>>>>> + Properties / Entry arguments:
>>>>> + - content: Phandle of binary to sign
>>>>> + - key_file: File with key inside it. If not provided, script
>>>>> generates RSA degenerate key
>>>>> + - core: Target core ID on which image would be running
>>>>> + - load: Target load address of the binary in hex
>>>>> +
>>>>> + Output files:
>>>>> + - certificate.bin: Signed certificate binary"""
>>>>> +
>>>>> + def __init__(self, section, etype, node):
>>>>> + super().__init__(section, etype, node)
>>>>> + self.key_file = fdt_util.GetString(self._node, 'key-file', "")
>>>>> + self.core = fdt_util.GetInt(self._node, 'core', 0)
>>>>> + self.load_addr = fdt_util.GetInt(self._node, 'load', 0x41c00000)
>>>>> +
>>>>> + def ReadNode(self):
>>>>> + super().ReadNode()
>>>>> + if self.key_file == "":
>>>>> + self.degen_key = True
>>>>> + else:
>>>>> + self.degen_key = False
>>>>> +
>>>>> + def _CreateCertificate(self):
>>>>> + """Create certificate for legacy boot flow"""
>>>>> + if self.degen_key == True:
>>>>> + gen_degen_key()
>>>>> + self.key_file = rand_key
>>>>> +
>>>>> + sha_val = get_sha_val("intermediate-sysfw.bin")
>>>>> + bin_size = get_file_size("intermediate-sysfw.bin")
>>>>> + addr = "%08x" % self.load_addr
>>>>> + if self.core == 0:
>>>>> + cert_type = 2
>>>>> + elif self.core == 16:
>>>>> + cert_type = 1
>>>>> + else:
>>>>> + cert_type = 2
>>>>> + debug_type = 0
>>>>> +
>>>>> + gen_template()
>>>>> + gen_cert(bin_size, sha_val, cert_type, bootcore_opts,
>>>>> + self.core, addr, debug_type, self.key_file)
>>>>> +
>>>>> + return tools.read_file("certificate.bin")
>>>>> +
>>>>> + def ObtainContents(self):
>>>>> + self.image = self.GetContents(False)
>>>>> + if self.image is None:
>>>>> + return False
>>>>> + f = open("intermediate-sysfw.bin", "wb")
>>>>> + f.write(self.image)
>>>>> + f.close()
>>>>> + self.SetContents(self._CreateCertificate())
>>>>> + return True
>>>>> +
>>>>> + def ProcessContents(self):
>>>>> + data = self._CreateCertificate()
>>>>> + return self.ProcessContentsUpdate(data)
>>>>
>>>> Why do you need _CreateCertificate() and ProcessContents()?
>>>> Just have one ObtainContents() and try to get rid of all the intermediate
>>>> files.
>>>>
>>>
>>> I used etype/vblock.py as a reference. I will clean up this etype further.
>>>
>>
>> There were some more comments below, in case you missed them.
> Thanks!
>
>>
>>>>> +
>>>>> +
>>>>> +def get_sha_val(binary_file):
>>>>> + process = Popen(['openssl', 'dgst', '-sha512', '-hex',
>>>>> + binary_file], stdout=PIPE, stderr=PIPE)
>>>>> + stdout, stderr = process.communicate()
>>>>> + sha_val = stdout.split()[1]
>>>>> + return sha_val
>>>>> +
>>>>> +
>>>>> +def get_file_size(binary_file):
>>>>> + return os.path.getsize(binary_file)
>>>>> +
>>>>> +
>>>>> +def gen_degen_template():
>>>>> + with open("degen-template.txt", 'w+', encoding='utf-8') as f:
>>>>> + degen_temp = """
>>>>> +asn1=SEQUENCE:rsa_key
>>>>> +
>>>>> +[rsa_key]
>>>>> +version=INTEGER:0
>>>>> +modulus=INTEGER:0xDEGEN_MODULUS
>>>>> +pubExp=INTEGER:1
>>>>> +privExp=INTEGER:1
>>>>> +p=INTEGER:0xDEGEN_P
>>>>> +q=INTEGER:0xDEGEN_Q
>>>>> +e1=INTEGER:1
>>>>> +e2=INTEGER:1
>>>>> +coeff=INTEGER:0xDEGEN_COEFF"""
>>>>> + f.write(degen_temp)
>>>>> +
>>>>> +
>>>>> +def gen_template():
>>>>> + """Generate x509 Template"""
>>>>> + with open("x509-template.txt", "w+", encoding='utf-8') as f:
>>>>> + x509template = """
>>>>> +[ req ]
>>>>> +distinguished_name = req_distinguished_name
>>>>> +x509_extensions = v3_ca
>>>>> +prompt = no
>>>>> +dirstring_type = nobmp
>>>>> +
>>>>> +[ req_distinguished_name ]
>>>>> +C = US
>>>>> +ST = TX
>>>>> +L = Dallas
>>>>> +O = Texas Instruments Incorporated
>>>>> +OU = Processors
>>>>> +CN = TI support
>>>>> +emailAddress = supp...@ti.com
>>>>> +
>>>>> +[ v3_ca ]
>>>>> +basicConstraints = CA:true
>>>>> +1.3.6.1.4.1.294.1.1 = ASN1:SEQUENCE:boot_seq
>>>>> +1.3.6.1.4.1.294.1.2 = ASN1:SEQUENCE:image_integrity
>>>>> +1.3.6.1.4.1.294.1.3 = ASN1:SEQUENCE:swrv
>>>>> +# 1.3.6.1.4.1.294.1.4 = ASN1:SEQUENCE:encryption
>>>>> +1.3.6.1.4.1.294.1.8 = ASN1:SEQUENCE:debug
>>>>> +
>>>>> +[ boot_seq ]
>>>>> +certType = INTEGER:TEST_CERT_TYPE
>>>>> +bootCore = INTEGER:TEST_BOOT_CORE
>>>>> +bootCoreOpts = INTEGER:TEST_BOOT_CORE_OPTS
>>>>> +destAddr = FORMAT:HEX,OCT:TEST_BOOT_ADDR
>>>>> +imageSize = INTEGER:TEST_IMAGE_LENGTH
>>>>> +
>>>>> +[ image_integrity ]
>>>>> +shaType = OID:2.16.840.1.101.3.4.2.3
>>>>> +shaValue = FORMAT:HEX,OCT:TEST_IMAGE_SHA_VAL
>>>>> +
>>>>> +[ swrv ]
>>>>> +swrv = INTEGER:0
>>>>> +
>>>>> +# [ encryption ]
>>>>> +# initalVector = FORMAT:HEX,OCT:TEST_IMAGE_ENC_IV
>>>>> +# randomString = FORMAT:HEX,OCT:TEST_IMAGE_ENC_RS
>>>>> +# iterationCnt = INTEGER:TEST_IMAGE_KEY_DERIVE_INDEX
>>>>> +# salt = FORMAT:HEX,OCT:TEST_IMAGE_KEY_DERIVE_SALT
>>>>> +
>>>>> +[ debug ]
>>>>> +debugUID =
>>>>> FORMAT:HEX,OCT:0000000000000000000000000000000000000000000000000000000000000000
>>>>> +debugType = INTEGER:TEST_DEBUG_TYPE
>>>>> +coreDbgEn = INTEGER:0
>>>>> +coreDbgSecEn = INTEGER:0"""
>>>>> + f.write(x509template)
>>>>> +
>>>>> +
>>>>> +def parse_key(inp_key, section):
>>>>> + parsed_key = ""
>>>>> + section_true = False
>>>>> + with open(inp_key, 'r') as file:
>>>>> + for line in file:
>>>>> + if section in line:
>>>>> + section_true = True
>>>>> + elif section_true:
>>>>> + if " " not in line:
>>>>> + break
>>>>> + else:
>>>>> + parsed_key += line.replace(":", "").replace(" ",
>>>>> "")
>>>>> + return parsed_key.replace("\n", "")
>>>>> +
>>>>> +
>>>>> +def gen_degen_key():
>>>>> + """Generate a 4096 bit RSA key"""
>>>>> + try:
>>>>> + # generates 1024 bit PEM encoded RSA key in PKCS#1 format
>>>>> + private_key = RSA.generate(1024)
>>>>> + f = open('key.pem', 'wb')
>>>>> + f.write(private_key.exportKey('PEM'))
>>>>> + f.close()
>>>>> + except:
>>>>> + raise(Exception)
>>>>> +
>>>>> + try:
>>>>> + process = Popen(['openssl', 'rsa', '-in', 'key.pem',
>>>>> + '-text', '-out', 'key.txt'], stdout=PIPE,
>>>>> stderr=PIPE)
>>>>> + stdout, stderr = process.communicate()
>>>>> + except:
>>>>> + raise(stderr)
>>>>> +
>>>>> + DEGEN_MODULUS = parse_key("key.txt", "modulus")
>>>>> + DEGEN_P = parse_key("key.txt", "prime1")
>>>>> + DEGEN_Q = parse_key("key.txt", "prime2")
>>>>> + DEGEN_COEFF = parse_key("key.txt", "coefficient")
>>>>> +
>>>>> + gen_degen_template()
>>>>> +
>>>>> + with open("degen-template.txt", 'r') as file_input:
>>>>> + with open("degenerateKey.txt", 'w') as file_output:
>>>>> + for line in file_input:
>>>>> + s = line.replace("DEGEN_MODULUS", DEGEN_MODULUS).replace(
>>>>> + "DEGEN_P", DEGEN_P).replace("DEGEN_Q",
>>>>> DEGEN_Q).replace("DEGEN_COEFF", DEGEN_COEFF)
>>>>> + file_output.write(s)
>>>>> +
>>>>> + try:
>>>>> + process = Popen(['openssl', 'asn1parse', '-genconf',
>>>>> 'degenerateKey.txt',
>>>>> + '-out', 'degenerateKey.der'], stdout=PIPE,
>>>>> stderr=PIPE)
>>>>> + stdout, stderr = process.communicate()
>>>>> + except:
>>>>> + raise(stderr)
>>>>> +
>>>>> + try:
>>>>> + process = Popen(['openssl', 'rsa', '-in', 'degenerateKey.der',
>>>>> + '-inform', 'DER', '-outform', 'PEM', '-out',
>>>>> rand_key])
>>>>> + stdout, stderr = process.communicate()
>>>>> + except:
>>>>> + raise(stderr)
>>>>> +
>>>>> +
>>>>> +def gen_cert(bin_size, sha_val, cert_type, bootcore_opts, bootcore,
>>>>> addr, debug_type, key):
>>>>> + with open(temp_x509, "w") as output_file:
>>>>> + with open("x509-template.txt", "r") as input_file:
>>>>> + for line in input_file:
>>>>> + output_file.write(line.replace("TEST_IMAGE_LENGTH",
>>>>> str(bin_size)).replace("TEST_IMAGE_SHA_VAL",
>>>>> sha_val.decode("utf-8")).replace("TEST_CERT_TYPE",
>>>>> str(cert_type)).replace(
>>>>> + "TEST_BOOT_CORE_OPTS",
>>>>> str(bootcore_opts)).replace("TEST_BOOT_CORE",
>>>>> str(bootcore)).replace("TEST_BOOT_ADDR",
>>>>> str(addr)).replace("TEST_DEBUG_TYPE", str(debug_type)))
>>>>> + process = Popen(['openssl', 'req', '-new', '-x509', '-key', key,
>>>>> '-nodes', '-outform',
>>>>> + 'DER', '-out', cert, '-config', temp_x509,
>>>>> '-sha512'], stdout=PIPE, stderr=PIPE)
>>>>> + stdout, stderr = process.communicate()
>>>>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
>>>>> index 5ff294a386..d8ee592250 100644
>>>>> --- a/tools/binman/ftest.py
>>>>> +++ b/tools/binman/ftest.py
>>>>> @@ -96,6 +96,7 @@ ENV_DATA = b'var1=1\nvar2="2"'
>>>>> PRE_LOAD_MAGIC = b'UBSH'
>>>>> PRE_LOAD_VERSION = 0x11223344.to_bytes(4, 'big')
>>>>> PRE_LOAD_HDR_SIZE = 0x00001000.to_bytes(4, 'big')
>>>>> +X509_DATA = b'filetobesigned'
>>>>> # Subdirectory of the input dir to use to put test FDTs
>>>>> TEST_FDT_SUBDIR = 'fdts'
>>>>> @@ -200,6 +201,7 @@ class TestFunctional(unittest.TestCase):
>>>>> TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
>>>>> TestFunctional._MakeInputFile('sysfw.bin', TI_SYSFW_DATA)
>>>>> TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
>>>>> + TestFunctional._MakeInputFile('tosign.bin', X509_DATA)
>>>>> # Add a few .dtb files for testing
>>>>> TestFunctional._MakeInputFile('%s/test-fdt1.dtb' %
>>>>> TEST_FDT_SUBDIR,
>>>>> @@ -5537,5 +5539,10 @@ fdt fdtmap Extract the
>>>>> devicetree blob from the fdtmap
>>>>> data = self._DoReadFile('232_ti_sysfw.dts')
>>>>> self.assertEqual(TI_SYSFW_DATA, data[:len(TI_SYSFW_DATA)])
>>>>> + def testX509Cert(self):
>>>>> + """Test an image with the default x509 certificate header"""
>>>>> + data = self._DoReadFile('232_x509_cert.dts')
>>>>> + self.assertEqual(X509_DATA, data[938:938 + len(X509_DATA)])
>>>>
>>>> what is 938?
>
> 938 was the size of the expected x509 certificate to which the data is
> appended.
And that is not exepected to change anytime in future?
If not, then you can use a macro define for that.
If yes, then this solution is not good.
>
>>>>
>>>> Isn't it easier to just assert that _DoReadFile('232_x509_cert.dts') is
>>>> greater than len(X509_DATA)?
> Would that test ensure the proper creation of certificate.bin?
>
No but it would just tell you that something was appended or not and
will not break if certificate size changes.
Your above check was not checking certificate contents either.
>>>>
>>>>> +
>>>>> if __name__ == "__main__":
>>>>> unittest.main()
>>>>> diff --git a/tools/binman/test/232_x509_cert.dts
>>>>> b/tools/binman/test/232_x509_cert.dts
>>>>> new file mode 100644
>>>>> index 0000000000..f768568ca7
>>>>> --- /dev/null
>>>>> +++ b/tools/binman/test/232_x509_cert.dts
>>>>> @@ -0,0 +1,18 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +
>>>>> +/dts-v1/;
>>>>> +
>>>>> +/ {
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <1>;
>>>>> +
>>>>> + binman {
>>>>> + x509-cert {
>>>>> + content = <&image>;
>>>>> + };
>>>>> +
>>>>> + image: blob-ext {
>>>>> + filename = "tosign.bin";
>>>>> + };
>>>>> + };
>>>>> +};
>>>>> diff --git a/tools/k3_gen_x509_cert.sh b/tools/k3_gen_x509_cert.sh
>>>>> index 298cec1313..b6ef5a2de3 100755
>>>>> --- a/tools/k3_gen_x509_cert.sh
>>>>> +++ b/tools/k3_gen_x509_cert.sh
>>>>> @@ -109,7 +109,7 @@ gen_degen_key() {
>>>>> openssl asn1parse -genconf degenerateKey.txt -out
>>>>> degenerateKey.der >>/dev/null 2>&1
>>>>> openssl rsa -in degenerateKey.der -inform DER -outform PEM -out
>>>>> $RAND_KEY >>/dev/null 2>&1
>>>>> KEY=$RAND_KEY
>>>>> - rm key.pem key.txt degen-template.txt degenerateKey.txt
>>>>> degenerateKey.der
>>>>> + #rm key.pem key.txt degen-template.txt degenerateKey.txt
>>>>> degenerateKey.der
>>>>> }
>>>>> declare -A options_help
>>>>> @@ -246,7 +246,7 @@ gen_cert
>>>>> cat $CERT $BIN > $OUTPUT
>>>>> # Remove all intermediate files
>>>>> -rm $TEMP_X509 $CERT x509-template.txt
>>>>> -if [ "$KEY" == "$RAND_KEY" ]; then
>>>>> - rm $RAND_KEY
>>>>> -fi
>>>>> +#rm $TEMP_X509 $CERT x509-template.txt
>>>>> +#if [ "$KEY" == "$RAND_KEY" ]; then
>>>>> +# rm $RAND_KEY
>>>>> +#fi
>>>>
>>>> Why these changes?
>>>> Maybe you should include them within
>>>> "ifndef CONFIG_BINMAN ... endif" to avoid breaking platforms not using
>>>> BINMAN.
>
> These file changes (k3_gen_x509_cert.sh) were not intended to be present in
> the patch series, sorry for that.
>
cheers,
-roger
