- 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>
Currently PG only allows to either send or withhold the reason to the
client but there are certain circumstances where you want to have more
detailed internal reason but still want to send some reason to the
client.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Some systems like to see the mapped IPv4 addresses as real IP addresses.
This commit adds the ability to show IP addresses as such.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
This adds the capability to implement a custom app level protocol
that support message passing over the OpenVPN control channel.
The protocol is agnostic to the data that is transported over it
and the message splitting/reassmbly is handled transparently by the
OpenVPN library itself.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
This is an obscure and never used feature to trigger sending
the SSO web auth URL from the client instead of server.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Using the raw pointer constructor only really works if the pointer is
intrusive. Ensure this with a static assert
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Do not run the daily scan if there was no push since the
last run.
Reduces resource usage and notification noise.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
The option compress is extremely seldom used since there virtually no sense
in using it as all clients that support the compress option also support
pushing compression, so adding a stub only compression method by default
in the configuration does not give any benefit, only downsides.
When compress is in the config *and* the server never pushes any compression
option (even "push compress" is fine), we initialise "comp-lzo no" instead.
And comp-lzo and compress are different compression stub methods (byte swap
vs no byte swap) that are incompatible.
compress without argument in config is extremely seldom used.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Some commits targeted for the Core v3.8.3 release did not end up
in the released branch and got lost. These were already approved
for the Core v3.8.3 release. This marks the starting point for
the Core v3.8.4 release.
Signed-off-by: David Sommerseth <davids@openvpn.net>
In class Stop, the stop_called member is safe to read
without locking, but make it volatile to document this.
Signed-off-by: James Yonan <james@openvpn.net>
Older clients, 2.5.x and below, send an ACK_V1 packet in response to
the server's HARD_RESET packet whereas 2.6.x clients send a CONTROL_V1
packet. The code that checked the packet length of the client's
response failed to comprehend the fact the ACK_V1 packet does not
include a packet id field following the peer session id field. So the
code rejected the ACK_V1 packet as being too short.
The fix was to require packet length only up to and including the peer
session id field. This works to allow safe parsing for both the
ACK_V1 response and the CONTROL_V1 response.
Signed-off-by: Mark Deric <jmark@openvpn.net>
hash() will return a hash of the raw time. It's useful
for unordered map/set collections where the key is a tuple
that includes a Time object.
Because hash() is a template method, it doesn't introduce
any new dependencies for class Time.
Signed-off-by: James Yonan <james@openvpn.net>
By default, the Netblock constructor already sets the server
gateway to the .1 address of the subnet, but the new method
override_server_gw() can now be used to override that
setting.
Signed-off-by: James Yonan <james@openvpn.net>
It was only supported by mbedTLS and is very easily used wrong since it
is just a boolean value. Other TLS stacks were using the regular strength
PRNG no matter what. Also we should not weaken a crypto strength PRNG,
now that we have the StrongRandomAPI type in place. It might give the
wrong sense of strength, when in reality we might reseed a hundred times
less often.
In places where prng was passed as true before, use MTRand now instead.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
RFC 5077 talks about the ticket key_names to be randomly generated to
avoid collision between servers. To enforce unpredictable randomness,
require a strong PRNG to construct such names.
This is a formality, since all users of the class already provide such a
strong randmon number source.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Since predictable names for temporary files can potentially cause a
security issue, require such filenames to be generated with
unpredictable randomness.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Since session ids should always be truly random require a
cryptographically strong random number generator.
Since all places in the codes (besides tests) already pass a strong
random source, this is just a formality.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
The need of having to call the assert_crypto() member function to ensure
that a cryptographically strong RNG is used where needed, was reported
as potentially insecure, since calling it manually can easily be missed.
In the commit the two new classes StrongRandomAPI and WeakRandomAPI are
introduced. They are to be used instead of just RandomAPI, unless it
doesn't matter what strength the RNG is.
All the places the assert_crypto() was called were converted to using
StrongRandomAPI instead. Also the RNGs for which assert_crypto() was not
throwing are now inheriting from StrongRandomAPI.
Variable names, which have the StrongRandomAPI type, but were called
prng, are changed to rng instead to follow the source code convention.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
We're pretty sure that these warnings are false positives.
Both are related to destructing MultiCompleteType objects.
GCC 12 (thread_unsafe_refcount):
rc.hpp:322:18: pointer used after ‘void operator delete(void*, std::size_t)’
[-Wuse-after-free]
GCC 13 (thread_safe_refcount, only arm64):
inlined from ‘...thread_safe_refcount::operator--()’ at .../rc.hpp:404:39
atomic_base.h:645:34: ‘long unsigned int __atomic_sub_fetch_8(...)’ writing
8 bytes into a region of size 0 overflows the destination
[-Werror=stringop-overflow=]
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Refactors the call of a virtual member function in the CTOR
by adding a private _impl function, which is non-virtual and
which both the CTOR and the original virtual function
delegate to.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
It's possible that someone might try to derive from and override
here, and if so the result will be perhaps not as expected. This
change will make it slightly harder to produce an unexpected
call to the wrong vtable dispatch from the ctor.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
This was changed in commit
ae663c573a ("Using new numeric
conversion tools") to avoid some conversion warnings. But
after understanding the workings of the function better, the
change turns out to have been wrong. Instead the function was
changed to use different intermediate variables for different
purposes.
This change ripples through the whole Netlink/SITNL interface.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Past changes make it clear that the interface was not well
understood.
While here, clean up the code to make it easier to understand.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
