Re: master 583995c: GnuTLS HMAC and symmetric cipher support

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Glenn Morris-3
Teodor Zlatanov wrote:

> branch: master
> commit 583995c62dd424775dda33d5134ce04bee2ae685

Hi, this change apparently causes gnutls tests to segfault on hydra.
I can't give you any more information than is accessible from

http://hydra.nixos.org/build/56430842

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Ted Zlatanov
On Sun, 16 Jul 2017 20:05:15 -0400 Glenn Morris <[hidden email]> wrote:

GM> Teodor Zlatanov wrote:
>> branch: master
>> commit 583995c62dd424775dda33d5134ce04bee2ae685

GM> Hi, this change apparently causes gnutls tests to segfault on hydra.
GM> I can't give you any more information than is accessible from

GM> http://hydra.nixos.org/build/56430842

Thanks, Glenn. The logs have nothing useful unfortunately[1].

I'm not looking to install Nix on my personal machine to debug this,
which seems to be necessary in order to replicate the issue. I also
don't have a login to any machines that can run Nix. Can you or anyone
else recommend a way to replicate the issue using a Docker or LXC
container or other means? Or give me a login to the Hydra build farm?

Thanks
Ted

[1] The log view link in Hydra doesn't work with eww. That's not a big
deal but I thought I'd mention it.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Michael Albinus
Ted Zlatanov <[hidden email]> writes:

Hi Ted,

> Thanks, Glenn. The logs have nothing useful unfortunately[1].

For Tramp, I have modified the Makefile to get continuous print of the
log to stdout. This helps in cases Emacs crashes (your case) or blocks
(Tramp case).

We could extend this for GnuTLS tests. Ping me if you want me to do
this (or check my changes in test/Makefile.in).

> Thanks
> Ted

Best regards, Michael.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Ted Zlatanov
On Mon, 17 Jul 2017 16:30:38 +0200 Michael Albinus <[hidden email]> wrote:

MA> For Tramp, I have modified the Makefile to get continuous print of the
MA> log to stdout. This helps in cases Emacs crashes (your case) or blocks
MA> (Tramp case).

MA> We could extend this for GnuTLS tests. Ping me if you want me to do
MA> this (or check my changes in test/Makefile.in).

I'd love this. Maybe there could be a way of triggering this mode from
Hydra's interface, so it's not always on. If not, then yes, please make
the change. I assume it will be related to

    WRITE_LOG = $(if $(and ${NIX_STORE}, $(findstring tramp, $@)), |& tee $@, > $@ 2>&1) \

but a $(or) call just for GnuTLS would duplicate quite a bit of code,
and $(findstring) doesn't do regexes. Maybe you can decide :)

Thanks again
Ted


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Glenn Morris-3
In reply to this post by Ted Zlatanov

Please stop dropping me from the cc. :(

Ted Zlatanov wrote:

>>> branch: master
>>> commit 583995c62dd424775dda33d5134ce04bee2ae685
>
> GM> Hi, this change apparently causes gnutls tests to segfault on hydra.
> GM> I can't give you any more information than is accessible from
>
> GM> http://hydra.nixos.org/build/56430842
>
> Thanks, Glenn. The logs have nothing useful unfortunately[1].

The logs say that gnutls 3.2.21 was used.
I installed that version on rhel 7, and trivially reproduced the crash
when loading the gnutls tests. Backtrace attached. Maybe if you install
that version, it will crash for you too...


gdb.txt.xz (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

npostavs
On Mon, Jul 17, 2017 at 1:12 PM, Glenn Morris <[hidden email]> wrote:
>
> The logs say that gnutls 3.2.21 was used.

How do you find this info from the logs? I searched for "gnutls" and
found only stuff like:

checking for LIBGNUTLS... yes
checking for LIBGNUTLS3... yes

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Glenn Morris-3
Noam Postavsky wrote:

> On Mon, Jul 17, 2017 at 1:12 PM, Glenn Morris <[hidden email]> wrote:
>>
>> The logs say that gnutls 3.2.21 was used.
>
> How do you find this info from the logs?

Sorry, the "build dependencies" tab, not the actual build log.

The crash occurs when gca = GNUTLS_CIPHER_UNKNOWN and
gnutls_cipher_get_name = NULL.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Michael Albinus
In reply to this post by Ted Zlatanov
Ted Zlatanov <[hidden email]> writes:

> I'd love this. Maybe there could be a way of triggering this mode from
> Hydra's interface, so it's not always on. If not, then yes, please make
> the change.

I'll wait a little bit, whether you could reproduce the problem with
Glenn's hint.

> Thanks again
> Ted

Best regards, Michael.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

npostavs
In reply to this post by Glenn Morris-3
On Mon, Jul 17, 2017 at 2:20 PM, Glenn Morris <[hidden email]> wrote:
> Noam Postavsky wrote:
>
>> On Mon, Jul 17, 2017 at 1:12 PM, Glenn Morris <[hidden email]> wrote:
>>>
>>> The logs say that gnutls 3.2.21 was used.
>>
>> How do you find this info from the logs?
>
> Sorry, the "build dependencies" tab, not the actual build log.

Oh, this is confusing; the build that you linked to,
http://hydra.nixos.org/build/56430842, doesn't have a "build
dependencies" tab.

Although http://hydra.nixos.org/build/56475235 for example, does.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Glenn Morris-3
Noam Postavsky wrote:

> http://hydra.nixos.org/build/56430842, doesn't have a "build
> dependencies" tab.
>
> Although http://hydra.nixos.org/build/56475235 for example, does.

Oh, weird. Maybe this information gets garbage-collected fairly
rapidly to save space. Just guessing though.


After fixing the crash issue, test-gnutls-004-symmetric-ciphers fails with:

  (error "GnuTLS cipher IDEA-PGP-CFB/encrypt initialization failed: The
  request is invalid.")


BTW I note that gnutls_mac_get_name and gnutls_digest_get_name can
also apparently return NULL, and are currently passed to "intern"
without checking for this. I don't know if this can actually crash in
practice though.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Paul Eggert
On 07/17/2017 12:27 PM, Glenn Morris wrote:
> gnutls_mac_get_name and gnutls_digest_get_name can
> also apparently return NULL

As I recall, they can't return NULL in the contexts where they're used,
as a previous call already established the validities of their arguments.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Glenn Morris-3
Paul Eggert wrote:

>> gnutls_mac_get_name and gnutls_digest_get_name can
>> also apparently return NULL
>
> As I recall, they can't return NULL in the contexts where they're
> used, as a previous call already established the validities of their
> arguments.

It's not obvious to me that 2 and 3 below are any different to 1, save
for the fact that GNUTLS_CIPHER_NULL == 1 (and GNUTLS_CIPHER_UNKNOWN ==
0, which is maybe what happens to stop 2 and 3 crashing in the same way
1 does. Seems a bit like a happy accident?):


1. Fgnutls_ciphers. Can intern NULL and crash.
const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
{
  gnutls_cipher_algorithm_t gca = gciphers[pos];
  Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
  ...
}


2. Fgnutls_macs.
const gnutls_mac_algorithm_t *macs = gnutls_mac_list ();
for (ptrdiff_t pos = 0; macs[pos] != 0; pos++)
{
  const gnutls_mac_algorithm_t gma = macs[pos];
  Lisp_Object gma_symbol = intern (gnutls_mac_get_name (gma));
  ...
}


3. Fgnutls_digests.
const gnutls_digest_algorithm_t *digests = gnutls_digest_list ();
for (ptrdiff_t pos = 0; digests[pos] != 0; pos++)
{
  const gnutls_digest_algorithm_t gda = digests[pos];
  Lisp_Object gda_symbol = intern (gnutls_digest_get_name (gda));
  ...
}

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Paul Eggert
Glenn Morris wrote:

> Seems a bit like a happy accident?):

It's not an accident, as every valid cipher etc. has a name.

> 1. Fgnutls_ciphers. Can intern NULL and crash.
> const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
> for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
> {
>    gnutls_cipher_algorithm_t gca = gciphers[pos];
>    Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
>    ...
> }

It's OK, as every cipher other than GNUTLS_CIPHER_NULL has a non-NULL name.

> 2. Fgnutls_macs.
> const gnutls_mac_algorithm_t *macs = gnutls_mac_list ();
> for (ptrdiff_t pos = 0; macs[pos] != 0; pos++)
> {
>    const gnutls_mac_algorithm_t gma = macs[pos];
>    Lisp_Object gma_symbol = intern (gnutls_mac_get_name (gma));
>    ...
> }

Also OK, as every mac algorithm has a non-NULL name.

> 3. Fgnutls_digests.
> const gnutls_digest_algorithm_t *digests = gnutls_digest_list ();
> for (ptrdiff_t pos = 0; digests[pos] != 0; pos++)
> {
>    const gnutls_digest_algorithm_t gda = digests[pos];
>    Lisp_Object gda_symbol = intern (gnutls_digest_get_name (gda));
>    ...
> }

Likewise.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Glenn Morris-3
Paul Eggert wrote:

> Glenn Morris wrote:
>
>> Seems a bit like a happy accident?):
>
> It's not an accident, as every valid cipher etc. has a name.
>
>> 1. Fgnutls_ciphers. Can intern NULL and crash.
>> const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
>> for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
>> {
>>    gnutls_cipher_algorithm_t gca = gciphers[pos];
>>    Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
>>    ...
>> }
>
> It's OK, as every cipher other than GNUTLS_CIPHER_NULL has a non-NULL name.

But it isn't OK. That's what this thread is about.
GNUTLS_CIPHER_UNKNOWN apparently has a NULL name.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Paul Eggert
Glenn Morris wrote:
>> It's OK, as every cipher other than GNUTLS_CIPHER_NULL has a non-NULL name.
> But it isn't OK. That's what this thread is about.
> GNUTLS_CIPHER_UNKNOWN apparently has a NULL name.

Sorry, I still don't see the problem here. From what I can see,
GNUTLS_CIPHER_NULL is not a cipher (or to be more precise, it is not a valid
GnuTLS cipher ID). The code in question:

> const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
> for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
> {
>   gnutls_cipher_algorithm_t gca = gciphers[pos];
>   Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));

looks only at GnuTLS cipher IDs returned by gnutls_cipher_list, and
GNUTLS_CIPHER_UNKNOWN is not one of those cipher IDs.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Glenn Morris-3
Paul Eggert wrote:

> looks only at GnuTLS cipher IDs returned by gnutls_cipher_list, and
> GNUTLS_CIPHER_UNKNOWN is not one of those cipher IDs.

I feel like you've missed the first half of the discussion, so I'll
summarize it:

Since 583995c, make check on hydra crashes when loading the gnutls tests.
It uses gnutls 3.2.21. I installed that on my rhel7 system (with
--disable-non-suiteb-curves) and reproduced the crash. The backtrace is
at http://lists.gnu.org/archive/html/emacs-devel/2017-07/msg00716.html .

Specifically, I find it crashes because gnutls_cipher_list returns a list
containing GNUTLS_CIPHER_UNKNOWN.

The following patch fixes it for me:

*** /tmp/uOhC3f_gnutls.c 2017-07-17 19:32:32.851361998 -0700
--- src/gnutls.c 2017-07-17 12:03:27.514906186 -0700
***************
*** 1857,1870 ****
    for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
      {
        gnutls_cipher_algorithm_t gca = gciphers[pos];
 
        /* A symbol representing the GnuTLS cipher.  */
!       Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
 
!       ptrdiff_t cipher_tag_size = gnutls_cipher_get_tag_size (gca);
 
!       Lisp_Object cp
! = listn (CONSTYPE_HEAP, 15, cipher_symbol,
  QCcipher_id, make_number (gca),
  QCtype, Qgnutls_type_cipher,
  QCcipher_aead_capable, cipher_tag_size == 0 ? Qnil : Qt,
--- 1857,1877 ----
    for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
      {
        gnutls_cipher_algorithm_t gca = gciphers[pos];
+       const char *cipher_name;
+       Lisp_Object cipher_symbol, cp;
+       ptrdiff_t cipher_tag_size;
+
+       cipher_name = gnutls_cipher_get_name (gca);
+
+       if (! cipher_name)
+         continue;
 
        /* A symbol representing the GnuTLS cipher.  */
!       cipher_symbol = intern (cipher_name);
 
!       cipher_tag_size = gnutls_cipher_get_tag_size (gca);
 
!       cp = listn (CONSTYPE_HEAP, 15, cipher_symbol,
                    QCcipher_id, make_number (gca),
                    QCtype, Qgnutls_type_cipher,
                    QCcipher_aead_capable, cipher_tag_size == 0 ? Qnil : Qt,

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Paul Eggert
Glenn Morris wrote:
> Since 583995c, make check on hydra crashes when loading the gnutls tests.
> It uses gnutls 3.2.21. I installed that on my rhel7 system (with
> --disable-non-suiteb-curves) and reproduced the crash. The

I don't see how the crash can occur with vanilla GnuTLS 3.2.21, as its
gnutls_cipher_list returns a list of IDs that does not contain
GNUTLS_CIPHER_UNKNOWN. Perhaps you were using a modified GnuTLS 3.2.21. Or
possibly I'm misreading the GnuTLS source code, though I don't see how.

As you're observing the problem I installed the attached, which is similar to
the patch that worked for you.

0001-Port-gnutls.c-to-older-buggier-GnuTLS.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Glenn Morris-3
Paul Eggert wrote:

> I don't see how the crash can occur with vanilla GnuTLS 3.2.21, as its
> gnutls_cipher_list returns a list of IDs that does not contain
> GNUTLS_CIPHER_UNKNOWN. Perhaps you were using a modified GnuTLS
> 3.2.21. Or possibly I'm misreading the GnuTLS source code, though I
> don't see how.

I was using ftp://ftp.gnutls.org/gcrypt/gnutls/v3.2/gnutls-3.2.21.tar.xz

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Paul Eggert
Glenn Morris wrote:
> I was using ftp://ftp.gnutls.org/gcrypt/gnutls/v3.2/gnutls-3.2.21.tar.xz

Weird, because I was looking at the same source code. It does not build under
recent Ubuntu or Fedora, so I did not compile it: I just looked at the source
code. Its implementation of gnutls_cipher_list appears to be straightforward: it
adds only values found as the 'id' component of an entry in the 'algorithms'
array in lib/algorithms/ciphers.c. None of these values are
GNUTLS_CIPHER_UNKNOWN, so gnutls_cipher_list cannot possibly return an array
containing GNUTLS_CIPHER_UNKNOWN.

For example, on my platform, if I compile and run the attached program via:

gcc gs.c -lgnutls
./a.out

the program silently succeeds. What happens on your platform?

gs.c (406 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Glenn Morris-3
Paul Eggert wrote:

> For example, on my platform, if I compile and run the attached program via:
>
> gcc gs.c -lgnutls
> ./a.out
>
> the program silently succeeds. What happens on your platform?

Your example doesn't correspond to how the code in Emacs used to be.
Change it to:

  for (int pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)

and it fails for me, just as Emacs used to:

  gciphers[17] == GNUTLS_CIPHER_UNKNOWN ?!?

As I said, it seems to be a happy accident that GNUTLS_CIPHER_UNKNOWN == 0.
So your example as written happens to skip it.

I hope we have now established I'm not mad or lying... :)

12
Loading...