smtpd_tls_chain_files and EC PARAMETERS

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

smtpd_tls_chain_files and EC PARAMETERS

Moviuro
Hi all,

As reported on 2019-11-08 on IRC, I have issues with ECC certificates in
smtpd_tls_chain_files, which don't happen with the older
smtpd_tls_eccert_file and smtpd_tls_eckey_file.

I use acme.sh [0] to renew my certificates from let's encrypt: crontab extract:
@weekly /usr/local/sbin/acme.sh --renew --dns dns_ovh -d mail.domain.tld --keylength ec-256 --cert-file /usr/local/etc/ssl/mail.domain.tld/ecc.crt --key-file /usr/local/etc/ssl/mail.domain.tld/ecc.key --ca-file /usr/local/etc/ssl/mail.domain.tld/ecc.ca.crt --fullchain-file /usr/local/etc/ssl/mail.domain.tld/ecc.fullchain.cer
@weekly /usr/local/sbin/acme.sh --renew --dns dns_ovh -d mail.domain.tld --cert-file /usr/local/etc/ssl/mail.domain.tld/rsa.crt --key-file /usr/local/etc/ssl/mail.domain.tld/rsa.key --ca-file /usr/local/etc/ssl/mail.domain.tld/rsa.ca.crt --fullchain-file /usr/local/etc/ssl/mail.domain.tld/rsa.fullchain.cer

This generates/updates the following files:
# ecc.ca.crt / ecc.crt / rsa.ca.crt / rsa.crt look like:
-----BEGIN CERTIFICATE-----
...
-----END CERTIFICATE-----
# ecc.fullchain.cer / rsa.fullchain.cer looks like:
-----BEGIN CERTIFICATE-----
...
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
...
-----END CERTIFICATE-----
# ecc.key is: (note the EC PARAMETERS object)
-----BEGIN EC PARAMETERS-----
...
-----END EC PARAMETERS-----
-----BEGIN EC PRIVATE KEY-----
...
-----END EC PRIVATE KEY-----
# rsa.key is:
-----BEGIN RSA PRIVATE KEY-----
...
-----END RSA PRIVATE KEY-----

According to the documentation, smtpd_tls_chain_files is the preferred
way of handling TLS files now, so I use:

smtpd_tls_chain_files =
  /usr/local/etc/ssl/mail.domain.tld/rsa.key,
  /usr/local/etc/ssl/mail.domain.tld/rsa.fullchain.cer

which works well:

 % openssl s_client -showcerts -connect mail.domain.tld:465
 ...
 No client certificate CA names sent
 Peer signing digest: SHA256
 Peer signature type: RSA-PSS
 Server Temp Key: X25519, 253 bits
 ...

However, if I add my ECC key and certificate...

smtpd_tls_chain_files =
  /usr/local/etc/ssl/mail.domain.tld/ecc.key,
  /usr/local/etc/ssl/mail.domain.tld/ecc.fullchain.cer,
  /usr/local/etc/ssl/mail.domain.tld/rsa.key,
  /usr/local/etc/ssl/mail.domain.tld/rsa.fullchain.cer

 % openssl s_client -showcerts -connect mail.domain.tld:465
 CONNECTED(00000003)
 write:errno=0
 ---
 no peer certificate available
 ---
 No client certificate CA names sent
 ---
 SSL handshake has read 0 bytes and written 314 bytes
 Verification: OK
 ---
 New, (NONE), Cipher is (NONE)
 Secure Renegotiation IS NOT supported
 Compression: NONE
 Expansion: NONE
 No ALPN negotiated
 Early data was not sent
 Verify return code: 0 (ok)
 ---

Mail log also has an error:

  Nov  8 21:32:25 mail postfix/smtps/smtpd[3146]: warning: error loading /usr/local/etc/ssl/mail.domain.tld/ecc.key: unexpected PEM type: EC PARAMETERS

This lead me to [1] where the author simply removes the EC PARAMETERS to
get a working file. When I then try to remove the EC PARAMETERS object
in ecc.key and use smtpd_tls_chain_files, I get no such issue:

 % openssl s_client -showcerts -connect mail.domain.tld:465
 ...
 No client certificate CA names sent
 Peer signing digest: SHA256
 Peer signature type: ECDSA
 Server Temp Key: X25519, 253 bits
 ...

Which looks like it's working correctly. So the EC PARAMETERS object
breaks an otherwise fine key file for postfix.

However, before reading [1], I tried the "old" method with
smtpd_tls_eccert_file and smtpd_tls_eckey_file and my ecc.key file (with
EC PARAMETERS still in it):

smtpd_tls_eccert_file = /usr/local/etc/ssl/mail.domain.tld/ecc.fullchain.cer
smtpd_tls_eckey_file = /usr/local/etc/ssl/mail.domain.tld/ecc.key

This also works with no issue.

So:
* smtpd_tls_chain_files can't handle the EC PARAMETERS object of the
  ecc.key file
* however previous configuration options could

With my little understanding of C code, I suspect that fixing this bug
should be made around lines 407..418 of src/tls/tls_certkey.c [2], to
silently ignore the EC PARAMETERS object if encountered (though ignoring
those parameters may have consequences I know nothing of).

I could also edit my key file, but as it's valid, it seems weird to do
it. And because the key was generated by a third-party tool ([0]), more
people will encounter the issue if they use it too, same as I did to get
my certificates.

For the time being, I have disabled ECC certificates on my postfix
instance - and keep the knowledge of how to get it to work if it's ever
needed one day...

[0] https://acme.sh
[1] https://gitmemory.com/issue/lukas2511/dehydrated/660/514896977
[2] https://github.com/vdukhovni/postfix/blob/v3.4.7/postfix/src/tls/tls_certkey.c#L407-L418
Reply | Threaded
Open this post in threaded view
|

Re: smtpd_tls_chain_files and EC PARAMETERS

Viktor Dukhovni
On Fri, Nov 08, 2019 at 10:03:55PM +0100, Moviuro wrote:

> Hi all,
>
> # ecc.key is: (note the EC PARAMETERS object)
> -----BEGIN EC PARAMETERS-----
> ...
> -----END EC PARAMETERS-----
> -----BEGIN EC PRIVATE KEY-----
> ...
> -----END EC PRIVATE KEY-----

> According to the documentation, smtpd_tls_chain_files is the preferred
> way of handling TLS files now, so I use:
>
> smtpd_tls_chain_files =
>   /usr/local/etc/ssl/mail.domain.tld/rsa.key,
>   /usr/local/etc/ssl/mail.domain.tld/rsa.fullchain.cer

Yes, but that file format is Postfix-specific, and requires that
the key precede its certificate chain, and *only* supports with
keys and associated certificates, and no other objects.

    http://www.postfix.org/postconf.5.html#smtpd_tls_chain_files

The documentation does not mention support for ignoring unrelated
PEM objects.

> Mail log also has an error:
>
>   Nov  8 21:32:25 mail postfix/smtps/smtpd[3146]: warning:
>     error loading /usr/local/etc/ssl/mail.domain.tld/ecc.key:
>     unexpected PEM type: EC PARAMETERS

This is expected.

> Which looks like it's working correctly. So the EC PARAMETERS object
> breaks an otherwise fine key file for Postfix.

Yes, the chain files are not expected to hold spurious objects.

> With my little understanding of C code, I suspect that fixing this bug
> should be made around lines 407..418 of src/tls/tls_certkey.c [2], to
> silently ignore the EC PARAMETERS object if encountered (though ignoring
> those parameters may have consequences I know nothing of).

For the record, there is no bug, the file format does not support
spurious objects.  This file format is also used for SNI, where all
the PEM blobs are loaded into memory or "compiled" into an indexed
file, and should not really drag along unintended data.

> I could also edit my key file, but as it's valid, it seems weird to do
> it. And because the key was generated by a third-party tool ([0]), more
> people will encounter the issue if they use it too, same as I did to get
> my certificates.

Your key + chain file should be constructed atomically from the
pieces delivered by certbot et. al. and moved (rename(2)) into place
once it is verified to hold a matching key and certificate.

A sensible feature request would be to ask for (or better yet
contribute) a new "postfix tls" script sub-command that creates a
"key + chain" file from a standard PEM file with a key, a standard
PEM file with the cert + optional chain and an optional file with
even more of the chain.  The script would verify that the all bits
are sane, and then combine the (key, cert, chain) triple into a
single file with just the expected objects.

It is of course also possible to change to the code to ignore
unexpected PEM objects in the chain file, perhaps that's less
surprising, but it also leads to surprised if the objects were
supposed to be used, but were not recognized.

So there's a trade-off here, and I could perhaps be persuaded
that a more liberal parser is needed.

--
        Viktor.
Reply | Threaded
Open this post in threaded view
|

Re: smtpd_tls_chain_files and EC PARAMETERS

Wietse Venema
Viktor Dukhovni:

> On Fri, Nov 08, 2019 at 10:03:55PM +0100, Moviuro wrote:
>
> > Hi all,
> >
> > # ecc.key is: (note the EC PARAMETERS object)
> > -----BEGIN EC PARAMETERS-----
> > ...
> > -----END EC PARAMETERS-----
> > -----BEGIN EC PRIVATE KEY-----
> > ...
> > -----END EC PRIVATE KEY-----

This is a usability problem that I think should be addressed in
Postfix 3.4. Either we provide a tool that produces the expected
inputs, or we add code to recognize and ignore known-harmless
objects that appear in output from other tools.

I think that adding code to ignore known-harmless content is
the safer approach because it makes Postfix interoperable with
other tools.

What other examples of known-harmless content can people expect to
see? Should the list be configurable? If all these blobs embedded
beween lines

-----BEGIN TYPE OF OBJECT-----

-----END TYPE OF OBJECT-----

then it can be purely mechanical.

        Wietse
Reply | Threaded
Open this post in threaded view
|

Re: smtpd_tls_chain_files and EC PARAMETERS

Viktor Dukhovni
On Sat, Nov 09, 2019 at 08:07:51AM -0500, Wietse Venema wrote:

> What other examples of known-harmless content can people expect to
> see? Should the list be configurable? If all these blobs embedded
> beween lines
>
> -----BEGIN TYPE OF OBJECT-----
>
> -----END TYPE OF OBJECT-----
>
> then it can be purely mechanical.

The OpenSSL PEM file parser already ignores content outside of
BEGIN/END boundaries, so the minimal patch to silently ignore
unexpected PEM data would be:

--- src/tls/tls_certkey.c
+++ src/tls/tls_certkey.c
@@ -412,9 +412,6 @@ static int load_pem_object(pem_load_state_t *st)
        || ((pkey_type = EVP_PKEY_DSA) != NID_undef
    && strcmp(name, PEM_STRING_DSA) == 0)) {
  load_pkey(st, pkey_type, buf, buflen);
-    } else if (!st->mixed) {
- msg_warn("error loading %s: unexpected PEM type: %s", st->source, name);
- st->state = PEM_LOAD_STATE_NOGO;
     }
     OPENSSL_free(name);
     OPENSSL_free(header);

On an mostly unrelated note, OpenSSL 3.0 (~Q4 2020) is changing the
error API, so we'll eventually need:

--- src/tls/tls_misc.c
+++ src/tls/tls_misc.c
@@ -1332,6 +1332,18 @@ void    tls_print_errors(void)
     int     line;
     int     flags;
 
+#if defined(OPENSSL_VERSION_PREREQ) && OPENSSL_VERSION_PREREQ(3,0)
+    const char *func;
+
+    while ((err = ERR_get_error_all(&file, &line, &func, &data, &flags)) != 0) {
+ ERR_error_string_n(err, buffer, sizeof(buffer));
+ if (flags & ERR_TXT_STRING)
+    msg_warn("TLS library problem: %s:%s:%s:%d:%s:",
+     buffer, file, func, line, data);
+ else
+    msg_warn("TLS library problem: %s:%s:%s:%d:", buffer, file, func, line);
+    }
+#else
     while ((err = ERR_get_error_line_data(&file, &line, &data, &flags)) != 0) {
  ERR_error_string_n(err, buffer, sizeof(buffer));
  if (flags & ERR_TXT_STRING)
@@ -1340,6 +1352,7 @@ void    tls_print_errors(void)
  else
     msg_warn("TLS library problem: %s:%s:%d:", buffer, file, line);
     }
+#endif
 }
 
 /* tls_info_callback - callback for logging SSL events via Postfix */

--
        Viktor.
Reply | Threaded
Open this post in threaded view
|

Re: smtpd_tls_chain_files and EC PARAMETERS

John Stoffel-2
>>>>> "Viktor" == Viktor Dukhovni <[hidden email]> writes:

Viktor> On an mostly unrelated note, OpenSSL 3.0 (~Q4 2020) is changing the
Viktor> error API, so we'll eventually need:

Viktor> --- src/tls/tls_misc.c
Viktor> +++ src/tls/tls_misc.c
Viktor> @@ -1332,6 +1332,18 @@ void    tls_print_errors(void)
Viktor>      int     line;
Viktor>      int     flags;
 
Viktor> +#if defined(OPENSSL_VERSION_PREREQ) && OPENSSL_VERSION_PREREQ(3,0)
Viktor> +    const char *func;
Viktor> +
Viktor> +    while ((err = ERR_get_error_all(&file, &line, &func, &data, &flags)) != 0) {
Viktor> + ERR_error_string_n(err, buffer, sizeof(buffer));
Viktor> + if (flags & ERR_TXT_STRING)
Viktor> +    msg_warn("TLS library problem: %s:%s:%s:%d:%s:",
Viktor> +     buffer, file, func, line, data);
Viktor> + else
Viktor> +    msg_warn("TLS library problem: %s:%s:%s:%d:", buffer, file, func, line);

Can we be more specific here with what the problem is?  Maybe the
'data' entry has more details that I'm missing here.  

Reply | Threaded
Open this post in threaded view
|

Re: smtpd_tls_chain_files and EC PARAMETERS

Wietse Venema
John Stoffel:

> >>>>> "Viktor" == Viktor Dukhovni <[hidden email]> writes:
>
> Viktor> On an mostly unrelated note, OpenSSL 3.0 (~Q4 2020) is changing the
> Viktor> error API, so we'll eventually need:
>
> Viktor> --- src/tls/tls_misc.c
> Viktor> +++ src/tls/tls_misc.c
> Viktor> @@ -1332,6 +1332,18 @@ void    tls_print_errors(void)
> Viktor>      int     line;
> Viktor>      int     flags;
>  
> Viktor> +#if defined(OPENSSL_VERSION_PREREQ) && OPENSSL_VERSION_PREREQ(3,0)
> Viktor> +    const char *func;
> Viktor> +
> Viktor> +    while ((err = ERR_get_error_all(&file, &line, &func, &data, &flags)) != 0) {
> Viktor> + ERR_error_string_n(err, buffer, sizeof(buffer));
> Viktor> + if (flags & ERR_TXT_STRING)
> Viktor> +    msg_warn("TLS library problem: %s:%s:%s:%d:%s:",
> Viktor> +     buffer, file, func, line, data);
> Viktor> + else
> Viktor> +    msg_warn("TLS library problem: %s:%s:%s:%d:", buffer, file, func, line);
>
> Can we be more specific here with what the problem is?  Maybe the
> 'data' entry has more details that I'm missing here.  

You mean, eliminate the 'flags & ERR_TXT_STRING' test?

        Wietse
Reply | Threaded
Open this post in threaded view
|

Re: smtpd_tls_chain_files and EC PARAMETERS

John Stoffel-2
>>>>> "Wietse" == Wietse Venema <[hidden email]> writes:

Wietse> John Stoffel:
>> >>>>> "Viktor" == Viktor Dukhovni <[hidden email]> writes:
>>
Viktor> On an mostly unrelated note, OpenSSL 3.0 (~Q4 2020) is changing the
Viktor> error API, so we'll eventually need:
>>
Viktor> --- src/tls/tls_misc.c
Viktor> +++ src/tls/tls_misc.c
Viktor> @@ -1332,6 +1332,18 @@ void    tls_print_errors(void)
Viktor> int     line;
Viktor> int     flags;
>>
Viktor> +#if defined(OPENSSL_VERSION_PREREQ) && OPENSSL_VERSION_PREREQ(3,0)
Viktor> +    const char *func;
Viktor> +
Viktor> +    while ((err = ERR_get_error_all(&file, &line, &func, &data, &flags)) != 0) {
Viktor> + ERR_error_string_n(err, buffer, sizeof(buffer));
Viktor> + if (flags & ERR_TXT_STRING)
Viktor> +    msg_warn("TLS library problem: %s:%s:%s:%d:%s:",
Viktor> +     buffer, file, func, line, data);
Viktor> + else
Viktor> +    msg_warn("TLS library problem: %s:%s:%s:%d:", buffer, file, func, line);
>>
>> Can we be more specific here with what the problem is?  Maybe the
>> 'data' entry has more details that I'm missing here.  

Wietse> You mean, eliminate the 'flags & ERR_TXT_STRING' test?

More the msg_warn("TLS library problem: ...") is what I'm commenting
on, but I suspect that the needed info is in the 'data' string passed
in for error reporting.

Thanks to both you and Victor for all you've done with postfix,
Excellent software!
Reply | Threaded
Open this post in threaded view
|

Re: smtpd_tls_chain_files and EC PARAMETERS

Wietse Venema
In reply to this post by Viktor Dukhovni
Viktor Dukhovni:

> The OpenSSL PEM file parser already ignores content outside of
> BEGIN/END boundaries, so the minimal patch to silently ignore
> unexpected PEM data would be:
>
> --- src/tls/tls_certkey.c
> +++ src/tls/tls_certkey.c
> @@ -412,9 +412,6 @@ static int load_pem_object(pem_load_state_t *st)
>         || ((pkey_type = EVP_PKEY_DSA) != NID_undef
>     && strcmp(name, PEM_STRING_DSA) == 0)) {
>   load_pkey(st, pkey_type, buf, buflen);
> -    } else if (!st->mixed) {
> - msg_warn("error loading %s: unexpected PEM type: %s", st->source, name);
> - st->state = PEM_LOAD_STATE_NOGO;
>      }
>      OPENSSL_free(name);
>      OPENSSL_free(header);

I have adopted this for interoperability, but will log a warning
because ignoring inputs is not safe,

> On an mostly unrelated note, OpenSSL 3.0 (~Q4 2020) is changing the
> error API, so we'll eventually need:
>
> --- src/tls/tls_misc.c
> +++ src/tls/tls_misc.c
> @@ -1332,6 +1332,18 @@ void    tls_print_errors(void)
>      int     line;
>      int     flags;
>  
> +#if defined(OPENSSL_VERSION_PREREQ) && OPENSSL_VERSION_PREREQ(3,0)

Unfortunately that does not compile. Maybe we need a helper macro
like HAVE_GLIBC_API_VERSION_SUPPORT() in sys_defs.h.

        Wietse