This is something useful for debugging. We do not expose this feature
to avoid it being used for real connections.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Prior to this branch the various instances of the LoggerMixin were
coincidentally shared, depending on whether the default levels had
the same values in the template arguments or not. Since it's not clear
this sharing was intended or accidental I made it possible to tag if
desired to ensure the similarly tagged instances are unique.
The host route to the VPN server disappeared when a mac client, using
the agent, was reconnecting. That was causing --redirect-gateway tunnels
to break because no traffic could be sent anymore. Cause for this was
some internal state in the agent not being reset when the utun device
is temporarily removed during the restart.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
log_level() was returning a function (log_.log_level instead
of log_.log_level()). Now fixed.
Signed-off-by: Razvan Cojocaru <razvan.cojocaru@openvpn.com>
This is reusing the auth pending method as this is just another method where
the final authentication decision is pending on some results. In this case
custom messages going back and forth. This can be tested using
client-pending-auth 5 1 "ACC:1000 A:6 cck1:certcheck:cxa1:fortune" 60'
with OpenVPN 2.x servers easily.
Also correctly use ClientEvent::Base::Ptr with the Clientevents to avoid
problems with not correctly using RCPtr
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Commit 0c5e356 ("Simplify client options classes") has
indeed simplified options handling by consolidating
options into the base class. However "disble_client_cert"
option was copied, not moved, from its original location.
As a consequence, it become broken.
Fix by removing unneeded copy of this option and use the
one from correct location in options base class.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Instead of silently ignoring errors in control channel message and removing
invalid characters, we should be more strict and reject these message.
A similar change has been also submitted to OpenVPN 2.x
Clients need access to the server VPN ca for whitelisting reasons
so it is now available inside the EvalConfig structure. Implemented
the change and added a unit test for same.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.net>
The syslog.h UNIX header already #defines LOG_{DEBUG, INFO} as
log-level constants, which means that we can't have code that
includes both openvpn/log/logger.hpp and syslog.h.
This patch renames all the LOG_<LEVEL>() macros to
OVPN_LOG_<LEVEL>(), to hopefully eliminate "macro already defined"
conflicts in the future.
Signed-off-by: Razvan Cojocaru <razvan.cojocaru@openvpn.com>
This allow debugging the certificates the server sends. It did this as hack
two times before. I think it is now time to get this officially in.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Compilers will otherwise complain about unused function when this header
is included in a compilation unit that only uses a subset of the functions
(e.g. in a unit test).
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Before this commit traffic to loopback was limited when only DNS
(port 53) was blocked, due to the "not loopback" match condition being
replaced instead of the match condition being made more specific.
This broke the client option to override access to DNS servers listening
on loopback.
To fix this three things are done:
1) do not add DNS block rules if the override option is active.
2) explicitly block port 53 on loopback, except when the override
option is active.
3) remove the implicit block of port 53 on loopback and instead let
the firewall rule for non-loopback devices only.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
This also moves some of the reasonsibility from ClientProto to
OpenVPNClient. This plays better with the EPKI implementation and also
does not break the idea of the current certcheck implementation as we now
just give the certcheck in client protocol a preconfigured SSL Config instead
of all the certificates individually.
The implementation would previously return the alias the client library
requested to use but would not allow to have multiple different
external aliases be correctly used. This adds supports to have the
correct alias being used as part of the signature callback.
Instead of relying on passing an empty domain name into the NRPT class
for the '.' rule not to be created, skip calling the NRPT code
altogether. Since there's no rule generated in the case where local
resolvers should be used when no split DNS is to be configured, skipping
the NRPT call is more readable and less magic, when viewed from the
setup class. Also more effective during runtime.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
This is meant to help with the following scenario: main thread
creates secondary threads, each secondary thread asks the context
store for its own context, then goes into a "forever" scheduling
loop. Main thread waits for the secondary threads to finish, and
owns the context store. It would be nice to be able to have yet
another thread (say, a SIGINT handler) be able to call .stop()
on all the contexts managed by the context store, thus being able
to end the loops in the secondary threads and allow the main
thread to exit.
Signed-off-by: Razvan Cojocaru <razvan.cojocaru@openvpn.com>
With DCO, userspace has no visibility of packets arriving
on data channel. To provide "last packet received" time,
update it when stats are pulled from DCO and there is a
difference in incoming transport bytes.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
The AppControl feature provides an internal communications channel that
is described in detail in the relevant commits that implement it. This
change adds two intrinsic app control 'protocols' to the list of
supported handlers in the client. The new protocols are:
- cxa1: This is a request for the client to begin a TLS handshake via
the app control channel.
- cck1: This is the protocol that allows the exchange of the requested
TLS handshake data.
The 'cxa1' handler parses the request and initiates the handshake from
the client. This handshake is exchanged via the 'cck1' protocol and
serves to prove to the server that the client has access to the required
private key.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
According to the notes in the issues and changelog GCC
has fixed some false-positives of this but basically
accepted the rest and will not fix them.
They introduced gnu::no_dangling attribute to allow people
to override this warning on a class/function basis. But
that doesn't help us since we also need to support older
compilers.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
pool.hpp:158:13: error:
template-id not allowed for destructor in C++20
[-Werror=template-id-cdtor]
158 | virtual ~PoolType<ADDR>() = default;
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
This flag was introduced to allow clients to decide if they want to
ignore non-split DNS option pushed to them. So, to be compatible with
the previous behavior with --dhcp-option, we act on the flag as wenn
when there are no resolve-domains specified.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
The option is only enforced with the --dns option, since DNS settings
coming in via --dhcp-option have always voluntarily blocked port 53.
This behavior is kept for backwards compatibility.
Since the --dns option allows local name servers to continue to work,
even thought no split DNS is pushed, supporting the option makes sense.
If admins do not want any DNS queries outside the tunnel, this is the
option to push alongside the --dns options.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Incompatible changes to the --dns server address and --dns server
exclude-domains options were introduced after the code for handling them
was released. Add and send a new IV_PROTO flag, so servers which act on
the flags set can differentiate between clients which have implemented
--dns and those which just support the new option. This enables them to
decide which variant of options to send to the client.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Previous to this --dns and DNS related --dhcp-options shared the same
code to apply the settings to Windows and macOS systems. So, both
options were pretty much just aliases, with --dns offering more and
finer grained settings that were mostly ignored.
Now --dhcp-options are applied the way they have always been and --dns
does it its own - the new - way. Reason for this behavioral change is
foremost that we want it to be the same between openvpn version 2 and
version 3. But there are also a few new features (e.g. DNSSEC), previously
not present with the --dhcp-options.
The name server and split-domain configuration is exclusively set via
NRPT on Windows, since it overrules any other resolver setting. If there
is no split DNS configured and all domains are resolved using the pushed
name server, we make sure that local domain names are still resolvable by
adding so called exclude NRPT rules, that make sure local domains get
resolved by their local DNS resolvers.
Since Windows does not know about alternative secure transports, the
'transport' and 'sni' settings are ignored.
For macOS the 'dnssec' setting is ignored in addition to that. Besides
that not much does change on that platform. In case of --dns options the
explicit values are used now. The API in use may be changed at a later time.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Create a struct NetApi, which contains various network related functions
that will be used. This is done so that these operations can be injected
as a dependency and thus replaced with mock operation for the purpose of
testing.
There are also functions which operate solely on the Registry, those are
left out of the NetApi since they can already be abstracted by struct Reg.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Create a struct Reg, which contains various setter and getter functions
for different registry types and other operations that will be used.
This is done so that these operations can be injected as a dependency
and thus replaced with mock operation for the purpose of testing.
Besides that it makes code more brief and less error prone, since
there's now one implementation for converting C <-> C++ for each operation.
Move existing class RegKey and class RegKeyEnumerator into struct Reg as
well, so they are now known as Reg::Key and Reg::KeyEnumerator.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Functions from_utf8() and to_utf8() added one extra '\0' glyph to the
output string, i.e. if the input basic_string::size() was 8 the output
was 9. Normally this would not make a difference since for most string
usage, as the extra NUL at the end would mostly be ignored. However if
you used the output string to append to another string the extra NULs
were actually also appended, resulting in a string with embedded NUL
characters. Which is a problem with the next use case.
The pack_string_vector() function failed to produce a wide MULTI_SZ
string from a vector of strings, unlike advertised. The extra NUL
actually led to the MULTI_SZ string always being terminated after the
first string. Besides that, the function actually never terminated the
MULTI_SZ in the first place and also failed to handle empty vectors
gracefully.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Since C++17 the codecvt header is deprecated and scheduled for removal
in C++26. MSVC warns about use of the deprecated API already, other will
follow soon. It was decided to deprecate and remove it since it does not
support the current UNICODE standard anymore.
Also test for the _WIN32 define, instead of WIN32, so that this keeps
working with MinGW headers, when cross-compiling.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
MinGW's g++ displays this warning when compiling:
warning: the address of ‘IP_ADDRESS_STRING::String’ will never be NULL [-Waddress]
since String is defined as a C array, it can never be nullptr, so the
warning is correct and the check can be removed.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Instead of using nullptr for uninitialized RegKey, use the value defined
in WIN32 API for that. We need to check for it anyways, so unifying this
makes the checks more straight forward.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
OpenVPN uses a idiosyncrasy that all ciphers are uppercase but none is
spelt lowercase and excepts this idiosyncrasy also in IV_CIPHERS
Signed-off-by: Arne Schwabe <arne@openvpn.net>
The reauthentication logic differs from openvpn2
and the code is a bit hard to follow. Simplify
the code and make it behave like in openvpn2.
- password is cached by default
- password is purged when auth-nocache is presented in a local config or pushed
- when AUTH_FAILED is received and we have no session-id, throw a fatal error
- when AUTH_FAILED is received and user interaction is required for
authentication (MFA), throw a fatal error
- when AUTH_FAILED is received, user interaction is not required
for authentication and either we have a cached password OR password is not
needed, we reconnect.
Password is "needed" when non-empty password is provided.
User interaction is required for static/dynamic challenge and SAML.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
In openvpn2 WFP block filters are added when the 'block-local' flag is
pushed to clients together with --redirect-{gateway|private}. That is
done in addition to adding routes to harden defense against attacks
collectively known as Tunnelcrack on Windows systems.
Since the openvpn3 library did not deal with the block-local flag at all
before this commit, on Windows it is sufficient to simply block traffic
to local interfaces by placing firewall rules. Traffic will only be
allowed originating from the OpenVPN process, on the VPN interface, and
loopback.
Note that previously WFP rules were already added to prevent access to
local DNS servers, when DNS servers were pushed. These are contained
within the ones added with 'block-local' and need not be set
additionally in that case.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Some classes are moved to subclasses of class WFP. Other things just got
a more descriptive name. Here is what this commit changes effectively:
* class WFPContext -> class WFP::Context
* class WFP::WFPEngine -> class WFP::EngineHandle (private)
* class ActionWFP is split into ActionBase and two derived classes
ActionBlock and ActionUnblock, so that the purpose is more visible
to the uninitiated observer (instead of just a bool making the
difference)
* instead of the 'tap_' prefix to names, use 'itf_' now, since we're
not only dealing with tap interfaces anymore
* INVALID_HANDLE_VALUE is used instead of NULL to mark a WIN32 handle
as uninitialized
Signed-off-by: Heiko Hund <heiko@openvpn.net>
We currently accept cipher none as pushed cipher when legacy ciphers are
enabled but do not announce support for it in IV_CIPHERS. This means we
currently display inconsistent behaviour. Servers that ignore IV_CIPHERS
can use none but server that are correctly working cannot.
Setting ADAPTER_DOMAIN_SUFFIX for non-DHCP adapters requires
registry modification. For that, we need adapter GUID.
This passes adapter GUID from agent to client via /tun-open call
and then from client to agent via /tun-setup call, when adapter
domain suffix is set.
Github: #304
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Signed-off-by: Krasovskiy Saveliy Igorevich <skrasovskiy@ozon.ru>
With compression logic now being able to be tuned at runtime the
different executable to allow a different define for the compression
log level is no longer needed.
Prefix the defines of test_proto.cpp with PROTO_ to avoid collision of
these very generic named defines
Signed-off-by: Arne Schwabe <arne@openvpn.net>
This allows the test_proto.cpp to supress all the logging of this
class. This is also the only place in our project that actually uses
a non-default loglevel for this class. A lot of files were defining the
OPENVPN_LOG_SSL(x) macro to be what the also remove ssllog.hpp would do
anyway if it were not defined.
The removed debug_level field only controlled the mssfix
logging and is now controlled by the general protocol logging instead.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
This template base class allows specifying a maximum log level that
can be used to make the logging function empty above that level.
This allows us to make our logging more structured and move to a more
standard logging approach. The use of if constexpr ensures that logging of
very high level is not emitting code.
This also add a few macros (LOG_VERBOSE, LOG_INFO, LOG_TRACE) that are drop
in replacements for the old OPENVPN_xx_LOG and OPENVPN_xx_LOG_VERBOSE macros
and also ensure not to do string construction if logging at a certain level
is already disabled at compile time.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
The code in dco/dcocli.hpp used #if ENABLE_KOVPN, which will
fail on newer compilers if the macro is defined in a source
file. Compilers may not complain if the macro is defined on
the command line, via -D.
This type of checks should use either #ifdef or #if defined(...).
The #if conditional expects a boolean expression.
Since these code blocks also depended on #elif (also expects
a boolean expression , the defined(...) approach was chosen
throughout this file.
Signed-off-by: David Sommerseth <davids@openvpn.net>
Currently the protocontext is used as kind of composition but not really
and makes following the code harder, since this inheritance not only serves
for composition but also as callbacks through virtual method inheritance.
Making ProtoContext a normal field and definining a callback interface makes
the class relationship easier to understand.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
By resetting the timer on each incoming packet, we can capture more gaps.
Consider the following examples:
Old logic:
0 seconds: openvpn3 starts with an inactive timeout of 60 seconds.
30 seconds: An ICMP packet was received with 48 bytes.
60 seconds: 48 bytes received for the last 60 seconds, continue...
90 seconds: An ICMP packet was received with 48 bytes.
120 seconds: 48 bytes received...
New logic:
0 seconds: openvpn3 starts with an inactive timeout of 60 seconds.
30 seconds: An ICMP packet was received with 48 bytes.
Inactive timeout reset!.
90 seconds: Inactive timeout triggered. Terminating session..."
Signed-off-by: illia.polishchuk illia.polishchuk@openvpn.com
When correcting conversion issues in RouteBase a to_string bug was
introduced which caused some characters to be escaped when inserted
to the string, for example a prefix_len of 0 would render as "\0"
rather than inserting '0'. The std::ios::binary flag does not seem
to prevent this for std::ostringstream so I have cast the data member
up to uint16_t which should be safe, and solves the issue.
Added a unit test to demonstrate the issue. Old code output was
"0.0.0.0/\0", now outputs "0.0.0.0/0" as expected.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
A refactoring of the logging code in commit 9ffa263b removed
conventional #ifndef header guards and replaced them with #pragma
once. Some consumers of the code used the header #defines to guide
their behavior. The missing guards caused the consumers to log
incorrectly. The fix was to still rely on the #pragma once for
guarding, but to reinstate the #defines from the original headers.
Signed-off-by: Mark Deric <jmark@openvpn.net>
The previous commit restructured the way how peer info was built and
accidentally move those into its own method without calling the method.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
In commit 1b4f736bb9, an additional parentheses was added to
the MacGatewayInfo constructor. This results in code which cannot be
compiled.
Signed-off-by: David Sommerseth <davids@openvpn.net>
A few minor changes:
- add ORGANIZATION meta option to ignore list
- remove excessive OVPN_ACCESS_SERVER_ prefix from NO_WEB meta option
- Increase status message length from 256 to 2048 to be able to show
the full list of unsupported options
Signed-off-by: Lev Stipakov <lev@openvpn.net>
ERR_INVALID_OPTION_DNS -- invalid value for some of DNS\Domain options
ERR_INVALID_OPTION_CRYPTO -- invalid value for some of SSL\Crypto option
ERR_INVALID_CONFIG -- missing option or not supported option
ERR_INVALID_OPTION_PUSHED -- pushed to server option error
ERR_INVALID_OPTION_VAL -- invalid value for some general option
Signed-off-by: Illia Polishchuk <illia.polishchuk@openvpn.com>
The OPENVPN_EXCEPTION_WITH_CODE(C, DEFAULT_CODE, ...)
macro creates enum C_code with __VA_ARGS__ codes
and constructor with the C_code as first argument which
adds label at the beginning of error message, other
constructors add DEFAULT_CODE label
Signed-off-by: Illia Polishchuk <illia.polishchuk@openvpn.com>
Until now sitnl was just default to metric 0 when installing routes,
while ignoring any value that may have been passed by the user.
Extend logic to properly accept a user value.
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
When the user specifies no metric (i.e. value is -1), the TunBuilder
should pass the default value down the stack.
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
This also move the building IV_HWADDR peer info variable to the point
that the server address is actually available.
This also avoids failing to connect when push-peer-info is enabled and
there is no IPv4 default gateway. The new code will always pick the device
that holds the route to the current remote.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Config
client
pull
was not correctly handled like client + tls-client
since the code short-circuited if tls-client wasn't set
and so didn't touch pull option.
Github: #277
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
It was noted that the SITNL unit test is always failing for no clear
reason.
It turned out that commit 22ba196429
("SITNL: revert change of sitnl_send return type, return int"),
that was supposed to be a simple revert of
ae663c573a ("Using new numeric
conversion tools") is actually converting two "return ret" into
return -1 and return -EINVAL accordingly.
This accidental change results in two functions always returning
an error despite terminating succesfully.
This behaviour was obviously fooling the unitest which failed in result.
Fix both functions by properly returning "ret" as it was originally.
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
Helpfully the comment above the code actually provided
a solution...
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
(cherry picked from commit db7ea3d96a)
This makes it easier to see what is going on when looking at
individual CMakeLists.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
(cherry picked from commit 4c81069564)
With the change to OpenSSL 3 and introducing insecure as profile we
actually allowed MD5 again. Update the warning to reflect this.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
The purpose of this change is to allow headers that require the
logbase.hpp classes to compile in executables using logsimple.hpp.
By munging classes and macros into both headers, an avoidable conflict
of macro re-definition is created. This commit separates the classes
from the macros into new headers. Then propagates the mistake into the
current headers so none of the existing code is broken. ;-)
Signed-off-by: Mark Deric <jmark@openvpn.net>
Currently we error out on the first unsupported
option which belongs to the "fatal" category, such as
"removed deprecated option" or "Option allowed only to
be pushed by the server".
To improve user experice and allow application code
to display all problematic options and their categories,
collect options into a category->options map and then
serialize it into multiline string:
cat1: opt1,opt2
cat2: opt3
Introduce a new error code UNUSED_OPTIONS, which is
placed into ClientAPI::Status::status. The serialized
options map is placed into ClientAPI::Status::message.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
- Add notes regarding some unexpected behaviors in sslctx
- Add unit tests specifically for sslctx, including simple in-memory
handshaking with both success and failure examples.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
Out of all the suggestions by Coverity I picked
the ones that move non-Ptr objects into variables
or attributes.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
If we get a valid but almost empty PKCS7 structure we otherwise try
to access invalid fields.
CVE: CVE-2023-6247
Reported-by: Bahaa Naamneh <bahaa.cpl@gmail.com>
Signed-off-by: Arne Schwabe <arne@openvpn.net>
- Split big classes into declaration and definition
- Added doxygen
The goal here is to add make the classes easier to reason about by
splitting them into declaration and definition and then adding
doxygen.
The notify parts are left intentionally undocumented for now.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
ClientEvent::Base is the base class for many other classes including
a few that add data members. If at some point one of these enhanced
derived classes is referenced and then deleted via a base class
pointer, some memory could leak.
I don't think we do that yet, but it seems worth preventing.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
When compiling against OpenSSL 3.0, use the newer API for generating the
TLS 1.0 PRF. Older OpenSSL versions will use the OpenSSL 1.x API.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
The NTLM protocol implementation does not validate the length of
the proxy server’s response. If the response is shorter than
expected, the code will access the response buffer out of bounds,
which will raise an exception. This change checks and explicitly
raises an exception with an informative message if the response
is too short.
This was never a security issue as such but might result in a client
terminating early and without a nice diagnostic.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
CID 11851: (#1 of 1): Structurally dead code (UNREACHABLE)
unreachable: This code cannot be reached
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Instead of exposing protected data to the global environment, provide
a special purpose accessor to satisfy AppControl needs.
Signed-off-by: Mark Deric <jmark@openvpn.net>
