Re: [PATCH RFC v2 05/11] ti: etype: x509: Add etype for x509 certificate for K3 devices

[Roger Quadros <rogerq@kernel.org>]

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-francis@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 <sjg@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           = support@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