[Buildroot] [PATCH 1/2] package/wolftpm: fix dependencies

Sun Jun 12 04:13:47 UTC 2022

Good Morning Fabrice,

Please find my answers in-line below and thank you for asking good 
questions. It really helps the discussion.

On 2022-06-12 12:24 AM, Fabrice Fontaine wrote:
> Hi Dimi,
> 
> Le sam. 11 juin 2022 à 22:19, Dimi Tomov <dimi at tpm.dev> a écrit :
>> 
>> Hi Fabrice,
>> 
>> 1) I advise strongly against this patch in its current form due to
>> security concerns. Please find my motivation below.
>> 
>> 2) Existing wolfTPM dependencies are propagated from the wolfssl
>> package.
>> 
>> 3) I am unable to reproduce your build error. Please provide the exact
>> commands and order you are running them on a clean buildroot master.
>> 
>> Topic #1
>> 
>> In order to have Man-in-the-middle protection, wolfTPM needs a
>> cryptogrpahic provider to set a secure channel of communication 
>> between
>> the HOST CPU and the TPM 2.0 HSM chip. Without MITM protection, keys 
>> and
>> other sensitive data can be sniffed, making it pointless to use a TPM
>> 2.0 security chip in the first place.
>> 
>> Therefore, making the default wolfTPM build work without wolfssl is a
>> SECURITY RED FLAG.
>> 
>> Please consider adding an OPTION to build wolfTPM without wolfssl,
>> however keep the default wolfTPM build require the wolfssl library.
> 
> I could add a BR2_PACKAGE_WOLFTPM_WOLFSSL option that is enabled by
> default in a v2.

I think this is the best path forward.

> 
> However, do you know why wolfTPM can be built without wolfcrypt?
> upstream added this option back in 2018 [1] for "cases where the
> wolfTPM software was not using parameter encryption or session nonces"
> [2].

I am happy to provide you with more details. Hardware security, 
including TPM, is my passion.

- XOR Parameter Encryption was added in Sep 2020 
https://github.com/wolfSSL/wolfTPM/pull/122
- AES CFB Parameter Encryption was added in Dec 2020 
https://github.com/wolfSSL/wolfTPM/pull/129
- HMAC sessions were also enabled in Dec 2020 also PR129

- Parameter Encryption is the TPM 2.0 capability to encrypt commands 
with sensitive data between the HOST CPU and the TPM 2.0 chip,  like 
generating a new key pair, key authorization to perform 
encryption/decryption/message signing, etc.
- For TPM 2.0 commands that do not support parameter encryption there 
are HMAC sessions to encrypt the communication to and from the TPM 2.0 
chip.
- Without these two capabilities, the SPI/I2C communication between the 
HOST CPU and the TPM 2.0 chip is send plain/raw.

The reason wolfTPM can be built without wolfcrypt is because the TPM 2.0 
chip allows it. Unfortunately, this is how the famous Dolos attack 
happened - https://trmm.net/tpm-sniffing/ , a basic attack that could 
have been easily mitigated if Parameter Encryption was enabled.

Great example of what happens when OEMs do not enable TPM 2.0 Parameter 
Encryption at boot and even Microsoft miss to require it from the 
UEFI/Firmware boot.

> I'm far from being a TPM expert but could it be that some TPM use
> cases don't need encryption between host CPU and HSM chip?

In my professional opinion, there is only one use case where not 
enabling Parameter Encryption may make sense - When a softwareTPM is 
used instead of a discrete TPM chip. Then, the softwareTPM is run in a 
Trusted Execution Environment together with the firmware using wolfTPM, 
thus the TPM 2.0 "chip" is actually in the same isolated memory space as 
the firmware using it (there is no physical bus to be sniffed).

> 
> [1] 
> https://github.com/wolfSSL/wolfTPM/commit/d8174d4ef714fc5e6a18e26d0586c6e91c3275ab
> [2] https://github.com/wolfSSL/wolfTPM/pull/24
> 
>> 
>> Topic #2
>> 
>> The threads and dynamic library support dependencies are propagated 
>> from
>> the wolfssl package and per recommendation of Thomas (adding to our
>> discussion).
> 
> threads dependency must indeed be propagated to this new
> BR2_PACKAGE_WOLFTPM_WOLFSSL option.
> However, dynamic library support is not needed as, from my
> understanding and build testing, wolftpm doesn't need
> BR2_PACKAGE_WOLFSSL_ALL (and so can be statically built).

wolfTPM has functionalities that depend on some very specific wolfSSL 
options like --openssl-extras , --enable-aes-cfb, etc. By using the 
wolfSSL_ALL we guarantee that these are built in. Otherwise there is 
also --enable-wolftpm configure line for wolfssl, however this one does 
not include --openssl-extras that is also needed if we want to export 
TPM public keys to PEM/DER.

I would recommend enabling wolfSSL by default for wolfTPM with the 
following "./configure --enable-wolftpm --enable-opensslextra 
--enable-keygen". This could be an extra wolfSSL package option and then 
wolfTPM selects this option if wolfSSL_ALL is not already chosen. 
Perhaps such improvement is possible?

> 
>> 
>> Both, wolfssl and wolfTPM could be build statically, however this is 
>> not
>> how I found the wolfssl package working in buildroot. Therefore, I 
>> used
>> the existing code base and added the wolftpm package accordingly.
>> 
>> Topic #3
>> 
>> It is a good practice when reporting an error to share how it can be
>> reproduced. I am unable to reproduce your build error.
> 
> The build failure was raised by one of the autobuilder, you can easily
> reproduce it by retrieving the defconfig from the link in the commit
> message [3].
> Basically, this build failure will be raised on any "fresh" build
> because wolfssl has not been added to WOLFTPM_DEPENDENCIES.
> Section [4] of buildroot manual will probably help you to reproduce
> this build failure.

This is odd. I thought depends on does this and guarantees wolfSSL is 
built before wolfTPM.

> 
> [3] 
> http://autobuild.buildroot.org/results/77a93521b909e701ef4e86f18524258b9242c721
> [4] 
> https://buildroot.org/downloads/manual/manual.html#_analyzing_and_fixing_autobuild_failures
> 
>> 
>> Last
>> 
>> I find it difficult to address multiple topics/major changes in one
>> patch. I have tried to decouple the topics above and I am ready to
>> discuss further. Thank you for providing this feedback. I think there
>> are improvements we could make based on what is the expectation and
>> buildroot maintainers' requirements.
>> 
>> Regards,
>> Dimi
>> --
>> Founder of TPM.dev
>> 
>> 
>> On 2022-06-11 05:35 PM, Fabrice Fontaine wrote:
>> > pkg-config is not used by wolftpm and wolfssl dependency is missing
>> > (and
>> > optional) resulting in the following build failure since the addition
>> > of
>> > the package in commit 4bb884a3c61c6b71e33f69453a90eb2a367f64b7:
>> >
>> > configure: error: WolfSSL library not found. You can get it from
>> > http://www.wolfssl.com/download.html
>> >         If it's already installed, specify its path using
>> > --with-wolfcrypt=/dir or --prefix=/dir
>> >
>> > Moreover, BR2_PACKAGE_WOLFSSL_ALL, threads and dynamic libray support
>> > are not mandatory:
>> >
>> > ./utils/test-pkg -p wolftpm
>> >                     bootlin-armv5-uclibc [1/6]: OK
>> >                      bootlin-armv7-glibc [2/6]: OK
>> >                    bootlin-armv7m-uclibc [3/6]: OK
>> >                      bootlin-x86-64-musl [4/6]: OK
>> >                       br-arm-full-static [5/6]: OK
>> >                             sourcery-arm [6/6]: OK
>> > 6 builds, 0 skipped, 0 build failed, 0 legal-info failed, 0 show-info
>> > failed
>> >
>> > Fixes:
>> >  -
>> > http://autobuild.buildroot.org/results/77a93521b909e701ef4e86f18524258b9242c721
>> >
>> > Signed-off-by: Fabrice Fontaine <fontaine.fabrice at gmail.com>
>> > ---
>> >  package/wolftpm/Config.in  |  7 -------
>> >  package/wolftpm/wolftpm.mk | 13 ++++++++++---
>> >  2 files changed, 10 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/package/wolftpm/Config.in b/package/wolftpm/Config.in
>> > index 0fe094f277..741d92aa4e 100644
>> > --- a/package/wolftpm/Config.in
>> > +++ b/package/wolftpm/Config.in
>> > @@ -1,9 +1,5 @@
>> >  config BR2_PACKAGE_WOLFTPM
>> >       bool "wolftpm"
>> > -     depends on BR2_TOOLCHAIN_HAS_THREADS
>> > -     depends on !BR2_STATIC_LIBS
>> > -     select BR2_PACKAGE_WOLFSSL
>> > -     select BR2_PACKAGE_WOLFSSL_ALL
>> >       help
>> >         wolfTPM is a portable, open-source TPM 2.0 stack with
>> >         backward API compatibility, designed for embedded use.
>> > @@ -11,6 +7,3 @@ config BR2_PACKAGE_WOLFTPM
>> >         resource usage.
>> >
>> >         https://www.wolfssl.com/
>> > -
>> > -comment "wolftpm needs a toolchain w/ threads, dynamic library"
>> > -     depends on !BR2_TOOLCHAIN_HAS_THREADS || BR2_STATIC_LIBS
>> > diff --git a/package/wolftpm/wolftpm.mk b/package/wolftpm/wolftpm.mk
>> > index ea01eaad6b..f0cf0df0d3 100644
>> > --- a/package/wolftpm/wolftpm.mk
>> > +++ b/package/wolftpm/wolftpm.mk
>> > @@ -10,7 +10,6 @@ WOLFTPM_INSTALL_STAGING = YES
>> >  WOLFTPM_LICENSE = GPL-2.0+
>> >  WOLFTPM_LICENSE_FILES = LICENSE
>> >  WOLFTPM_CPE_ID_VENDOR = wolfssl
>> > -WOLFTPM_DEPENDENCIES = host-pkgconf
>> >  WOLFTPM_CONFIG_SCRIPTS = wolftpm-config
>> >
>> >  # wolfTPM's source code is released without a configure script,
>> > @@ -19,8 +18,7 @@ WOLFTPM_AUTORECONF = YES
>> >
>> >  WOLFTPM_CONF_OPTS = \
>> >       --disable-examples \
>> > -     --enable-devtpm \
>> > -     --with-wolfcrypt=$(STAGING_DIR)/usr
>> > +     --enable-devtpm
>> >
>> >  # Fix for missing config.rpath in the codebase
>> >  define WOLFTPM_TOUCH_CONFIG_RPATH
>> > @@ -29,4 +27,13 @@ define WOLFTPM_TOUCH_CONFIG_RPATH
>> >  endef
>> >  WOLFTPM_PRE_CONFIGURE_HOOKS += WOLFTPM_TOUCH_CONFIG_RPATH
>> >
>> > +ifeq ($(BR2_PACKAGE_WOLFSSL),y)
>> > +WOLFTPM_CONF_OPTS += \
>> > +     --enable-wolfcrypt \
>> > +     --with-wolfcrypt=$(STAGING_DIR)/usr
>> > +WOLFTPM_DEPENDENCIES += wolfssl
>> > +else
>> > +WOLFTPM_CONF_OPTS += --disable-wolfcrypt
>> > +endif
>> > +
>> >  $(eval $(autotools-package))
> 
> Best Regards,
> 
> Fabrice

-- 
Founder of TPM.dev