bug#26948: gnutls errors on multiple guix commands

[Mark H Weaver <mhw@netris.org>]

Hi Ludovic,

ludo@gnu.org (Ludovic Courtès) writes:

> Mark H Weaver <mhw@netris.org> skribis:
>
>> This reminds me of a bug that I found in the Guile binding in GnuTLS a
>> while ago, but forgot to report.  Maybe it's related:
>>
>> In 'set_certificate_file' in gnutls-3.5.9/guile/src/core.c:
>>
>>   static unsigned int
>>   set_certificate_file (certificate_set_file_function_t set_file,
>>                         SCM cred, SCM file, SCM format, const char *func_name)
>>   #define FUNC_NAME func_name
>>   {
>>     int err;
>>     char *c_file;
>>     size_t c_file_len;
>>   
>>     gnutls_certificate_credentials_t c_cred;
>>     gnutls_x509_crt_fmt_t c_format;
>>   
>>     c_cred = scm_to_gnutls_certificate_credentials (cred, 1, FUNC_NAME);
>>     SCM_VALIDATE_STRING (2, file);
>>     c_format = scm_to_gnutls_x509_certificate_format (format, 3, FUNC_NAME);
>>   
>>     c_file_len = scm_c_string_length (file);
>>     c_file = alloca (c_file_len + 1);
>>   
>>     (void) scm_to_locale_stringbuf (file, c_file, c_file_len + 1);
>>     c_file[c_file_len] = '\0';
>>   
>>     err = set_file (c_cred, c_file, c_format);
>>     if (EXPECT_FALSE (err < 0))
>>       scm_gnutls_error (err, FUNC_NAME);
>>   
>>     /* Return the number of certificates processed.  */
>>     return ((unsigned int) err);
>>   }
>>
>> 'scm_c_string_length' is inappropriately assumed to return the length
>> of the encoded C string in bytes, whereas it actually returns the
>> number of characters (code points).
>
> This is terrible!  WDYT of this:
>
> diff --git a/guile/src/core.c b/guile/src/core.c
> index 605c91f7a..38d573fa9 100644
> --- a/guile/src/core.c
> +++ b/guile/src/core.c
> @@ -1,5 +1,5 @@
>  /* GnuTLS --- Guile bindings for GnuTLS.
> -   Copyright (C) 2007-2014, 2016 Free Software Foundation, Inc.
> +   Copyright (C) 2007-2014, 2016-2017 Free Software Foundation, Inc.
>  
>     GnuTLS is free software; you can redistribute it and/or
>     modify it under the terms of the GNU Lesser General Public
> @@ -1428,22 +1428,19 @@ set_certificate_file (certificate_set_file_function_t set_file,
>  {
>    int err;
>    char *c_file;
> -  size_t c_file_len;
>  
>    gnutls_certificate_credentials_t c_cred;
>    gnutls_x509_crt_fmt_t c_format;
>  
>    c_cred = scm_to_gnutls_certificate_credentials (cred, 1, FUNC_NAME);
>    SCM_VALIDATE_STRING (2, file);
> -  c_format = scm_to_gnutls_x509_certificate_format (format, 3, FUNC_NAME);
> -
> -  c_file_len = scm_c_string_length (file);
> -  c_file = alloca (c_file_len + 1);
>  
> -  (void) scm_to_locale_stringbuf (file, c_file, c_file_len + 1);
> -  c_file[c_file_len] = '\0';
> +  c_format = scm_to_gnutls_x509_certificate_format (format, 3, FUNC_NAME);
> +  c_file = scm_to_locale_string (file);
>  
>    err = set_file (c_cred, c_file, c_format);
> +  free (c_file);
> +
>    if (EXPECT_FALSE (err < 0))
>      scm_gnutls_error (err, FUNC_NAME);
>  

Looks good to me.  In the case when a UTF-8 locale is active, and where
Guile 2.0.12 or later is available, we could use
'scm_c_string_utf8_length' to find the length in bytes, but optimizing
that case is probably not worth the extra code complexity.

> Unfortunately there’s a dozen of places in core.c that use this idiom
> and would need to be fixed (it’s pre-2.0 code I think).

Bummer.

> In the meantime we can work around it this way:
>
> diff --git a/guix/build/download.scm b/guix/build/download.scm
> index ce4708a87..6ef623334 100644
> --- a/guix/build/download.scm
> +++ b/guix/build/download.scm
> @@ -296,6 +296,13 @@ session record port using PORT as its underlying communication port."
>    (make-parameter (or (getenv "GUIX_TLS_CERTIFICATE_DIRECTORY")
>                        (getenv "SSL_CERT_DIR"))))  ;like OpenSSL
>  
> +(define (set-certificate-credentials-x509-trust-file!* cred file format)
> +  "Like 'set-certificate-credentials-x509-trust-file!', but without the file
> +name decoding bug described at
> +<https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26948#17>."
> +  (let ((data (call-with-input-file file get-bytevector-all)))
> +    (set-certificate-credentials-x509-trust-data! cred data format)))
> +
>  (define (make-credendials-with-ca-trust-files directory)
>    "Return certificate credentials with X.509 authority certificates read from
>  DIRECTORY.  Those authority certificates are checked when
> @@ -309,7 +316,7 @@ DIRECTORY.  Those authority certificates are checked when
>                  (let ((file (string-append directory "/" file)))
>                    ;; Protect against dangling symlinks.
>                    (when (file-exists? file)
> -                    (set-certificate-credentials-x509-trust-file!
> +                    (set-certificate-credentials-x509-trust-file!*
>                       cred file
>                       x509-certificate-format/pem))))
>                (or files '()))
>
>
> WDYT?  I’ll commit it if that’s fine with you.

I'm not sufficiently familiar with GnuTLS to properly review this, but I
trust your judgement.

     Thanks!
       Mark