Re: [PATCH] imx: hab: add documentation about the required keys/certs

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 16/05/2024 10.25, Claudius Heine wrote:
> Hi Tim and Marek,
> 
> On 2024-05-16 12:46 am, Tim Harvey wrote:
>> On Tue, May 14, 2024 at 11:50 AM Tim Harvey <tharvey@gateworks.com>
>> wrote:
>>> On Sun, May 12, 2024 at 10:08 PM Marek Vasut <marex@denx.de> wrote:
>>>> On 5/8/24 9:23 AM, Claudius Heine wrote:
>>>>> On 2024-05-07 3:28 pm, Marek Vasut wrote:
>>>>>> It would be good to mention the DT properties which govern the crypto
>>>>>> material paths -- nxp,srk-table, nxp,csf-crt, nxp,img-crt --
>>>>>> somewhere
>>>>>> around this sentence.
>>>>>
>>>>> This is something that should be documented with the changes where
>>>>> that
>>>>> code was added, IMO. I only documented here what I found out and have
>>>>> used myself, I haven't used those.
>>>>>
>>>>> I would be interested in reading how to best overwrite those paths and
>>>>> the image structured from board u-boot.dtsi files myself.
>>>>>
>>>>> If you want to can pickup my patch and integrate it into your
>>>>> series and
>>>>> extend it.
>>>>
>>>> I'll keep it in mind for V3.
>>
>> Hi Marek,
>>
>> The documentation patch here by Claudius does resolve my issues
>> discussed in the other thread and I can confirm symlinks work fine so
>> I think something like the following should be added:
>>
>> CST_DIR=/usr/src/cst-3.3.2/
>> ln -s $CST_DIR/crts .
>> ln -s $CST_DIR/keys .
> 
> `keys` and `crts` are very short and generic names, and putting them
> into the build directory might cause issues at some point. But I would
> not be against putting them into a sub directory (`imx-hab/{keys,crts}`?).

It is probably useful to be aware of the quality of the cst code. For
reference, I quote get_key_file()

int32_t get_key_file(const char* cert_file, char* key_file)
{
    /* Algorithm to locate key file from given cert file  */
    /* for now just assume the key to present in the      */
    /* same folder as cert file. The crt in the name will */
    /* will be replaced with key */
    char * folder;
    int32_t i = strlen(cert_file);  /**< Index into key filename,
initialized
                                         to filename length */

    strcpy(key_file, cert_file);
    key_file[i] = 0;

    key_file[i-5] = 'y';
    key_file[i-6] = 'e';
    key_file[i-7] = 'k';

    /* Search for folder name "certs" in the file and replace it with
"keys" */
    /* Keys are found in "keys" folder and certs are in "certs" folder
    */

    folder = strstr(key_file, "crts");
    if(folder)
    {
        folder[0] = 'k';
        folder[1] = 'e';
        folder[2] = 'y';
        folder[3] = 's';
    }
    return CAL_SUCCESS;
}

Ignoring the inconsistencies in the comments, obviously there are a lot
of implicit assumptions on file names and paths. First, the assumption
that the filename of they key corresponding to the certificate can be
obtained by replacing [-7:-5] by "key". Second, and much more egregious,
is the use of strstr() on key_file searching for "crts", and just
blindly replacing the first such with "keys", and ignoring it if not
found. So if that string appears anywhere in the path (say, my homedir
is /home/dcrts/ and I have the key material somewhere below that) this
will replace the wrong occurrence (and look in /home/dkeys/ ....).

And of course it was unthinkable that this could have been written using
the much shorter memcpy(..., "keys", 4) so that one could actually `git
grep 'keys'` and figure out what was going on.

Rasmus