[Buildroot] [PATCH v4, 1/1] package/uacme: don't allow mbedtls with ualpn

[Buildroot] [PATCH v4, 1/1] package/uacme: don't allow mbedtls with ualpn

[Fabrice Fontaine]

ualpn with mbedtls requires the activation of
MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION on mbedtls which can
be a security risk.

So let the user explicitly choose the crypto library by copy/pasting
behavior of libssh and don't allow the user to select mbedtls with ualpn

Fixes:
 - http://autobuild.buildroot.org/results/5d42189299549cd655218e9e7cfcfa63e79f74ec

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
---
Changes v3 -> v4:
 - Fix commit title

Changes v2 -> v3 (after review of Yann E. Morin):
 - Put back the option to select crypto backend and do not allow the
   user to select mbedtls with ualpn

Changes v1 -> v2 (after review of Thomas Petazzoni and Yann E. Morin):
 - Do not use ualpn with mbedtls

 package/uacme/Config.in | 24 ++++++++++++++++++++++++
 package/uacme/uacme.mk  |  6 +++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/package/uacme/Config.in b/package/uacme/Config.in
index 58b7c534e7..ba60d787f0 100644
--- a/package/uacme/Config.in
+++ b/package/uacme/Config.in
@@ -16,6 +16,30 @@ config BR2_PACKAGE_UACME
 
 if BR2_PACKAGE_UACME
 
+choice
+	prompt "Crypto Backend"
+	help
+	  Select crypto library to be used in uacme.
+
+config BR2_PACKAGE_UACME_GNUTLS
+	bool "gnutls"
+	depends on BR2_PACKAGE_GNUTLS
+
+config BR2_PACKAGE_UACME_MBEDTLS
+	bool "mbedtls"
+	depends on BR2_PACKAGE_MBEDTLS
+	depends on !BR2_PACKAGE_UACME_UALPN
+
+comment "mbedtls crypto backend unavailable with ualpn"
+	depends on BR2_PACKAGE_MBEDTLS
+	depends on BR2_PACKAGE_UACME_UALPN
+
+config BR2_PACKAGE_UACME_OPENSSL
+	bool "openssl"
+	depends on BR2_PACKAGE_OPENSSL
+
+endchoice
+
 config BR2_PACKAGE_UACME_UALPN
 	bool "enable ualpn"
 	depends on BR2_TOOLCHAIN_HAS_THREADS
diff --git a/package/uacme/uacme.mk b/package/uacme/uacme.mk
index 6df13eced6..90c3a24c13 100644
--- a/package/uacme/uacme.mk
+++ b/package/uacme/uacme.mk
@@ -15,13 +15,13 @@ UACME_DEPENDENCIES = libcurl
 
 UACME_CONF_ENV = ac_cv_prog_cc_c99='-std=gnu99'
 
-ifeq ($(BR2_PACKAGE_GNUTLS),y)
+ifeq ($(BR2_PACKAGE_UACME_GNUTLS),y)
 UACME_CONF_OPTS += --with-gnutls
 UACME_DEPENDENCIES += gnutls
-else ifeq ($(BR2_PACKAGE_MBEDTLS),y)
+else ifeq ($(BR2_PACKAGE_UACME_MBEDTLS),y)
 UACME_CONF_OPTS += --with-mbedtls
 UACME_DEPENDENCIES += mbedtls
-else ifeq ($(BR2_PACKAGE_OPENSSL),y)
+else ifeq ($(BR2_PACKAGE_UACME_OPENSSL),y)
 UACME_CONF_OPTS += --with-openssl
 UACME_DEPENDENCIES += openssl
 endif
-- 
2.25.1

[Buildroot] [PATCH v4, 1/1] package/uacme: don't allow mbedtls with ualpn

[Yann E. MORIN]

Fabrice, All,

On 2020-04-26 13:05 +0200, Fabrice Fontaine spake thusly:
> ualpn with mbedtls requires the activation of
> MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION on mbedtls which can
> be a security risk.
> 
> So let the user explicitly choose the crypto library by copy/pasting
> behavior of libssh and don't allow the user to select mbedtls with ualpn
> 
> Fixes:
>  - http://autobuild.buildroot.org/results/5d42189299549cd655218e9e7cfcfa63e79f74ec
> 
> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> ---
[--SNIP--]
> diff --git a/package/uacme/Config.in b/package/uacme/Config.in
> index 58b7c534e7..ba60d787f0 100644
> --- a/package/uacme/Config.in
> +++ b/package/uacme/Config.in
> @@ -16,6 +16,30 @@ config BR2_PACKAGE_UACME
>  
>  if BR2_PACKAGE_UACME
>  
> +choice
> +	prompt "Crypto Backend"
> +	help
> +	  Select crypto library to be used in uacme.
> +
> +config BR2_PACKAGE_UACME_GNUTLS
> +	bool "gnutls"
> +	depends on BR2_PACKAGE_GNUTLS
> +
> +config BR2_PACKAGE_UACME_MBEDTLS
> +	bool "mbedtls"
> +	depends on BR2_PACKAGE_MBEDTLS
> +	depends on !BR2_PACKAGE_UACME_UALPN
> +
> +comment "mbedtls crypto backend unavailable with ualpn"
> +	depends on BR2_PACKAGE_MBEDTLS
> +	depends on BR2_PACKAGE_UACME_UALPN
> +
> +config BR2_PACKAGE_UACME_OPENSSL
> +	bool "openssl"
> +	depends on BR2_PACKAGE_OPENSSL
> +
> +endchoice

Sorry, but this is still not correct: enable mbedtls, then enable uacme
and ualpn: there is no crypto backend selectable in the choice...

Regards,
Yann E. MORIN.

>  config BR2_PACKAGE_UACME_UALPN
>  	bool "enable ualpn"
>  	depends on BR2_TOOLCHAIN_HAS_THREADS
> diff --git a/package/uacme/uacme.mk b/package/uacme/uacme.mk
> index 6df13eced6..90c3a24c13 100644
> --- a/package/uacme/uacme.mk
> +++ b/package/uacme/uacme.mk
> @@ -15,13 +15,13 @@ UACME_DEPENDENCIES = libcurl
>  
>  UACME_CONF_ENV = ac_cv_prog_cc_c99='-std=gnu99'
>  
> -ifeq ($(BR2_PACKAGE_GNUTLS),y)
> +ifeq ($(BR2_PACKAGE_UACME_GNUTLS),y)
>  UACME_CONF_OPTS += --with-gnutls
>  UACME_DEPENDENCIES += gnutls
> -else ifeq ($(BR2_PACKAGE_MBEDTLS),y)
> +else ifeq ($(BR2_PACKAGE_UACME_MBEDTLS),y)
>  UACME_CONF_OPTS += --with-mbedtls
>  UACME_DEPENDENCIES += mbedtls
> -else ifeq ($(BR2_PACKAGE_OPENSSL),y)
> +else ifeq ($(BR2_PACKAGE_UACME_OPENSSL),y)
>  UACME_CONF_OPTS += --with-openssl
>  UACME_DEPENDENCIES += openssl
>  endif
> -- 
> 2.25.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v4, 1/1] package/uacme: don't allow mbedtls with ualpn

[Thomas Petazzoni]

On Sun, 26 Apr 2020 13:36:39 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> > +choice
> > +	prompt "Crypto Backend"
> > +	help
> > +	  Select crypto library to be used in uacme.
> > +
> > +config BR2_PACKAGE_UACME_GNUTLS
> > +	bool "gnutls"
> > +	depends on BR2_PACKAGE_GNUTLS
> > +
> > +config BR2_PACKAGE_UACME_MBEDTLS
> > +	bool "mbedtls"
> > +	depends on BR2_PACKAGE_MBEDTLS
> > +	depends on !BR2_PACKAGE_UACME_UALPN
> > +
> > +comment "mbedtls crypto backend unavailable with ualpn"
> > +	depends on BR2_PACKAGE_MBEDTLS
> > +	depends on BR2_PACKAGE_UACME_UALPN
> > +
> > +config BR2_PACKAGE_UACME_OPENSSL
> > +	bool "openssl"
> > +	depends on BR2_PACKAGE_OPENSSL
> > +
> > +endchoice  
> 
> Sorry, but this is still not correct: enable mbedtls, then enable uacme
> and ualpn: there is no crypto backend selectable in the choice...

Do we need a choice in the first place ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v4, 1/1] package/uacme: don't allow mbedtls with ualpn

[Fabrice Fontaine]

Le dim. 26 avr. 2020 ? 14:13, Thomas Petazzoni
<thomas.petazzoni@bootlin.com> a ?crit :
>
> On Sun, 26 Apr 2020 13:36:39 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
>
> > > +choice
> > > +   prompt "Crypto Backend"
> > > +   help
> > > +     Select crypto library to be used in uacme.
> > > +
> > > +config BR2_PACKAGE_UACME_GNUTLS
> > > +   bool "gnutls"
> > > +   depends on BR2_PACKAGE_GNUTLS
> > > +
> > > +config BR2_PACKAGE_UACME_MBEDTLS
> > > +   bool "mbedtls"
> > > +   depends on BR2_PACKAGE_MBEDTLS
> > > +   depends on !BR2_PACKAGE_UACME_UALPN
> > > +
> > > +comment "mbedtls crypto backend unavailable with ualpn"
> > > +   depends on BR2_PACKAGE_MBEDTLS
> > > +   depends on BR2_PACKAGE_UACME_UALPN
> > > +
> > > +config BR2_PACKAGE_UACME_OPENSSL
> > > +   bool "openssl"
> > > +   depends on BR2_PACKAGE_OPENSSL
> > > +
> > > +endchoice
> >
> > Sorry, but this is still not correct: enable mbedtls, then enable uacme
> > and ualpn: there is no crypto backend selectable in the choice...
>
> Do we need a choice in the first place ?
v2 didn't add a choice but it was a bit too confusing so I made v3/v4
upon Yann's request. I'll wait for your feedback before making a new
iteration.
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Best Regards,

Fabrice

[Buildroot] [PATCH v4, 1/1] package/uacme: don't allow mbedtls with ualpn

[Nicola Di Lieto]

On Sun, Apr 26, 2020 at 02:30:04PM +0200, Fabrice Fontaine wrote:
>> Do we need a choice in the first place ?
>v2 didn't add a choice but it was a bit too confusing so I made v3/v4
>upon Yann's request. I'll wait for your feedback before making a new
>iteration.

Hi Fabrice,

I think the idea of a backend choice menu is good, if you manage to sort 
out all the subtleties. There is a similar thing in libcurl which maybe 
you could have a look at:

https://git.busybox.net/buildroot/tree/package/libcurl/Config.in

As an aside, I filed a pull request with mbedTLS. If it gets accepted 
I'll be able to make ualpn work with mbedTLS without depending on 
MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION:

https://github.com/ARMmbed/mbedtls/pull/3243

Take care,
Nicola

[Buildroot] [PATCH v4, 1/1] package/uacme: don't allow mbedtls with ualpn

[Yann E. MORIN]

Thomas, All,

On 2020-04-26 14:13 +0200, Thomas Petazzoni spake thusly:
> On Sun, 26 Apr 2020 13:36:39 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> 
> > > +choice
> > > +	prompt "Crypto Backend"
> > > +	help
> > > +	  Select crypto library to be used in uacme.
> > > +
> > > +config BR2_PACKAGE_UACME_GNUTLS
> > > +	bool "gnutls"
> > > +	depends on BR2_PACKAGE_GNUTLS
> > > +
> > > +config BR2_PACKAGE_UACME_MBEDTLS
> > > +	bool "mbedtls"
> > > +	depends on BR2_PACKAGE_MBEDTLS
> > > +	depends on !BR2_PACKAGE_UACME_UALPN
> > > +
> > > +comment "mbedtls crypto backend unavailable with ualpn"
> > > +	depends on BR2_PACKAGE_MBEDTLS
> > > +	depends on BR2_PACKAGE_UACME_UALPN
> > > +
> > > +config BR2_PACKAGE_UACME_OPENSSL
> > > +	bool "openssl"
> > > +	depends on BR2_PACKAGE_OPENSSL
> > > +
> > > +endchoice  
> > Sorry, but this is still not correct: enable mbedtls, then enable uacme
> > and ualpn: there is no crypto backend selectable in the choice...
> Do we need a choice in the first place ?

We already have a few packages (libcurl, libssh, libssh2) that have a
choice for the crypto backend, so why not for uacme?

Yes, the above are all libraries, but that is not much different for an
application I believe.

Regards,
Yann E. MORIN.

> Thomas
> -- 
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v4, 1/1] package/uacme: don't allow mbedtls with ualpn

[Yann E. MORIN]

Fabrice, All,

On 2020-04-26 14:30 +0200, Fabrice Fontaine spake thusly:
> Le dim. 26 avr. 2020 ? 14:13, Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> a ?crit :
> > On Sun, 26 Apr 2020 13:36:39 +0200
> > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > > > +choice
> > > > +   prompt "Crypto Backend"

Note that a choice must define its type, so use 'bool', not 'prompt'.

Yes, I know we have a lot of choices that use 'prompt' and do not spcify
their types. That's not a reason to continue begin wrong. ;-) We also
have quite a few choices that correctly use 'bool', so that would not be
a first either.

(Note: choices can be tristate, too, even if we don't use tristates in
Buildroot, hence the reason to specify the type.)

> > > > +   help
> > > > +     Select crypto library to be used in uacme.
> > > > +
> > > > +config BR2_PACKAGE_UACME_GNUTLS
> > > > +   bool "gnutls"
> > > > +   depends on BR2_PACKAGE_GNUTLS
> > > > +
> > > > +config BR2_PACKAGE_UACME_MBEDTLS
> > > > +   bool "mbedtls"
> > > > +   depends on BR2_PACKAGE_MBEDTLS
> > > > +   depends on !BR2_PACKAGE_UACME_UALPN
> > > > +
> > > > +comment "mbedtls crypto backend unavailable with ualpn"
> > > > +   depends on BR2_PACKAGE_MBEDTLS
> > > > +   depends on BR2_PACKAGE_UACME_UALPN
> > > > +
> > > > +config BR2_PACKAGE_UACME_OPENSSL
> > > > +   bool "openssl"
> > > > +   depends on BR2_PACKAGE_OPENSSL
> > > > +
> > > > +endchoice
> > >
> > > Sorry, but this is still not correct: enable mbedtls, then enable uacme
> > > and ualpn: there is no crypto backend selectable in the choice...
> >
> > Do we need a choice in the first place ?
> v2 didn't add a choice but it was a bit too confusing so I made v3/v4
> upon Yann's request. I'll wait for your feedback before making a new
> iteration.

For the record, v1 of the patchset had a choice. It was not the choice
that propmpted a respin, but the way the patches were ordered (i.e. the
fix was coming after the features).

It turns out that adding the choice can be both the fix (for the ualpn
incompatibility with mbedtls) *and* the feature (choosing the crypto
backend).

One way to solve the issue would be something like:

    diff --git a/package/uacme/Config.in b/package/uacme/Config.in
    index 58b7c534e7..53a0ab4bf2 100644
    --- a/package/uacme/Config.in
    +++ b/package/uacme/Config.in
    @@ -16,14 +16,36 @@ config BR2_PACKAGE_UACME
     
     if BR2_PACKAGE_UACME
     
    +choice
    +	bool "crypto backend"
    +
    +config BR2_PACKAGE_UACME_CRYPTO_GNUTLS
    +	bool "gnutls"
    +	depends on BR2_PACKAGE_GNUTLS
    +
    +config BR2_PACKAGE_UACME_CRYPTO_MBEDTLS
    +	bool "mbedtls"
    +	depends on BR2_PACKAGE_MBEDTLS
    +
    +config BR2_PACKAGE_UACME_CRYPTO_OPENSSL
    +	bool "openssl"
    +	depends on BR2_PACKAGE_OPENSSL
    +
    +endchoice
    +
     config BR2_PACKAGE_UACME_UALPN
     	bool "enable ualpn"
     	depends on BR2_TOOLCHAIN_HAS_THREADS
    +	depends on !BR2_PACKAGE_UACME_CRYPTO_MBEDTLS
     	select BR2_PACKAGE_LIBEV
     	help
     	  Build and install ualpn, the transparent proxying tls-alpn-01
     	  challenge responder.
     
    +comment "ualpn incompatible with the mbedtls crypto backend"
    +	depends on BR2_TOOLCHAIN_HAS_THREADS
    +	depends on !BR2_PACKAGE_UACME_CRYPTO_MBEDTLS
    +
     comment "ualpn needs a toolchain w/ threads"
     	depends on !BR2_TOOLCHAIN_HAS_THREADS
     

Regards,
Yann E. MORIN.

> > Thomas
> > --
> > Thomas Petazzoni, CTO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> Best Regards,
> 
> Fabrice

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v4, 1/1] package/uacme: don't allow mbedtls with ualpn

[Thomas Petazzoni]

On Sun, 26 Apr 2020 21:21:30 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> > > Sorry, but this is still not correct: enable mbedtls, then enable uacme
> > > and ualpn: there is no crypto backend selectable in the choice...  
> > Do we need a choice in the first place ?  
> 
> We already have a few packages (libcurl, libssh, libssh2) that have a
> choice for the crypto backend, so why not for uacme?

Are they really needed? Don't we generally try to use automatic
dependencies when possible?

We also have plenty of packages without any choice, for example:

ifeq ($(BR2_PACKAGE_GNUTLS),y)
LIBNICE_CONF_OPTS += --with-crypto-library=gnutls
LIBNICE_DEPENDENCIES += gnutls
else
LIBNICE_CONF_OPTS += \
        --with-crypto-library=openssl \
        --with-openssl=$(STAGING_DIR)/usr
LIBNICE_DEPENDENCIES += openssl
endif

ifeq ($(BR2_PACKAGE_GNUTLS),y)
MSMTP_CONF_OPTS += --with-tls=gnutls
MSMTP_DEPENDENCIES += gnutls
else ifeq ($(BR2_PACKAGE_OPENSSL),y)
MSMTP_CONF_OPTS += --with-tls=openssl
MSMTP_DEPENDENCIES += openssl
else
MSMTP_CONF_OPTS += --with-tls=no
endif

etc, etc.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com