Re: libnettle/libhogweed WIP

[Ted Zlatanov <tzz@lifelogs.com>]

On Thu, 16 Mar 2017 17:28:20 +0200 Eli Zaretskii <eliz@gnu.org> wrote: 

>> From: Ted Zlatanov <tzz@lifelogs.com>
>> Date: Wed, 15 Mar 2017 17:19:32 -0400

>> * data is not wiped after use

EZ> I'm not an expert, but AFAIU this is a serious flaw in
EZ> security-related software.  Should this be optional (as it probably
EZ> has non-negligible run-time costs)?

I don't think the costs are huge, so this is definitely TODO before the
code is ready.

>> * it compiles but doesn't link: "error adding symbols: DSO missing from
>> command line" which I hope is something trivial

EZ> Did the -lhogweed -lnettle switches appear on the link command line?

Here's the full monster (I just did `./autogen.sh all && configure
--with-nettle && make' on a Ubuntu system):

gcc -Demacs  -I. -I. -I../lib -I../lib   -pthread -isystem /usr/include/gtk-3.0 -isystem /usr/include/at-spi2-atk/2.0 -isystem /usr/include/at-spi-2.0 -isystem /usr/include/dbus-1.0 -isystem /usr/lib/x86_64-linux-gnu/dbus-1.0/include -isystem /usr/include/gtk-3.0 -isystem /usr/include/gio-unix-2.0/ -isystem /usr/include/mirclient -isystem /usr/include/mircommon -isystem /usr/include/mircookie -isystem /usr/include/cairo -isystem /usr/include/pango-1.0 -isystem /usr/include/harfbuzz -isystem /usr/include/pango-1.0 -isystem /usr/include/atk-1.0 -isystem /usr/include/cairo -isystem /usr/include/pixman-1 -isystem /usr/include/freetype2 -isystem /usr/include/libpng16 -isystem /usr/include/gdk-pixbuf-2.0 -isystem /usr/include/libpng16 -isystem /usr/include/glib-2.0 -isystem /usr/lib/x86_64-linux-gnu/glib-2.0/include -isystem /usr/include/freetype2  -isystem /usr/include/alsa -pthread -isystem /usr/include/librsvg-2.0 -isystem /usr/include/gdk-pixbuf-2.0 -isystem /usr/include/libpng16 -isystem /usr/include/cairo -isystem /usr/include/glib-2.0 -isystem /usr/lib/x86_64-linux-gnu/glib-2.0/include -isystem /usr/include/pixman-1 -isystem /usr/include/freetype2 -isystem /usr/include/libpng16 -fopenmp -DMAGICKCORE_HDRI_ENABLE=0 -DMAGICKCORE_QUANTUM_DEPTH=16 -fopenmp -DMAGICKCORE_HDRI_ENABLE=0 -DMAGICKCORE_QUANTUM_DEPTH=16 -isystem /usr/include/x86_64-linux-gnu/ImageMagick-6 -isystem /usr/include/ImageMagick-6 -isystem /usr/include/x86_64-linux-gnu/ImageMagick-6 -isystem /usr/include/ImageMagick-6 -isystem /usr/include/libpng16 -isystem /usr/include/libxml2 -isystem /usr/include/dbus-1.0 -isystem /usr/lib/x86_64-linux-gnu/dbus-1.0/include      -pthread -isystem /usr/include/glib-2.0 -isystem /usr/lib/x86_64-linux-gnu/glib-2.0/include -pthread -isystem /usr/include/gconf/2 -isystem /usr/include/dbus-1.0 -isystem /usr/lib/x86_64-linux-gnu/dbus-1.0/include -isystem /usr/include/glib-2.0 -isystem /usr/lib/x86_64-linux-gnu/glib-2.0/include -isystem /usr/include/glib-2.0 -isystem /usr/lib/x86_64-linux-gnu/glib-2.0/include -isystem /usr/include/freetype2 -isystem /usr/include/freetype2 -isystem /usr/include/freetype2  -MMD -MF deps/.d -MP   -lnettle -isystem /usr/include/p11-kit-1    -fno-common -W -Wabi -Waddress -Waggressive-loop-optimizations -Wall -Wattributes -Wbool-compare -Wbuiltin-macro-redefined -Wcast-align -Wchar-subscripts -Wchkp -Wclobbered -Wcomment -Wcomments -Wcoverage-mismatch -Wcpp -Wdate-time -Wdeprecated -Wdeprecated-declarations -Wdesignated-init -Wdisabled-optimization -Wdiscarded-array-qualifiers -Wdiscarded-qualifiers -Wdiv-by-zero -Wdouble-promotion -Wduplicated-cond -Wempty-body -Wendif-labels -Wenum-compare -Wextra -Wformat-contains-nul -Wformat-extra-args -Wformat-security -Wformat-signedness -Wformat-y2k -Wformat-zero-length -Wframe-address -Wfree-nonheap-object -Whsa -Wignored-attributes -Wignored-qualifiers -Wimplicit -Wimplicit-function-declaration -Wimplicit-int -Wincompatible-pointer-types -Winit-self -Wint-conversion -Wint-to-pointer-cast -Winvalid-memory-model -Winvalid-pch -Wjump-misses-init -Wlogical-not-parentheses -Wlogical-op -Wmain -Wmaybe-uninitialized -Wmemset-transposed-args -Wmisleading-indentation -Wmissing-braces -Wmissing-declarations -Wmissing-include-dirs -Wmissing-parameter-type -Wmissing-prototypes -Wmultichar -Wnarrowing -Wnested-externs -Wnonnull -Wnonnull-compare -Wnull-dereference -Wodr -Wold-style-declaration -Wold-style-definition -Wopenmp-simd -Woverflow -Woverride-init -Wpacked -Wpacked-bitfield-compat -Wparentheses -Wpointer-arith -Wpointer-sign -Wpointer-to-int-cast -Wpragmas -Wreturn-local-addr -Wreturn-type -Wscalar-storage-order -Wsequence-point -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value -Wsizeof-array-argument -Wsizeof-pointer-memaccess -Wstrict-aliasing -Wstrict-prototypes -Wsuggest-attribute=format -Wsuggest-attribute=noreturn -Wsuggest-final-methods -Wsuggest-final-types -Wswitch-bool -Wtautological-compare -Wtrampolines -Wtrigraphs -Wuninitialized -Wunknown-pragmas -Wunused -Wunused-but-set-parameter -Wunused-but-set-variable -Wunused-function -Wunused-label -Wunused-local-typedefs -Wunused-macros -Wunused-result -Wunused-value -Wunused-variable -Wvarargs -Wvariadic-macros -Wvector-operation-performance -Wvolatile-register-var -Wwrite-strings -Warray-bounds=2 -Wnormalized=nfc -Wshift-overflow=2 -Wredundant-decls -Wno-missing-field-initializers -Wno-sign-compare -Wno-type-limits -Wno-unused-parameter -Wno-format-nonliteral -g3 -O2  -Wl,-znocombreloc  -no-pie  \
  -o temacs   dispnew.o frame.o scroll.o xdisp.o menu.o xmenu.o window.o charset.o coding.o category.o ccl.o character.o chartab.o bidi.o cm.o term.o terminal.o xfaces.o xterm.o xfns.o xselect.o xrdb.o xsmfns.o xsettings.o gtkutil.o emacsgtkfixed.o dbusbind.o emacs.o keyboard.o macros.o keymap.o sysdep.o buffer.o filelock.o insdel.o marker.o minibuf.o fileio.o dired.o cmds.o casetab.o casefiddle.o indent.o search.o regex.o undo.o alloc.o data.o doc.o editfns.o callint.o eval.o floatfns.o fns.o font.o print.o lread.o  syntax.o unexelf.o bytecode.o process.o gnutls.o nettle.o callproc.o region-cache.o sound.o atimer.o doprnt.o intervals.o textprop.o composite.o xml.o inotify.o  profiler.o decompress.o thread.o systhread.o sheap.o     xfont.o ftfont.o xftfont.o ftxfont.o  fontset.o fringe.o image.o xgselect.o  terminfo.o lastfile.o gmalloc.o     ../lib/libegnu.a       -ltiff -ljpeg -lpng16 -lgif -lXpm -lgtk-3 -lgdk-3 -lpangocairo-1.0 -lpango-1.0 -latk-1.0 -lcairo-gobject -lcairo -lgdk_pixbuf-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lSM -lICE -lX11 -lX11-xcb -lxcb -lXrender -lXft -lasound -lrsvg-2 -lm -lgio-2.0 -lgdk_pixbuf-2.0 -lgobject-2.0 -lglib-2.0 -lcairo -lMagickWand-6.Q16 -lMagickCore-6.Q16 -lacl     -lrt -ldbus-1  -lXrandr -lXinerama -lXfixes -lXext -lxml2 -lgpm   -ltinfo  -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lgconf-2 -lglib-2.0 -lgobject-2.0 -lglib-2.0 -lselinux -lfreetype -lfontconfig -lfreetype -lfreetype -lotf -lfreetype  -lhogweed -lgnutls -lpthread -lanl  -lm -lz 

And the output:

  CCLD     temacs
/usr/bin/ld: nettle.o: undefined reference to symbol 'nettle_hmac_set_key@@NETTLE_6'
/usr/lib/gcc/x86_64-linux-gnu/6/../../../x86_64-linux-gnu/libnettle.so: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
Makefile:612: recipe for target 'temacs' failed

I'm still not sure why it's failing. Is it the order of the libraries?
What does the error mean? I'm sure I had it working before.

EZ>  . the patch is missing additions to the manual and NEWS

Of course, those are TODOs.

EZ>  . support for dynamic loading on MS-Windows should be added

Can we agree that's a followup task? I don't think it should be a
blocker. I have neither the expertise nor a machine running this OS so
it needs a developer.

EZ>  . did you consider exposing this functionality through corresponding
EZ>    GnuTLS functions?

I'm not sure what you mean.

EZ> Specific comments follow:

>> +  # Windows loads libnettle dynamically
>> +  if test "${opsys}" = "mingw32"; then
>> +    LIBNETTLE_LIBS=
>> +  else
>> +    CFLAGS="$CFLAGS $LIBNETTLE_CFLAGS"
>> +    LIBS="$LIBNETTLE_LIBS $LIBS"
>> +  fi

EZ> CFLAGS should be set for the Windows build as well.  Only LIBS should
EZ> not be added.

OK.

>> diff --git a/lisp/nettle.el b/lisp/nettle.el
>> new file mode 100644
>> index 0000000000..6f49c3f031
>> --- /dev/null
>> +++ b/lisp/nettle.el
>> @@ -0,0 +1,335 @@
>> +;;; nettle.el --- Interface to libnettle/libhogweed  -*- lexical-binding: t; -*-

EZ> Actually, this does not seem to be an interface at all.  What you have
EZ> here are defcustom that seems not to be used anywhere, a bunch of
EZ> diagnostic functions useful for debugging, and tests that should be
EZ> moved elsewhere anyway.  Do we really need this file?

This will be the test code, I think. The defcustoms and functions will
mostly be useful for testing and debugging. I think when I did the
original patch the tests directory was not as evolved :)

Once the code compiles, I'll move this to the tests.

>> +;; Copyright (C) 2013  Teodor Zlatanov

EZ> This should cite the FSF as the copyright holder, I think.

Fixed.

>> +#ifdef HAVE_NETTLE
>> +      syms_of_nettle ();
>> +#endif

EZ> syms_of_nettle should be called only if !initialized.

Fixed.

>> +DEFUN ("nettle-available-p", Fnettle_available_p, Snettle_available_p, 0, 0, 0,

EZ> This function should be outside of the "#ifdef HAVE_NETTLE"
EZ> conditional, otherwise it will not be available in an Emacs built
EZ> without the library, and Lisp programs will need to use fboundp
EZ> instead, which all but defeats the purpose of the function.

Yup, fixed.

>> +DEFUN ("nettle-hash", Fnettle_hash, Snettle_hash, 2, 2, 0,
>> +       doc: /* Hash INPUT string with HASH-METHOD into a unibyte string.

EZ> Here and elsewhere, the doc string should explicitly tell that INPUT
EZ> must be a unibyte string.

Yeah, it's a pain.

EZ> A design question: would it make sense to support vectors as INPUT,
EZ> here and in the rest of the functions?

I think so--I was heading that way. Since I see this as a low-level
stateless interface, it makes sense to require consumers to use bindat,
and it would remove the ambiguity. Any objections?

EZ> Another design question: should be support buffer regions, instead of
EZ> strings, as input to these functions?  The way your code is written,
EZ> Lisp programs must always cons a string from buffer text, before they
EZ> invoke these functions.  This could be gratuitous cost in some use
EZ> cases.

Yes, but I think a low-level interface shouldn't know about buffers.
This has security implications as well--it's a little bit harder to see
the data that was passed to the function, but I feel that's not enough
to offset the complexity of translating from buffer data to C.

>> +  (Lisp_Object input, Lisp_Object hash_method)
>> +{
>> +  Lisp_Object ret = Qnil;
>> +
>> +  CHECK_STRING (input);
>> +  CHECK_STRING (hash_method);
>> +
>> +  for (int i = 0; NULL != nettle_hashes[i]; i++)
>> +    {
>> +      if (NETTLE_STRING_EQUAL_UNIBYTE (hash_method, nettle_hashes[i]->name))

EZ> I don't think it's a good idea to use strings for methods and other
EZ> such fixed parameters in your patch.  We usually use symbols for that.
EZ> The advantage of symbols is that you can test equality with EQ,
EZ> instead of the costly NETTLE_STRING_EQUAL_UNIBYTE, or any equivalent
EZ> code which compares strings.

Right. But I can't pre-define the symbols because I don't know what
hashes and ciphers will be dynamically available. So is it OK to check
if the symbol name equals a C string? Is there a fast safe way to do
that (considering that symbol names have a pretty rich charset)?

EZ> Here and elsewhere, the size of the result is known in advance, so I
EZ> would avoid allocating a scratch buffer and then copying its data
EZ> (inside make_unibyte_string) into a newly-allocated string.  Instead,
EZ> use make_uninit_string, and then pass its string data pointer to the
EZ> algorithm that produces the digest.

Got it, will be a TODO once compilation works.

>> +          uint8_t *digest;
EZ>              ^^^^^^^
EZ> Why not 'unsigned char'?

I was matching nettle-types.h which uses uint8_t. I've switched to
unsigned char.

EZ> This doc string doesn't document the ITERATIONS and SALT arguments.
(x several times)

Definitely TODO all these docs.

>> +  mode = CAR_SAFE (Fmember (cipher_mode, Fnettle_cipher_modes ()));

EZ> Wouldn't it be less expensive to access the data on the C level,
EZ> without consing a list?
...
EZ> I think there are more efficient ways of doing this, which don't need
EZ> an explicit loop.
...
EZ> I don't understand why you copy the data here, instead of passing the
EZ> algorithm a pointer to the original string's data.
...
EZ> Likewise here.  In addition, since you are using 'min', shouldn't we
EZ> signal an error if KEY is too long?

I'll put these on the TODO list.

EZ> Once again, since the length is known in advance, producing output
EZ> into an allocated buffer and then creating a Lisp string by copying
EZ> that buffer is wasteful.  It is better to produce output directly into
EZ> a string's data.

I don't know enough about the Lisp string internals to do this safely.

I've pushed your latest suggestions (the "fixed" ones above) to the branch.

I'll proceed with the TODOs (for now, simply dropped in nettle.c) as
time allows but I need the compilation to work.

Ted