Closed fedya closed 1 year ago
@jjkeijser, @enaess, @tisj: About OpenSSL 3.0 development branch...
I tested this against openssl-3.0.0-alpha10, and built successfully with only a small modification (although there are deprecation warnings ofcourse, like with openssl-1.1.x):
diff -Nurp ppp-2.4.9.orig/pppd/Makefile ppp-2.4.9/pppd/Makefile
--- ppp-2.4.9.orig/pppd/Makefile 2021-01-15 00:21:46.000000000 +0000
+++ ppp-2.4.9/pppd/Makefile 2021-01-14 23:54:55.000000000 +0000
@@ -8,7 +8,7 @@ CC=$(CROSS_COMPILE)cc
COPTS=-O2 -g -pipe
# Default installation locations
-DESTDIR = $(INSTROOT)/usr/local
+DESTDIR = $(INSTROOT)/tmp
BINDIR = $(DESTDIR)/sbin
MANDIR = $(DESTDIR)/share/man/man8
INCDIR = $(DESTDIR)/include
@@ -93,7 +93,7 @@ INCLUDE_DIRS= -I../include
COMPILE_FLAGS= -DHAVE_PATHS_H -DIPX_CHANGE -DHAVE_MMAP -pipe
-CFLAGS= $(COPTS) $(COMPILE_FLAGS) $(INCLUDE_DIRS) '-DDESTDIR="/usr/local"'
+CFLAGS= $(COPTS) $(COMPILE_FLAGS) $(INCLUDE_DIRS) '-DDESTDIR="/tmp"'
ifdef CHAPMS
CFLAGS += -DCHAPMS=1
@@ -166,6 +166,9 @@ endif
ifdef NEEDCRYPTOLIB
LIBS += -lcrypto
+CFLAGS += -I/tmp/openssl-3.0.0-alpha10/include
+CFLAGS += -I/tmp/openssl-3.0.0-alpha10/include/openssl
+CFLAGS += -L/tmp/openssl-3.0.0-alpha10/
endif
# For "Pluggable Authentication Modules", see ftp.redhat.com:/pub/pam/.
diff -Nurp ppp-2.4.9.orig/pppd/eap-tls.c ppp-2.4.9/pppd/eap-tls.c
--- ppp-2.4.9.orig/pppd/eap-tls.c 2021-01-04 23:06:37.000000000 +0000
+++ ppp-2.4.9/pppd/eap-tls.c 2021-01-14 23:52:58.000000000 +0000
@@ -326,8 +326,7 @@ SSL_CTX *eaptls_init_ssl(int init_server
return NULL;
}
- SSL_library_init();
- SSL_load_error_strings();
+ OPENSSL_init_ssl(OPENSSL_INIT_SSL_DEFAULT, NULL);
/* load the openssl config file only once and load it before triggering
the loading of a global openssl config file via SSL_CTX_new()
I extracted openssl & ppp under /tmp, built openssl first, applied the patch to PPP.
/tmp/ppp-2.4.9# LD_LIBRARY_PATH=/tmp/openssl-3.0.0-alpha10/ ldd pppd/pppd
linux-vdso.so.1 (0x00007fffe079a000)
librt.so.1 => /usr/lib/librt.so.1 (0x00007f3e1491e000)
libssl.so.3 => /tmp/openssl-3.0.0-alpha10/libssl.so.3 (0x00007f3e14881000)
libcrypt.so.1 => /usr/lib/libcrypt.so.1 (0x00007f3e14847000)
libutil.so.1 => /usr/lib/libutil.so.1 (0x00007f3e14842000)
libcrypto.so.3 => /tmp/openssl-3.0.0-alpha10/libcrypto.so.3 (0x00007f3e1440c000)
libdl.so.2 => /usr/lib/libdl.so.2 (0x00007f3e14407000)
libpcap.so.1 => /usr/lib/libpcap.so.1 (0x00007f3e143c3000)
libc.so.6 => /usr/lib/libc.so.6 (0x00007f3e14204000)
libpthread.so.0 => /usr/lib/libpthread.so.0 (0x00007f3e141e3000)
/usr/lib/ld-linux-x86-64.so.2 (0x00007f3e149e0000)
Note that openssl-3 is only yet in alpha fase, and still subject for changes in the API
@fedya, @tisj, @paulusmack: OpenSSL 3.0 Beta 1 is here.
ppp 2.4.9 compiles and links against openssl 3.0beta1 fine, with the exception of pkcs#11 (engine = deprecated) support. There is little that our code can do about this, until someone creates a new pkcs#11 provider for OpenSSL 3.0. So as far as I am concerned OpenSSL 3.0 "just works" with the exception of HSM/tokens.
@enaess: What do you think about OpenSSL 3.0.0 currently in beta 2? Please look the @jjkeijser comment.
@Neustradamus I believe @jjkeijser assessment is correct. You can now specify --with-openssl=/path/to/OpenSSL/3.0.0/install/location --disable-openssl-engine to compile against 3.0.0.
I did compile against 3.0.0 once. Not sure what API they would have for engine / tokens ...
@Neustradamus I did try to compile with it again, and it does issue a bunch of deprecated warnings when compiling against it. Nothing too major, but it looks like to fix this; we need to switch over to
Also, noted that DES is no longer provided by glibc or libcrypt and thus the option to compile without openssl is broken b/c of lack of DES support. The support was removed in 2018 if my recollection is correct.
Seems like some of these files should be moved to a sub-directory pppd/crypto, and included where openssl lack the given algorithm or if openssl isn't used. Not sure how to best do this, but places that depends on e.g. MD5 should probably use MD5() and this ppp/crypto library could provide a override such that pppMD5() would implement it using EVP_md5() or functions internal to the library if needed. That way, we avoid having the code littered with ifdef openssl version > .. blah blah, ..
hmm I just checked out enaess branch of the ppp code and am quite surprised:
So, overall I would NACK the switch to the "use openssl's md4/md5/sha1" routines and I'd propose to actually migrate the DES stuff we need into the ppp source tree, so that we are no longer dependent on the xcrypt or openssl libs with building with EAP-TLS turned off. To make it easier to integrate, I'd propose to rename the internal functions to e.g. ppp_MD4, ppp_MD5 etc.
@jjkeijser good to hear from you again!
I am arriving at the very same conclusion as you.
another option is to rip the DES routines from an older glibc - it should be as short as the md5 stuff, if not shorter and it will remove an external dependency.
The notion of FIPS mode (related to locking down the system) will cause problems for the pppd project. If OpenSSL is in FIPS mode, using any of the illegal algorithms will cause a segfault.
I would argue that’s the whole point of FIPS mode. If you use that to disable insecure ancient crypto algorithms then you should have no expectation that protocols that use them should continue to work. pppd should not try to work around this. It should, however, handle this more gracefully than segfaulting.
Unfortunately, some distro maintainers (e.g. debian) insist on nuking the internal algorithms in favor of using OpenSSL. We'll need to find a way where both can coexist.
There were various reasons for doing this in Debian and I can’t even remember them now (it’ll be in the Debian BTS). OpenSSL was a good choice at the time as we were also applying the EAP-TLS patch (before it was included). I’m open to other options, though, such as libmd or similar.
I agree that the whole point of FIPS mode is to ensure that insecure ancient crypto algorithms are not used under certain circumstances. Keep in mind that even in FIPS mode you can use things like OpenSSL's MD5_* but you have to justify it (and use it appropriately).
But we're talking about (in my view) oldschool and insecure authentication methods to begin with, such as PAP, CHAP, MS-CHAP and I would argue that if you're in FIPS mode all we need to do is
if (is_FIPS_enabled())
{
printf("pppd authentication methods PAP, CHAP, MS-CHAP are disabled due to FIPS compliance\n');
exit (0);
}
or similar - I mean, there's not much for something like pppd in FIPS mode anyways.
Having said that, I feel no obligation to make pppd
any more or less FIPS-compliant than it is at this moment - and if people start reporting issues due to the switch to OpenSSL's MD4, MD5, SHA1 functions then I'd simply say "then compile pppd
without that functionality"
The built in MD4 algorithm is busted and should probably be replaced. The MD4Update() needs to force the data stream in chunks when feeding the algorithm. I didn't realize this and the old code for NTPassword got busted. That's an argument for refreshing the old code, or use OpenSSL instead. I also added a unit-test code in there to validate proper operation of the MSCHAPv1/2 stuff.
What I would dislike is if the code going forward gets littered with a bunch of #ifdefs for using OpenSSL, and if VERSION >= 3.0.0, or something.
If I understand you correctly, FIPS mode can have an insecure crypto algorithm if you use it judiciously? Okay, you can pass the --disable-microsoft-extensions and it would disable the MS-CHAP, MPPE stuff; but it will all pass if you wrap the insecure in a secure environment, i.e. PEAP-MSCHAPv2 w/MPPE or EAP-TLS w/MPPE?
not sure about MD4, but MD5 you can still use in FIPS mode, read up at e.g. https://networkradius.com/articles/2020/10/28/freeradius-fips.html
Basically, one needs to do a call similar to
HMAC_CTX_set_flags(ctx, EVP_MD_CTX_FLAG_NON_FIPS_ALLOW);
and then you can use non-FIPS hash functions for PRF functions only. I am no idea how this would work for CHAP or MS-CHAP authentication (or the horror - plain MD5) but there is a little leeway, it seems.
But I still think that replacing the built in MD4/5 and SHA (and possible DES) code with the OpenSSL onces and then add even more stuff to work around the fact that these algorithms might be present in OpenSSL (disabled at compile time) or not allowed (due to RedHat security policies and/or FIPS) is not worth the hassle. It will also mean that you'd have to maintain two code paths (with or without using the openssl builtins) to ensure that things remaining functional.
So this applies only to the PRF stuff and I have no idea where the old hashing algorithms are used throughout the pppd
code. It used to be present in the EAP-TLS code at some point, but that got dropped when the patch was integrated into the mainline.
Also, keep in mind that FIPS is considered "near-dead" by many in the IT industry, including Microsoft. I know that the OpenSSL team is not planning FIPS certification for OpenSSL 3.0 for quite a while...
I did know Microsoft has for a while discouraged FIPS, but if you are selling software to US government then it is a problem.
However, I think we can burry the idea of FIPS compliance. Don't believe it is in the interest to the pppd project.
The way I coded up the Autotools change was to use OpenSSL if it provided the crypto algorithm, but to allow fallback to a private implementation if one was missing. On my machine, it will use OpenSSL crypto out of the box with no problems.
You are right, fixing the issues with SHA1 conflicting with OpenSSL could be fixed by renaming the SHA1 internal functions so it would not cause an issue when you used the HMAC_init with sha1. That previously caused a crash.
Though, the Autotools change does a lot more than just selectively pick crypto mechanisms. It was to help maintainers that end up patching the build system for a variety number of reasons, e.g. set log or state directory, enable or disable uninteresting features, etc.
So you are suggesting scrapping all use of OpenSSL crypto for use of private implementation?
@tisj MSCHAPv2 still very much depends on the NTPasswordHash which is entirely done by using MD4
@enaess I'm very much aware, that's why I only mentioned DES and not MD4 :)
Ahh ... Sorry, I misspoke. I meant ChallengeResponse() is used by both versions of MSCHAP and applies DES to generate the NT Response.
So you are suggesting scrapping all use of OpenSSL crypto for use of private implementation?
I was suggesting that the following:
We should not (cannot) drop all private implementations of the crypto stuff, and being able to build pppd
with no dependencies on OpenSSL will make embedded people happy. So, if we do both the OpenSSL crypto and the private stuff then we'd be maintaining two code paths which is more work and also dangerous. So the "easy" solution is to scrap using OpenSSL , or least not make it the default but a "use at your own risk" option. If some distro maintainer then wants to replace the private stuff with OpenSSL code, it's on their head ;)
@jjkeijser
1) Yes, I agree. We cannot drop the private implementation of the crypto stuff 2) We should put a drop-in implementation for DES as you cannot compile pppd without OpenSSL (note, that this was still required before my autotools change) 3) The autotools change ensures that the crypto algorithms missing from OpenSSL will use internal implementations for said algorithm 4) If you want the newest features like EAP-TLS, and soon P(EAP), you will need a library that implements SSL which also implies using a crypto library.
While I do concur from a conservative maintainer point of view that the code produced before should equal to that produced after. Also, renaming the internal crypto algorithms will likely avoid the linker issues as in #265. This would be a smaller and less risky fix. Though, using MD4/MD5 if present in OpenSSL is also likely not going to break things. Pppd was already using DES from OpenSSL by default. The only difference being the headache it introduces w.r.t. maintaining the configure options -- I hear what you are saying.
With the number of large patches and development on the pppd project, I actually consider it a project in active development. Now with pending PEAP patches, Autotools, OpenSSL 3.0, fixes for PPPoE, etc. Likely, if we clean up the project a bit, and bump the version to 2.5.0 instead of e.g. 2.4.10. Then add a shim for built in vs. OpenSSL to address the 3.0.0 upgrade. The embedded world is so diverse that it's hard to make any assumption other than, let them configure the project in whichever way they like. Using libcrypto from OpenSSL will just reduce the footprint for pppd and that would be a good thing.
if you are already using and linking to OpenSSL, I see no reason to why we should still use the internally provided algorithms less they are not provided by OpenSSL. Configure will take care of properly set up the compile process for you. That's kind of the purpose of configure.
Note: OpenSSL 3.0.0 has been released
What is the status of all talks on this ticket?
I think that it will be nice to have a best compatibility before a 2.4.10?
What do you think?
Apart from PKCS#11 support, the pppd code is fully compatible with OpenSSL 3.0; adding PKCS#11 support is outside the scope of pppd, as it depends on tools like opensc
, libp11
and the future replacement of engine_pkcs11
. Once those tools support OpenSSL 3.0 we can think about supporting PKCS#11 (i.e. hardware tokens) with OpenSSL 3.0
There is a few deprecated warnings I think. We should probably add a change to add DES encryption as a standalone for when compiling without OpenSSL. Does Fedora use OpenSSL already, then perhaps add another build CI with the latest image?
That aside, I don't think OpenSSL 3.0.0 would stop a 2.4.10 release alone.
@Neustradamus @paulusmack - This issue can be closed. We've fixed the compile warnings, implemented an abstraction for crypto and provided a local implementation of DES. As @jjkeijser indicated, the PKCS#11 support is outside the scope of pppd and OpenSSL 3.0.0 is supposedly working on built-in engine support. The current engine_pkcs11 still works, albeit some compile warnings that API is being deprecated.
can't build 2.4.9 ppp with latest openssl 3.0.0