Skip to content

Commit

Permalink
Make OpenSSL an hcrypto backend proper
Browse files Browse the repository at this point in the history 
This adds a new backend for libhcrypto: the OpenSSL backend.

Now libhcrypto has these backends:

 - hcrypto itself (i.e., the algorithms coded in lib/hcrypto)
 - Common Crypto (OS X)
 - PKCS#11 (specifically for Solaris, but not Solaris-specific)
 - Windows CNG (Windows)
 - OpenSSL (generic)

The ./configure --with-openssl=... option no longer disables the use of
hcrypto.  Instead it enables the use of OpenSSL as a (and the default)
backend in libhcrypto.  The libhcrypto framework is now always used.

OpenSSL should no longer be used directly within Heimdal, except in the
OpenSSL hcrypto backend itself, and files where elliptic curve (EC)
crypto is needed.

Because libhcrypto's EC support is incomplete, we can only use OpenSSL
for EC.  Currently that means separating all EC-using code so that it
does not use hcrypto, thus the libhx509/hxtool and PKINIT EC code has
been moved out of the files it used to be in.
  • Loading branch information
@nicowilliams
nicowilliams committed Apr 15, 2016
1 parent 9df8820 commit 490337f
Show file tree
Hide file tree
Showing 60 changed files with 2,206 additions and 976 deletions.
2 changes: 1 addition & 1 deletion admin/Makefile.am
View file
Expand Up @@ -2,7 +2,7 @@

include $(top_srcdir)/Makefile.am.common

AM_CPPFLAGS += $(INCLUDE_readline) $(INCLUDE_hcrypto)
AM_CPPFLAGS += $(INCLUDE_readline)

man_MANS = ktutil.1

Expand Down
2 changes: 1 addition & 1 deletion appl/ftp/ftp/Makefile.am
View file
Expand Up @@ -4,7 +4,7 @@ include $(top_srcdir)/Makefile.am.common

WFLAGS += $(WFLAGS_LITE)

AM_CPPFLAGS += -I$(srcdir)/../common $(INCLUDE_readline) $(INCLUDE_hcrypto)
AM_CPPFLAGS += -I$(srcdir)/../common $(INCLUDE_readline)

bin_PROGRAMS = ftp

Expand Down
2 changes: 0 additions & 2 deletions appl/otp/Makefile.am
View file
Expand Up @@ -2,8 +2,6 @@

include $(top_srcdir)/Makefile.am.common

AM_CPPFLAGS += $(INCLUDE_hcrypto)

bin_PROGRAMS = otp otpprint
bin_SUIDS = otp
otp_SOURCES = otp.c otp_locl.h
Expand Down
2 changes: 0 additions & 2 deletions appl/su/Makefile.am
View file
Expand Up @@ -2,8 +2,6 @@

include $(top_srcdir)/Makefile.am.common

AM_CPPFLAGS += $(INCLUDE_hcrypto)

bin_PROGRAMS = su
bin_SUIDS = su
su_SOURCES = su.c supaths.h
Expand Down
102 changes: 39 additions & 63 deletions cf/crypto.m4
View file
Expand Up @@ -6,7 +6,7 @@ dnl - own-built libhcrypto

m4_define([test_headers], [
#undef KRB5 /* makes md4.h et al unhappy */
#ifdef HAVE_OPENSSL
#ifdef HAVE_HCRYPTO_W_OPENSSL
#ifdef HAVE_SYS_TYPES_H
#include <sys/types.h>
#endif
Expand Down Expand Up @@ -54,7 +54,7 @@ m4_define([test_body], [
EVP_CIPHER_iv_length(((EVP_CIPHER*)0));
UI_UTIL_read_pw_string(0,0,0,0);
RAND_status();
#ifdef HAVE_OPENSSL
#ifdef HAVE_HCRYPTO_W_OPENSSL
EC_KEY_new();
#endif
Expand All @@ -63,77 +63,53 @@ m4_define([test_body], [
DES_cbc_encrypt(0, 0, 0, schedule, 0, 0);
RC4(0, 0, 0, 0);])

m4_define([bn_headers], [
#include <stdlib.h>
#include <openssl/bn.h>
])
m4_define([bn_body], [
BIGNUM *bn = BN_new();
BN_set_word(bn, 1);
if (BN_is_negative(bn))
exit(EXIT_FAILURE);
BN_set_negative(bn, 1);
if (!BN_is_negative(bn))
exit(EXIT_FAILURE);
exit(EXIT_SUCCESS);
])

AC_DEFUN([KRB_CRYPTO],[
crypto_lib=unknown
AC_WITH_ALL([openssl])
DIR_hcrypto=
AC_MSG_CHECKING([for crypto library])
openssl=no
if test "$crypto_lib" = "unknown" -a "$with_openssl" != "no"; then
save_CFLAGS="$CFLAGS"
save_LIBS="$LIBS"
INCLUDE_hcrypto=
LIB_hcrypto=
if test "$with_openssl" = "yes"; then
with_openssl=/usr
fi
if test "$with_openssl" != "no"; then
INCLUDE_openssl_crypto=
LIB_openssl_crypto=
if test "$with_openssl_include" != ""; then
INCLUDE_hcrypto="-I${with_openssl_include}"
INCLUDE_openssl_crypto="${with_openssl_include}"
else
INCLUDE_openssl_crypto="${with_openssl}/include"
fi
if test "$with_openssl_lib" != ""; then
LIB_hcrypto="-L${with_openssl_lib}"
LIB_openssl_crypto="-L${with_openssl_lib}"
fi
CFLAGS="-DHAVE_OPENSSL ${INCLUDE_hcrypto} ${CFLAGS}"
saved_LIB_hcrypto="$LIB_hcrypto"
for lres in "" "-ldl" "-lnsl -lsocket" "-lnsl -lsocket -ldl"; do
LIB_hcrypto="${saved_LIB_hcrypto} -lcrypto $lres"
LIB_hcrypto_a="$LIB_hcrypto"
LIB_hcrypto_so="$LIB_hcrypto"
LIB_hcrypto_appl="$LIB_hcrypto"
LIBS="${LIBS} ${LIB_hcrypto}"
AC_LINK_IFELSE([AC_LANG_PROGRAM([test_headers],[test_body])], [
crypto_lib=libcrypto openssl=yes
AC_MSG_RESULT([libcrypto])
AC_RUN_IFELSE([AC_LANG_PROGRAM([bn_headers],[bn_body])], [
AC_DEFINE([HAVE_BN_IS_NEGATIVE], 1, [define if OpenSSL provides BN_is_negative])
])
])
if test "$crypto_lib" = libcrypto ; then
break;
fi
done
AC_CHECK_LIB(crypto, OPENSSL_init, [])
CFLAGS="$save_CFLAGS"
LIBS="$save_LIBS"
CFLAGS="-DHAVE_HCRYPTO_W_OPENSSL -I${INCLUDE_openssl_crypto} ${CFLAGS}"
# XXX What about rpath? Yeah...
AC_CHECK_LIB([crypto], [OPENSSL_init],
[LIB_openssl_crypto="${LIB_openssl_crypto} -lcrypto"; openssl=yes], [openssl=no], [])
# These cases are just for static linking on older OSes,
# presumably.
if test "$openssl" = "no"; then
AC_CHECK_LIB([crypto], [OPENSSL_init],
[LIB_openssl_crypto="${LIB_openssl_crypto} -lcrypto -ldl"; openssl=yes], [openssl=no], [-ldl])
fi
if test "$openssl" = "no"; then
AC_CHECK_LIB([crypto], [OPENSSL_init],
[LIB_openssl_crypto="${LIB_openssl_crypto} -lcrypto -ldl -lnsl"; openssl=yes], [openssl=no], [-ldl -lnsl])
fi
if test "$openssl" = "no"; then
AC_CHECK_LIB([crypto], [OPENSSL_init],
[LIB_openssl_crypto="${LIB_openssl_crypto} -lcrypto -ldl -lnsl -lsocket"; openssl=yes], [openssl=no], [-ldl -lnsl -lsocket])
fi
fi
if test "$crypto_lib" = "unknown"; then
DIR_hcrypto='hcrypto'
LIB_hcrypto='$(top_builddir)/lib/hcrypto/libhcrypto.la'
LIB_hcrypto_a='$(top_builddir)/lib/hcrypto/.libs/libhcrypto.a'
LIB_hcrypto_so='$(top_builddir)/lib/hcrypto/.libs/libhcrypto.so'
LIB_hcrypto_appl="-lhcrypto"
LIB_hcrypto='$(top_builddir)/lib/hcrypto/libhcrypto.la'
LIB_hcrypto_a='$(top_builddir)/lib/hcrypto/.libs/libhcrypto.a'
LIB_hcrypto_so='$(top_builddir)/lib/hcrypto/.libs/libhcrypto.so'
LIB_hcrypto_appl="-lhcrypto"
AC_MSG_RESULT([included libhcrypto])
fi
AC_MSG_RESULT([included libhcrypto])
AC_ARG_WITH(pkcs11-module,
AS_HELP_STRING([--with-pkcs11-module=path],
Expand All @@ -147,12 +123,12 @@ if test "$pkcs11_module" != ""; then
fi
if test "$openssl" = "yes"; then
AC_DEFINE([HAVE_OPENSSL], 1, [define to use openssl's libcrypto])
AC_DEFINE([HAVE_HCRYPTO_W_OPENSSL], 1, [define to use openssl's libcrypto as the default backend for libhcrypto])
fi
AM_CONDITIONAL(HAVE_OPENSSL, test "$openssl" = yes)dnl
AM_CONDITIONAL(HAVE_HCRYPTO_W_OPENSSL, test "$openssl" = yes)dnl
AC_SUBST(DIR_hcrypto)
AC_SUBST(INCLUDE_hcrypto)
AC_SUBST(INCLUDE_openssl_crypto)
AC_SUBST(LIB_openssl_crypto)
AC_SUBST(LIB_hcrypto)
AC_SUBST(LIB_hcrypto_a)
AC_SUBST(LIB_hcrypto_so)
Expand Down
6 changes: 3 additions & 3 deletions include/config.h.w32
View file
@@ -1,5 +1,5 @@
/***********************************************************************
* Copyright (c) 2009, Secure Endpoints Inc.
* Copyright (c) 2009-2016, Secure Endpoints Inc.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
Expand Down Expand Up @@ -652,8 +652,8 @@ static const char *const rcsid[] = { (const char *)rcsid, "@(#)" msg }
/* Define to 1 if you have the `openpty' function. */
/* #define HAVE_OPENPTY 1 */

/* define to use openssl's libcrypto */
/* #undef HAVE_OPENSSL */
/* define to 1 to use openssl's libcrypto as a (default) backend for libhcrypto */
/* #undef HAVE_HCRYPTO_W_OPENSSL */

/* Define to enable basic OSF C2 support. */
/* #undef HAVE_OSFC2 */
Expand Down
36 changes: 0 additions & 36 deletions include/crypto-headers.h
View file
Expand Up @@ -5,37 +5,6 @@
#error "need config.h"
#endif

#ifdef HAVE_OPENSSL

#define OPENSSL_DES_LIBDES_COMPATIBILITY

#include <openssl/evp.h>
#include <openssl/des.h>
#include <openssl/rc4.h>
#include <openssl/rc2.h>
#include <openssl/md4.h>
#include <openssl/md5.h>
#include <openssl/sha.h>
#include <openssl/ui.h>
#include <openssl/rand.h>
#include <openssl/engine.h>
#include <openssl/pkcs12.h>
#include <openssl/pem.h>
#include <openssl/hmac.h>
#include <openssl/rsa.h>
#include <openssl/dsa.h>
#include <openssl/ec.h>
#include <openssl/ecdsa.h>
#include <openssl/ecdh.h>
#include <openssl/dh.h>
#include <openssl/bn.h>
#ifndef HAVE_BN_IS_NEGATIVE
#define BN_set_negative(bn, flag) ((bn)->neg=(flag)?1:0)
#define BN_is_negative(bn) ((bn)->neg != 0)
#endif

#else /* !HAVE_OPENSSL */

#ifdef KRB5
#include <krb5-types.h>
#endif
Expand All @@ -52,10 +21,5 @@
#include <hcrypto/engine.h>
#include <hcrypto/pkcs12.h>
#include <hcrypto/hmac.h>
#include <hcrypto/ec.h>
#include <hcrypto/ecdsa.h>
#include <hcrypto/ecdh.h>

#endif /* HAVE_OPENSSL */

#endif /* __crypto_header__ */
3 changes: 2 additions & 1 deletion include/hcrypto/Makefile.am
View file
Expand Up @@ -25,6 +25,7 @@ CLEANFILES = \
rc4.h \
rsa.h \
sha.h \
ui.h
ui.h \
undef.h

EXTRA_DIST = NTMakefile
2 changes: 1 addition & 1 deletion include/heim_threads.h
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2003 Kungliga Tekniska Högskolan
* Copyright (c) 2003-2016 Kungliga Tekniska Högskolan
* (Royal Institute of Technology, Stockholm, Sweden).
* All rights reserved.
*
Expand Down
2 changes: 1 addition & 1 deletion kadmin/Makefile.am
View file
Expand Up @@ -2,7 +2,7 @@

include $(top_srcdir)/Makefile.am.common

AM_CPPFLAGS += $(INCLUDE_libintl) $(INCLUDE_readline) $(INCLUDE_hcrypto) -I$(srcdir)/../lib/krb5 -I$(top_builddir)/include/gssapi
AM_CPPFLAGS += $(INCLUDE_libintl) $(INCLUDE_readline) -I$(srcdir)/../lib/krb5 -I$(top_builddir)/include/gssapi

bin_PROGRAMS = kadmin

Expand Down
2 changes: 1 addition & 1 deletion kcm/Makefile.am
View file
Expand Up @@ -2,7 +2,7 @@

include $(top_srcdir)/Makefile.am.common

AM_CPPFLAGS += $(INCLUDE_libintl) $(INCLUDE_hcrypto) -I$(srcdir)/../lib/krb5
AM_CPPFLAGS += $(INCLUDE_libintl) -I$(srcdir)/../lib/krb5

libexec_PROGRAMS = kcm

Expand Down
4 changes: 3 additions & 1 deletion kdc/Makefile.am
View file
Expand Up @@ -2,7 +2,7 @@

include $(top_srcdir)/Makefile.am.common

AM_CPPFLAGS += $(INCLUDE_libintl) $(INCLUDE_hcrypto) -I$(srcdir)/../lib/krb5
AM_CPPFLAGS += $(INCLUDE_libintl) -I$(srcdir)/../lib/krb5

lib_LTLIBRARIES = libkdc.la

Expand Down Expand Up @@ -44,6 +44,7 @@ libkdc_la_SOURCES = \
kerberos5.c \
krb5tgs.c \
pkinit.c \
pkinit-ec.c \
log.c \
misc.c \
kx509.c \
Expand Down Expand Up @@ -108,6 +109,7 @@ libkdc_la_LIBADD = \
$(LIB_kdb) \
$(top_builddir)/lib/ntlm/libheimntlm.la \
$(LIB_hcrypto) \
$(LIB_openssl_crypto) \
$(top_builddir)/lib/asn1/libasn1.la \
$(LIB_roken) \
$(DB3LIB) $(DB1LIB) $(LMDBLIB) $(NDBMLIB)
Expand Down
15 changes: 9 additions & 6 deletions kdc/NTMakefile
View file
@@ -1,6 +1,6 @@
########################################################################
#
# Copyright (c) 2009, Secure Endpoints Inc.
# Copyright (c) 2009-2016, Secure Endpoints Inc.
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
Expand Down Expand Up @@ -86,7 +86,7 @@ $(BINDIR)\digest-service.exe: $(OBJ)\digest-service.obj $(BIN_LIBS)
$(LIBEXECDIR)\kdc.exe: \
$(OBJ)\connect.obj $(OBJ)\config.obj $(OBJ)\announce.obj \
$(OBJ)\main.obj $(OBJ)\kdc-version.res \
$(LIBKDC) $(BIN_LIBS)
$(LIBKDC) $(BIN_LIBS) $(LIB_openssl_crypto)
$(EXECONLINK)
$(EXEPREP)

Expand All @@ -98,17 +98,19 @@ LIBKDC_OBJS=\
$(OBJ)\kerberos5.obj \
$(OBJ)\krb5tgs.obj \
$(OBJ)\pkinit.obj \
$(OBJ)\pkinit-ec.obj \
$(OBJ)\log.obj \
$(OBJ)\misc.obj \
$(OBJ)\kx509.obj \
$(OBJ)\process.obj \
$(OBJ)\windc.obj

LIBKDC_LIBS=\
$(LIBHDB) \
$(LIBHEIMBASE) \
$(LIBHEIMDAL) \
$(LIBHEIMNTLM) \
$(LIBHDB) \
$(LIBHEIMBASE) \
$(LIBHEIMDAL) \
$(LIBHEIMNTLM) \
$(LIB_openssl_crypto) \
$(LIBROKEN)

LIBKDCRES=$(OBJ)\libkdc-version.res
Expand All @@ -131,6 +133,7 @@ libkdc_la_SOURCES = \
kerberos5.c \
krb5tgs.c \
pkinit.c \
pkinit-ec.c \
log.c \
misc.c \
kx509.c \
Expand Down

8 comments on commit 490337f

@ajorajev
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicowilliams
hi Nico!

I have been looking into this change and trying to guess whether this change or something else is leading to PKI problems.

PKI certificate based TGT request started to fail.

Usecase:

  • call krb5_get_init_creds_opt_set_pkinit before getting initial ticket
    so instead of username/password, certificate is used.

this was working in old Heimdal 1.4. but stopped working in 7.7.

After debugging, i found that issues seems to be around BIGNUM.
The issue seems to occur due to a random number being returned for the private key size in the hx509 library crypto.c implementation of rsa_create_signature().

The rsa key size is returned via:

sig->length = RSA_size(signer->private_key.rsa);
sig->data = malloc(sig->length);

Heimdal has its own implementations of BIGNUM functions in bn.c which are different to OpenSSL. It seems that this library is attempting to cast a BIGNUM

struct bignum_st {
BN_ULONG d; / Pointer to an array of 'BN_BITS2' bit
* chunks. /
int top; / Index of last used d +1. /
/ The next are internal book keeping for bn_expand. /
int dmax; / Size of the d array. /
int neg; / one if the number is negative */
int flags;
};

to a heim_integer

typedef struct heim_integer {
size_t length;
void *data;
int negative;
} heim_integer;

and then using fields of the latter structure which give nonsense results, eg

int
BN_num_bytes(const BIGNUM *bn)
{
return ((const heim_integer *)bn)->length;
}

and hx509_cms_create_signed() where apparently normal credential data leads to a very large output from ASN1_MALLOC_ENCODE, (this macro is identical to 1.4 so it's more likely that the input data structure is flawed somehow).

The message data length is very large > 4 MB compared to 1.4 at 2914 bytes.
And the server was immediately disconnecting due to the large message size.

could you look at this please? any ideas?

also, I wonder, can we locally revert this commit?

regards,
Alibek

@lhoward
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try locally reverting but other things have changed in hcrypto since so, fixing it properly is always better :)

@lhoward
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heimdal and OpenSSL's BIGNUM implementations shouldn't be getting mixed up, all the Heimdal symbols should be renamed by bn.h to have a hc_ prefix. Not saying there's not a bug, but it may be elsewhere...

@ajorajev
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, so basically:

  1. Heimdal mostly uses hcrypto (which then uses openssl as a backend)
  2. in some cases, like PKI, code must still use separate implementation which uses OpenSSL directly (e.g. lib/krb5/pkinit-ec.c)

so it could be that 2nd condition was not always hold and in this case, it was mixed up (and different structure was casted)

@lhoward
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is entirely possible. I'm not super-familiar with the code so @nicowilliams might be the right person to answer, but I could dig in if necessary.

@ajorajev
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. thanks for offering your help. let me first dig some more before I ask you to help. I would like to exclude any configuration issues, etc. and will read code in more detail. may be I can spot or at least narrow down the problem.

@ajorajev
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f.y.i. reproduction of this issue is relatively simple (as I mentioned earlier) - just try to use certs instead of username and password. but one needs to write plug-in, so that Heimdal can call back and pick up certs application wants to use, using krb5_hx509_ks_register

@ajorajev
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep digging and will post more. but if any insights, please let me know. cheers!

Please sign in to comment.