Steffan Karger [Wed, 1 Nov 2017 22:03:42 +0000 (23:03 +0100)]
create_temp_file/gen_path: prevent memory leak if gc == NULL
If gc == NULL, the data allocated in the alloc_gc_buf() call in
create_temp_file or the string_mod_const call in gen_path would never
be free'd.
These functions are currently never called that way, but let's prevent
future problems.
While touching create_temp_file, also remove the counter variable, which is
never read, simplify the do-while to a while loop, and truncate the prefix
(if needed) to preserve the random and extension of the created filename.
Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20171101220342.14648-5-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15703.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Steffan Karger [Wed, 1 Nov 2017 22:03:41 +0000 (23:03 +0100)]
Don't throw fatal errors from create_temp_file()
This function is called in response to connecting clients, and can fail
when I/O fails for some (possibly temporary) reason. In such cases we
should not exit the process, but just reject the connecting client.
This commit changes the function to actually return NULL on errors, and
(where needed) changes the callers to check for and handle errors.
Since the tls-crypt-v2 metadata code also calls create_temp_file() when
clients connect, I consider this a prerequisite for tls-crypt-v2.
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20171101220342.14648-4-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15701.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Steffan Karger [Wed, 1 Nov 2017 22:03:40 +0000 (23:03 +0100)]
pf: reject client if PF plugin is configured, but init fails
This changes the behavior for pf plugins: instead of just not initializing
the firewall rules and happily continuing, this now rejects the client in
the case of an (unlikely) failure to initialize the pf.
Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20171101220342.14648-3-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15704.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Steffan Karger [Wed, 1 Nov 2017 22:03:39 +0000 (23:03 +0100)]
pf: clean up temporary files if plugin init fails
close_instance() tries to remove the file in c2.pf.filename, but that only
works if we actually set that if we fail. So, set that filename as soon
as we know we've created the file.
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20171101220342.14648-2-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15705.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Steffan Karger [Sun, 12 Nov 2017 16:36:36 +0000 (17:36 +0100)]
Add --tls-cert-profile option.
This allows the user to specify what certificate crypto algorithms to
support. The supported profiles are 'preferred', 'legacy' (default) and
'suiteb', as discussed in <84590a17-1c48-9df2-c48e-4160750b2e33@fox-it.com>
(https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14214.
html).
This fully implements the feature for mbed TLS builds, because for mbed it
is both more easy to implement and the most relevant because mbed TLS 2+
is by default somewhat restrictive by requiring 2048-bit+ for RSA keys.
For OpenSSL, this implements an approximation based on security levels, as
discussed at the hackathon in Karlsruhe.
This patch uses 'legacy' as the default profile following discussion on
the openvpn-devel mailing list. This way this patch can be applied to
both the release/2.4 and master branches. I'll send a follow-up patch for
the master branch to change the default to 'preferred' later.
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20171112163636.17434-1-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15848.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Simon Matter [Thu, 16 Nov 2017 14:09:58 +0000 (15:09 +0100)]
Add per session pseudo-random jitter to --reneg-sec intervals
While we were suffering from the "TLS Renegotiation Slowdown" bug here
https://community.openvpn.net/openvpn/ticket/854 we realized that there is
still room for improvement in our use case.
It appears that TLS renegotiation is getting more and more expensive in
terms of CPU cycles with recent changes for more security. To make things
worse, we realized that most renegotiation procedures took place at almost
the same time and increased the CPU load too much during these periods.
That's especially true on large, multi-instance openvpn setups.
I've created attached patch to add a per session pseudo-random component to
the --reneg-sec intervals so that renegotiation is evenly spread over time.
It is configured by simply adding a second value to --reneg-sec as
described in the --help text:
--reneg-sec max [min] : Renegotiate data chan. key after at most max
(default=3600) and at least min (default 90% of max on
servers and equal to max on clients).
The jitter is only enabled by default on servers, because the actual reneg
time is min(reneg_server, reneg_client). Introducing jitter at both ends
would bias the actual reneg time to the min value.
Note that the patch also slightly changes the log output to show the sec
value in the same way as the bytes/pkts values:
The idea and first versions of this patch are from Simon Matter. Steffan
Karger later incorporated the mailing list comments into this patch. So
credits go to Simon, and all bugs are Steffan's fault ;-)
Signed-off-by: Simon Matter <simon.matter@invoca.ch> Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20171116140958.12847-1-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15888.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Gert Doering [Sat, 11 Nov 2017 14:22:30 +0000 (15:22 +0100)]
Remove warning on pushed tun-ipv6 option.
tun-ipv6 is a no-op nowadays, and we print a warning to let users know -
which is not helpful for server-pushed tun-ipv6 (which might be the
result of --server-ipv6 automatically pushing this). So, remove the
warning if parsing pushed options.
Also, remove the VERIFY_PERMISSION() call here which has side effects
on the "which class of options got pushed, do we need to act on them
later on?" flag set.
v2: use existing pull_mode flag
Signed-off-by: Gert Doering <gert@greenie.muc.de> Acked-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20171111142230.3288-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/search?l=mid&q=20171111142230.3288-1-gert@greenie.muc.de Signed-off-by: Gert Doering <gert@greenie.muc.de>
Steffan Karger [Wed, 8 Nov 2017 12:12:54 +0000 (13:12 +0100)]
doxygen: add make target and use relative paths
Add a make target, such that 'make doxygen' works (both for in-tree and
out-of-tree builds). This now generates the doxygen in doc/doxygen/,
rather than in doxygen/.
While doing so, instead of genering docs with full path names (e.g.
/home/steffan/dev/openvpn/src/openvpn/crypto.h), use a relative path wrt
the project root (e.g. src/openvpn/crypto.h) in the generated
documentation. This makes the generated doxygen easier to read.
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1510143174-15248-1-git-send-email-steffan.karger@fox-it.com>
URL: https://www.mail-archive.com/search?l=mid&q=1510143174-15248-1-git-send-email-steffan.karger@fox-it.com Signed-off-by: Gert Doering <gert@greenie.muc.de>
Simon Rozman [Fri, 13 Oct 2017 09:50:08 +0000 (11:50 +0200)]
Uniform swprintf() across MinGW and MSVC compilers
Legacy _snwprintf() and snwprintf() functions replaced with ISO C
swprintf().
Assigning _snwprintf() return value to unused variable was also removed
at one occasion. Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20171013095008.8288-1-simon@rozman.si>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15633.html
Simon Rozman [Wed, 11 Oct 2017 13:45:30 +0000 (15:45 +0200)]
Document ">PASSWORD:Auth-Token" real-time message
Authentication tokens are security enhancement eliminating client
need to cache passwords, and are indispensable at two factor
authentication methods, such as HOTP or TOTP.
The ">PASSWORD:Auth-Token" message was not mentioned anywhere in
the OpenVPN Management Interface Notes. This patch adds a simple use
case example, while the more detailed feature description remains
explained in the OpenVPN manual. Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20171011134530.6676-1-simon@rozman.si>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15599.html
Simon Rozman [Thu, 12 Oct 2017 10:34:48 +0000 (12:34 +0200)]
Fix local #include to use quoted form
.h include files from the same folder or addressed relatively to the
same folder should be #included using quoted form in MSVC. The angled
form is reserved for include files from folders specified using /I
path.
Using angled form, MSVC fails to locate local #include file, unless
current folder is added to the include search path: /I . Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20171012103448.7632-1-simon@rozman.si>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15622.html
As per previous commit, this is a simple solution to cope with the
various sizes of time_t on different archs, including those that use 64
bits time_t on ILP32 archs to cope with y2038.
Also:
- convert the time_type/time_format abstraction that used unsigned long
to inlined long long code
- print suseconds_t as a long, which appears to be the underlying type
on most Unix systems around
Signed-off-by: Jeremie Courreges-Anglas <jca@wxcvbn.org> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <87k1zi18lt.fsf@ritchie.wxcvbn.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15667.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
time_t is only specified as an integer type per POSIX. To reliably
print it, better cast it to "long long", which is at least 64 bits wide
and can represent values beyond 2038.
Printing as a "long" could cause problems on ILP32 systems using a 64
bits time_t (eg OpenBSD/armv7).
James Bottomley [Sun, 29 Oct 2017 15:34:48 +0000 (15:34 +0000)]
autoconf: Fix engine checks for openssl 1.1
In openssl 1.1, ENGINE_cleanup became a #define instead of a function
(because it's no longer needed as engines are self cleaning). Update
the autoconf.ac script to check for ENGINE_cleanup as a declaration to
avoid falsely undefinig HAVE_OPENSSL_ENGINE in openssl 1.1+
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <1509291288.3116.14.camel@HansenPartnership.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15676.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Selva Nair [Fri, 20 Oct 2017 17:25:56 +0000 (13:25 -0400)]
Avoid illegal memory access when malformed data is read from the pipe
- If only 1 byte is read from the interactive service client pipe, that
evaluates to zero wide characters and subsequent check for NUL
termination in the data buffer segfaults.
Fix: reject clients that send less than a complete wide character.
Signed-off-by: Selva Nair <selva.nair@gmail.com> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1508520356-18277-1-git-send-email-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15657.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Simon Rozman [Thu, 12 Oct 2017 08:07:20 +0000 (10:07 +0200)]
Simplify iphlpapi.dll API calls
Dynamically locating API function addresses at run-time using
GetProcAddress() was a leftover from the early days of the interactive
service development. It was required before `NTDDI_VERSION` was raised
from Windows XP to Windows Vista.
After NTDDI_VERSION API level was raised to NTDDI_VISTA, the direct
calling of Vista introduced API functions is possible and much
simpler.
This patch simplifies the code while in the same time it removes
controversial function type definitions that caused interactive service
not to compile on MSVC. Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20171012080720.7764-1-simon@rozman.si>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15614.html
lz4: Fix broken builds when pkg-config is not present but system library is
In commit f91e4863bc1382 we fixed an issue where LZ4_LIBS could be
overwritten in some situations. But on systems where lz4 is installed on
the system but is lacking pkg-config information, the linker will not know
about the lz4 library when completing the build.
This fixes the issue by explicitly setting LZ4_LIBS to contain -llz4
if pkg-config test was run and failed verifying the installed lz4 version
number. This also ensures that LZ4_LIBS will not be overwritten if it
has been provided on the ./configure command line.
Signed-off-by: David Sommerseth <davids@openvpn.net> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20171002190732.12531-1-davids@openvpn.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15549.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Older LZ4 library versions used a version number > 100 and not the
current x.y.z versioning scheme. This results in version 122 being
numberically higher than the check we have liblz4 > 1.7.1. And
since that old version (122) does not have the LZ4_compress_default(),
the building explodes later on.
This patch enhances the version check to also ensure the version
number is lower than 100. In addition the function checking we
had was not triggered if system library was found via pkg-config,
so this have now been reworked to really check if we have at least
two of the most important LZ4 functions - as long as a system
library have been found or been accepted via the LZ4_{CFLAGS,LIBS}
variables.
There are more ways to check for functions in autoconf. I opted
for AC_CHECK_LIB() instead of AC_CHECK_FUNC{,S}() as the latter
ones does not test if a function exists in a specific library. This
have the downside of needing to tests instead of AC_CHECK_FUNCS()
which could test for more functions in one go. We also do not
overwrite the LZ4_LIBS variable on success, as that could change
already set library paths (-L)
Finally, a stupid typo got fixed as well.
Trac: 939 Signed-off-by: David Sommerseth <davids@openvpn.net> Tested-by: Richard Bonhomme <fragmentux@gmail.com> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20171002161812.9376-1-davids@openvpn.net>
URL: https://www.mail-archive.com/search?l=mid&q=20171002161812.9376-1-davids@openvpn.net Signed-off-by: Gert Doering <gert@greenie.muc.de>
Commit 3d6a4cded2b20fb81 introduced checking for "too many parameters"
at option processing, and neglected to take "ipv6only" as possible
(and optional) argument to "--bind" into account.
Trac: #938
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20170928031620.22331-1-hashiz@meridiani.jp>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15522.html
We are using a deprecated function, LZ4_compress_limitedOutput(), which
will be removed with time. The correct function to use is
LZ4_compress_default(). Both function takes the same number of
arguments and data types, so the change is minimal.
This patch will also enforce the system LZ4 library to be at least v1.7.1.
If the system library is not found or it is older, it will be build using
the bundled LZ4 library. The version number requirement is based on the
LZ4 version we ship.
The changes in configure.ac for the version check is modelled around the
same approach we use for OpenSSL. Plus it does a few minor reformats and
improvements to comply with more recommend autoconf coding style.
This patch is a result of the discussions in this mail thread:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14135.html
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20170907172004.22534-1-davids@openvpn.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15396.html Signed-off-by: David Sommerseth <davids@openvpn.net>
Steffan Karger [Tue, 15 Aug 2017 08:04:33 +0000 (10:04 +0200)]
Fix bounds check in read_key()
The bounds check in read_key() was performed after using the value, instead
of before. If 'key-method 1' is used, this allowed an attacker to send a
malformed packet to trigger a stack buffer overflow.
Fix this by moving the input validation to before the writes.
Note that 'key-method 1' has been replaced by 'key method 2' as the default
in OpenVPN 2.0 (released on 2005-04-17), and explicitly deprecated in 2.4
and marked for removal in 2.5. This should limit the amount of users
impacted by this issue.
CVE: 2017-12166 Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Gert Doering <gert@greenie.muc.de> Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <80690690-67ac-3320-1891-9fecedc6a1fa@fox-it.com>
URL: https://www.mail-archive.com/search?l=mid&q=80690690-67ac-3320-1891-9fecedc6a1fa@fox-it.com Signed-off-by: David Sommerseth <davids@openvpn.net>
systemd: Enable systemd's auto-restart feature for server profiles
Systemd supervises services it has started and can act upon unexpected
scenarios. This change will restart OpenVPN after 5 seconds if the OpenVPN
process exits unexpectedly.
The on-failure mode is the recommended mode by upstream systemd.
This change have been tested on a test server for some month, and it
works indeed as intended when provoking the OpenVPN process to stop.
Signed-off-by: David Sommerseth <davids@openvpn.net> Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20170906235202.26551-1-davids@openvpn.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15370.html Signed-off-by: David Sommerseth <davids@openvpn.net>
tcp-server: ensure AF family is propagated to child context
Commit 23d61c56 introduced the AF_UNSPEC socket family
to be used when we don't know the actual one until the local
socket binding is performed.
In such case AF_UNSPEC is stored in the `ce.af` member of
the `c->options` object, indicating that the family has to be
determined at runtime.
However, the determined value is never propagated back to the
`options` object, which remains AF_UNSPEC and that is
later used to initialize the TCP children contexts (UDP
children contexts are unaffected).
This unexpected setting can trigger weird behaviours, like
the one reported in ticket #933.
In this case the value AF_UNSPEC in combination with the
changes implemented in 2bed089d are leading to a TCP
server quitting with M_FATAL upon client connection.
Note that the misbehaviour described in #933 can only be
triggered when running a TCP server with mtu-disc set
in the config (no matter the value).
Fix this inconsistency by always propagating the AF
family from the top to the child context when running
in TCP server mode.
As a direct consequence, this patch fixes Trac #933.
Trac: 933 Signed-off-by: Antonio Quartulli <antonio@openvpn.net> Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20170907095530.15972-1-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15380.html Signed-off-by: David Sommerseth <davids@openvpn.net>
systemd: Ensure systemd shuts down OpenVPN in a proper way
By default, when systemd is stopping OpenVPN it will send the SIGTERM
to all processes within the same process control-group. This can come
as a surprise to plug-ins which may have fork()ed out child processes.
So we tell systemd to only send the SIGTERM signal to the main OpenVPN
process and let OpenVPN take care of the shutdown process on its own.
If the main OpenVPN process does not stop within 90 seconds (unless
changed), it will send SIGKILL to all remaining processes within
the same process control-group.
This issue have been reported in both Debian and Fedora.
OpenSSL: Always set SSL_OP_CIPHER_SERVER_PREFERENCE flag
* safe bet to say that server admins are better at updating their configs
than client users are and if client do want to restrict their ciphers,
they should simply evict the ciphers they don't want from their cipher
suite
* mbed TLS and OpenSSL behave more similar with the
SSL_OP_CIPHER_SERVER_PREFERENCE flag
Signed-off-by: Szilárd Pfeiffer <coroner@pfeifferszilard.hu> Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <20170904081012.1975-1-coroner@pfeifferszilard.hu>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15356.html Signed-off-by: David Sommerseth <davids@openvpn.net>
Gert van Dijk [Sun, 27 Aug 2017 16:15:15 +0000 (18:15 +0200)]
Warn that DH config option is only meaningful in a tls-server context
If specified in a tls-client context, don't try to open the file as it's
not used. Worse even, if 'none' was specified to disable explicitly, it
complained that the file 'none' could not be found.
[DS: On-the-fly update - Prefixed the message with 'WARNING: ']
Signed-off-by: Gert van Dijk <gert@gertvandijk.net> Acked-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20170827161515.2424-1-gert@gertvandijk.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15332.html Signed-off-by: David Sommerseth <davids@openvpn.net>
!A || (A && B) is equivalent to the simpler !A || B
therefore it is preferable to use the second version as
it is simpler to parse while reading the code.
Signed-off-by: Antonio Quartulli <a@unstable.cc> Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20170824075547.29844-1-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15313.html Signed-off-by: David Sommerseth <davids@openvpn.net>
David Sommerseth [Tue, 22 Aug 2017 11:47:15 +0000 (13:47 +0200)]
docs: Replace all PolarSSL references to mbed TLS
There were references in our documentation to the now deprecated PolarSSL
library, which have changed name upstream to mbed TLS.
In addition, where appropriate, the documentation now considers only
mbed TLS 2.0 and newer. This is in accordance with the requirements
./configure sets.
[DS: On-the-fly change - Updated Makefile.am to use README.mbedtls
instead of README.polarssl. This ensures make dist and buildbots
won't explode]
Signed-off-by: David Sommerseth <davids@openvpn.net> Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <20170822114715.14225-1-davids@openvpn.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15309.html Signed-off-by: David Sommerseth <davids@openvpn.net>
Steffan Karger [Sun, 20 Aug 2017 09:19:04 +0000 (11:19 +0200)]
travis: reorder matrix to speed up build
The OSX and mingw builds are much slower than the other jobs. Our free
travis account can only use 4 build executors in parallel. Run the slow
builds earlier, so that when one or more of these finish, the free build
executors will start building the configure variants in parallel with the
slow ones. (Instead of doing the slow ones last, which results in using
only 1-2 executors during the end stage.)
Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: Antonio Quartulli <a@unstable.cc>
Message-Id: <1503220744-5569-1-git-send-email-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15302.html Signed-off-by: David Sommerseth <davids@openvpn.net>
Steffan Karger [Wed, 16 Aug 2017 17:04:50 +0000 (19:04 +0200)]
tls-crypt: don't leak memory for incorrect tls-crypt messages
If tls_crypt_unwrap() failed, we would jump to cleanup and forget to free
the buffer. Instead, allocate the buffer through gc, which is free'd in
the cleanup section.
Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20170816170450.10415-1-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15282.html Signed-off-by: David Sommerseth <davids@openvpn.net>
Steffan Karger [Tue, 8 Aug 2017 15:55:41 +0000 (17:55 +0200)]
Add coverity static analysis to Travis CI config
Enable coverity analysis for the release/2.4 branch.
We can only do a limited number of coverity scans per week with our FOSS
account, but since we only occasionally push commits, that should work out
fine. But this limit is the reason we don't use the standard travis addon,
because that would cause the coverity script to run on all of our matrix
builds. That would cause us to reach our limit faster, and waste travis'
resources.
Since our FOSS coverity account doesn't handle multiple branches very well,
we have to pick one branch to run coverity on. I think it's best to use
the most recent stable branch for that (i.e. for now, release/2.4).
Though for ease of maintenance, it's probably best to apply the patch to
both master and release/2.4.
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <a@unstable.cc>
Message-Id: <1502207741-31750-1-git-send-email-steffan.karger@fox-it.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15176.html Signed-off-by: David Sommerseth <davids@openvpn.net>
crypto: create function to initialize encrypt and decrypt key
Instead of always initialize the encrypt and decrypt keys separately,
implement an helper function init_key_ctx_bi() that takes care of
both of them for us.
Reduces code duplication and improves readability.
Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: Antonio Quartulli <a@unstable.cc>
Message-Id: <20170707044704.7239-1-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15011.html Signed-off-by: David Sommerseth <davids@openvpn.net>
David Sommerseth [Tue, 15 Aug 2017 20:53:01 +0000 (22:53 +0200)]
Use consistent version references
A simple clean-up where the version references have been unified
all those places I could find now. The versioning scheme used is:
* OpenVPN 2.x
* v2.x
We want to avoid:
* 2.x (2.4 can be just an ordindary decimal number,
OID reference, a version number or anything else)
* OpenVPN v2.x (OpenVPN indicates we're talking about a version)
In addition, several places where it made sense I tried to ensure
the first version reference uses "OpenVPN 2.x" and the following
references in the same section/paragraph uses "v2.x", to set the
context for the version reference.
In Changes.rst modified paragraphs exceeding 80 chars lines where
reformatted as well.
Signed-off-by: David Sommerseth <davids@openvpn.net> Acked-by: Steffan Karger <steffan@karger.me>
Message-Id: <20170815205301.14542-1-davids@openvpn.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15260.html Signed-off-by: David Sommerseth <davids@openvpn.net>
David Sommerseth [Tue, 15 Aug 2017 21:54:51 +0000 (23:54 +0200)]
Highlight deprecated features
We have quite a list of deprecated options currently. Ensure this
is highlighted both in documentation and code.
This patch builds on the wiki page [1] enlisting all deprecated features
and their status. There are also some options not listed here, as
there exists patches in release/2.4 which awaits an update for git master.
Steffan Karger [Tue, 15 Aug 2017 15:39:46 +0000 (17:39 +0200)]
Move run_up_down() to init.c
This function is only used in init.c, and is not easy to fit into a
specific category because it both runs scripts and plugin hooks. Making
it static in init.c is probably the best place for this function.
(I think we should find a better place for everything currently in misc.c,
and get rid of it all together. This patch is part of that effort.)
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <a@unstable.cc>
Message-Id: <1502811586-19578-1-git-send-email-steffan.karger@fox-it.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15256.html Signed-off-by: David Sommerseth <davids@openvpn.net>
When setting the SOCKS proxy through the management interface, the
socks_proxy_port pointer would be set to a value that's no longer valid
by the time it's used by do_preresolve_host.
Signed-off-by: Thomas Veerman <thomas.veerman@wanwire.net> Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20170707195941.61773-1-thomas.veerman@wanwire.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15018.html Signed-off-by: David Sommerseth <davids@openvpn.net>
The --keysize option can only be used with already deprecated ciphers,
such as CAST5, RC2 or BF. Deviating from the default keysize is
generally not a good idea (see man page text), and otherwise only
complicates our code.
Since we will also remove the support for weak ciphers (ciphers with
cipher block length less than 128 bits) in OpenVPN 2.6 as well, we
start the deprecation of this option instantly.
[DS: Slightly amended the patch, referencing OpenVPN 2.6 and added
a few more details to Changes.rst and the commit message]
Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20170701112951.19119-1-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15004.html Signed-off-by: David Sommerseth <davids@openvpn.net>
Move create_temp_file() out of #ifdef ENABLE_CRYPTO
By using get_random() instead of prng_bytes(), we no longer have to place
create_temp_file() inside #ifdef ENABLE_CRYPTO.
The resulting filename now has 62 bits of entropy (2 * [0-INT_MAX])
instead of the previous 128 bits, but that should be plenty. Assuming an
int is 32 bits, we would need about 2**31 (2147483648) files to have a
(roughly) 0.5 chance of failing in one of the 6 attempts we do.
(This is preparing to move the function out of misc.c, where I'd prefer to
not have to add a #include "crypto.h".)
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20170725210234.5673-1-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15146.html Signed-off-by: David Sommerseth <davids@openvpn.net>
If a peer has set --keysize, and NCP negotiates a cipher with a different
key size (e.g. --keysize 128 + AES-256-GCM), that peer will exit with a
"invalid key size" error. To prevent that, always set keysize=0 for NCP'd
ciphers.
Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <1500573357-20496-1-git-send-email-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15110.html Signed-off-by: David Sommerseth <davids@openvpn.net>
Functions used only in the file where they are
defined and not exported in any header, should
always defined as static in order to make the scope
clear to the compiler and the developers.
Add the static attribute where missing.
Signed-off-by: Antonio Quartulli <antonio@openvpn.net> Acked-by: Steffan Karger <steffan@karger.me>
Message-Id: <20170811090744.31750-4-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15202.html Signed-off-by: David Sommerseth <davids@openvpn.net>
In the attempt of adhering to the C99 standard as much as possible,
ensure that all the function declarations with no parameter contain
the "void" keyword[1].
OpenSSL: remove unreachable call to SSL_CTX_get0_privatekey()
In tls_ctx_load_ecdh_params() the SSL_CTX_get0_privatekey() function
is invoked only when "OPENSSL_VERSION_NUMBER >= 0x10002000L" and
curve_name is NULL.
However, under the very same conditions the code flow will
lead to an earlier return, thus never reaching the invocation of
SSL_CTX_get0_privatekey().
Restructure the surrounding code in order to make the if/else
block a bit easier to read and get rid of the unreachable
invocation.
Signed-off-by: Antonio Quartulli <antonio@openvpn.net> Acked-by: Steffan Karger <steffan@karger.me>
Message-Id: <20170809074237.31291-1-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15186.html Signed-off-by: David Sommerseth <davids@openvpn.net>
David Sommerseth [Tue, 25 Jul 2017 15:07:23 +0000 (17:07 +0200)]
cleanup: Move init_random_seed() to where it is being used
The init_random_seed() function is only used by the init_static() in
init.c. As this function was pretty basic and it is only being called
once, it was merged into init_static() instead of keeping it as a separate
function.
(I agree that calling functions often makes the code more readable, but
I would rather see that as a part of cleaning up the whole init_static()
function - in fact when moving all "unit tests" in init_static() to cmocka,
it will not be too bad in the end.)
Signed-off-by: David Sommerseth <davids@openvpn.net> Acked-by: Steffan Karger <steffan@karger.me>
Message-Id: <20170725150723.14919-1-davids@openvpn.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15136.html Signed-off-by: David Sommerseth <davids@openvpn.net>
David Sommerseth [Tue, 25 Jul 2017 13:03:14 +0000 (15:03 +0200)]
contrib: Remove keychain-mcd code
After the security audits performed by Cryptography Engineering the
spring of 2017 [1], there were several concerns about the contrib code
for the macOS keychain support. After more careful review of this
code base, it was considered to be in such a bad shape that it will
need a massive overhaul. There were more issues than what the security
audit revealed.
It was attempted several times to get in touch with the contributor
of this code; with no response at all [2]. There has however
been some discussions with the Tunnelblick project [3]. There is one
person there willing to go through this and improve the situation.
The main Tunnelblick maintainer is also willing to include the improved
code to their project instead of having this as a contrib code in
the upstream OpenVPN project.
So this patch just removes the code which we will no longer
ship as part of OpenVPN - and the Tunnelblick project will take
over the responsibility for this code base on their own. And since
this code base is purely macOS specific, this seems to be a far
better place for this code to reside.
Signed-off-by: David Sommerseth <davids@openvpn.net>
[1]
<http://community.openvpn.net/openvpn/wiki/QuarkslabAndCryptographyEngineer
Audits#OVPN-04-1:PossibleNULLpointerderefenceincontribkeychain-mcdcert_data
.c>
[2]
<https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14559.
html>
[3] <https://github.com/Tunnelblick/Tunnelblick/pull/369> Acked-by: Jonathan K. Bullard <jkbullard@gmail.com>
Message-Id: <20170725130314.12919-1-davids@openvpn.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15130.html Signed-off-by: David Sommerseth <davids@openvpn.net>
Arne Schwabe [Sun, 23 Jul 2017 16:45:36 +0000 (18:45 +0200)]
Print ec bit details, refuse management-external-key if key is not RSA
V2: Print also curve details, add missing ifdef
V3: Goto err instead of using M_FATAL, format fixes, use
EC_GROUP_get_curve_name + OBJ_nid2sn instead of ECPKParameters_print, add
compat headers for 1.0.2
V4: Formatting changes and change M_ERR to M_WARN
Several binary buffers in the ntlm component are stored
as char *, however this generates a lot of warnings, because
hashing functions expect something unsigned.
Convert binary buffers to uint8_t *, while use explicit cast
for buffers that are really carrying a string inside.
This commit removes several warnings from ntlm.c that you can
catch with "-Wall -std=c99".
[DS: Done minor typo-fixes in commit message at commit time]
Signed-off-by: Antonio Quartulli <a@unstable.cc> Acked-by: Steffan Karger <steffan@karger.me>
Message-Id: <20170710043441.24770-2-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15032.html Signed-off-by: David Sommerseth <davids@openvpn.net>
management: preserve wait_for_push field when asking for user/pass
With the introduction of the wait_for_push field in the auth_user_pass
structure, we have to make sure that such field is not accidentally
erased when the management asks the user for user/pass.
Erasing such field would mess up the logic introduced by
("Ignore auth-nocache for auth-user-pass if auth-token is pushed").
Thanks to David Sommerseth for the preliminary analysis and debugging.
Reported-by: Steven Haigh <netwiz@crc.id.au> Signed-off-by: Antonio Quartulli <a@unstable.cc> Tested-by: Steven Haigh <netwiz@crc.id.au> Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20170707140108.31612-1-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15015.html Signed-off-by: David Sommerseth <davids@openvpn.net>
- fix typ0 in message: NLSMG -> NLMSG
- use strerror() to print a human readable message
- don't print error message if error is ENETUNREACH: it means no route
found
Signed-off-by: Antonio Quartulli <a@unstable.cc> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20170720082338.1302-1-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15101.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
openvpn_sleep() is basically "service the management interface for x
seconds, then return". Therefore, manage.c is a more suitable location
than the random collection of unrelated stuff called misc.c.
(I think we should find a better place for everything currently in misc.c,
and get rid of it all together. This patch is part of that effort.)
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1500566435-29920-1-git-send-email-steffan.karger@fox-it.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15109.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
This function was only called in string format functions, which already
copy the contents, so all this ever did was adding redundant malloc() and
free() calls.
Also, this wasn't as thread-safe as it claims: another thread could still
change the string value between the strerror() and buf_printf() calls. So,
instead of a not needed false sense of thread-safeness, just be honest and
use strerror() directly.
(I think we should find a better place for everything currently in misc.c,
and get rid of it all together. In this case, the better place is
/dev/null. This patch is part of that effort.)
Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1500550740-24773-1-git-send-email-steffan.karger@fox-it.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15105.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
The argument passed to my_strupr() is converted to an upper case
string by means of toupper(). The latter expects a single signed int
as argument, therefore it makes sense to have my_strupr() take a
signed argument too and avoid an explicit and an implicit cast.
Signed-off-by: Antonio Quartulli <a@unstable.cc> Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <20170710043441.24770-3-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15031.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
when passing the M_ERRNO flag to msg(), the latter will already
print the errno message (in a form of a string and number) for us,
hence there is no need to explicitly print it a second time.
Signed-off-by: Antonio Quartulli <antonio@openvpn.net> Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20170713080527.13299-2-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15057.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
David Sommerseth [Wed, 28 Jun 2017 19:15:38 +0000 (21:15 +0200)]
doc: The CRL processing is not a deprecated feature
The note related to the CRL processing was somehow put into
the deprecated section. This is quite confusing.
Since this is a fairly important change, and there have been
a noticable amount of supports questions related to OpenVPN
not starting due to CRL errors, I put this into the
"New features" section labelled as an improvement. Otherwise
I fear this would drown in the list of "User-visible Changes"
later on.
Signed-off-by: David Sommerseth <davids@openvpn.net> Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <20170628191538.9135-1-davids@openvpn.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14985.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Steffan Karger [Tue, 27 Jun 2017 22:20:29 +0000 (00:20 +0200)]
Undo cipher push in client options state if cipher is rejected
Because of the way we re-use the options parser for both config files and
pushed options, we always update the local options state when we accept an
option. This resulted in a pushed cipher being rejected the first time it
was pushed, but being accepted the second time.
This patch is a minimal way to resolve this issue in the master and
release/2.4 branches. I'll send a more invasive patch for master, to
reset the entire options state on reconnects, later.
Trac: #906
Signed-off-by: Steffan Karger <steffan@karger.me> Acked-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20170627222029.26623-1-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14984.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Emmanuel Deloget [Thu, 29 Jun 2017 14:21:19 +0000 (16:21 +0200)]
OpenSSL: remove EVP_CIPHER_CTX_free() from the compat layer
For unknown reason, the writer of the compat layer seemed to think that
this function was only present in OpenSSL 1.1. This is not the case at
all, since it has been introduced in OpenSSL before version 0.9.8.
Thus, there is no need to add this function to the compat layer, and it
can be safely removed.
Emmanuel Deloget [Thu, 29 Jun 2017 14:21:18 +0000 (16:21 +0200)]
OpenSSL: remove EVP_CIPHER_CTX_new() from the compat layer
For unknown reason, the writer of the compat layer seemed to think that
this function was only present in OpenSSL 1.1. This is not the case at
all, since it has been introduced in OpenSSL before version 0.9.8.
Thus, there is no need to add this function to the compat layer, and it
can be safely removed.
Steffan Karger [Wed, 21 Jun 2017 21:10:43 +0000 (23:10 +0200)]
Move adjust_power_of_2() to integer.h
misc.c is a mess of incoherent functions, and is therefore included by
virtually all our source files. That makes testing harder than it should
be. As a first step of cleaning up misc.c, move adjust_power_of_2() to
integer.h, which is a more suitable place for a function like this.
This allows us to remove the duplicate implementation from test_argv.c.
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20170621211043.6490-1-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14940.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Steffan Karger [Mon, 19 Jun 2017 11:51:05 +0000 (13:51 +0200)]
init_key_ctx: key and iv arguments can (now) be const
In older OpenSSL, the key and iv arguments of EVP_CipherInit_ex() were not
const, which meant that our API could not be const either. Since we
dropped support for OpenSSL 0.9.8, we can now fix our internal API.
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <1497873065-2229-1-git-send-email-steffan.karger@fox-it.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14881.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Arne Schwabe [Mon, 26 Jun 2017 11:13:26 +0000 (13:13 +0200)]
Set tls-cipher restriction before loading certificates
OpenSSL 1.1 does not allow MD5 signed certificates by default anymore.
This can be enabled again by settings tls-cipher "DEFAULT:@SECLEVEL=0" but
only if the cipher list is set before loading the certificates. This patch
changes the order of loading.
Emmanuel Deloget [Mon, 19 Jun 2017 15:35:13 +0000 (17:35 +0200)]
OpenSSL: remove pre-1.1 function from the OpenSSL compat interface
HMAC_CTX_init() has been removed from OpenSSL 1.1. Both this function
and function HMAC_CTX_cleanup() has been replaced by HMAC_CTX_reset().
Commit aba98e9050eb54d72d921e70bcd422cb892b9c6c introduced support for
HMAC_CTX_init() for OpenSSL 1.1+ while other functions were mimicking
the OpenSSL 1.1 interface for earlier version. This is clearly not a
good idea -- a better approach would be to provide the new interface for
pre-1.1 versions in order to have the dependant code use only one
interface version. To implement that, we remove HMAC_CTX_init() from our
compatibility layer and implement HMAC_CTX_reset() in terms of a cleanup
followed by an init (as the regular HMAC_CTX_reset() function does in
OpenSSL 1.1. This change has a consequence on HMAC_CTX_free() which now
need to cleanup() the HMAC context before freeing it.
Ilya Shipitsin [Mon, 19 Jun 2017 18:38:08 +0000 (23:38 +0500)]
travis-ci: added gcc and clang openssl-1.1.0 builds
openssl build script was modified according to official openssl manual:
https://wiki.openssl.org/index.php/Compilation_and_Installation Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <1497897488-15999-1-git-send-email-chipitsine@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14890.html
projects / thirdparty / openvpn.git / log
