Data channel packet ids (in the formats that OpenVPN 3.x supports)
are plain 32 or 64 bit ids while control channel is a 32 bit time + 32
bit counter id. Seperate these more clearly and let CBC mode use the
same Packet ID implementation that AEAD mode uses.
Also add more unit tests related to data channel tests packets by
adapting the control channel test where applicable and add a few more
related to packet id wrapping
Signed-off-by: Arne Schwabe <arne@openvpn.net>
This mode is only relevant for old OpenVPN 2.3.x clients in CBC mode
ciphers when using kovpn. Remove the mode from PID control and move
logic to kovpn key logic.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Rename BufferAllocated --> BufferAllocatedRc
Buffer: split RC from BufferAllocated
Also make changes as needed where BufferAllocated is used
Buffer: Split allocation flags into own struct
Leaving flags in template causes each alias to have identical flags
by different names, which requires each type to pointlessly use
the nested name.
Make RC: Clean up headers buffer.hpp, make_rc.hpp
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
Split the implementation of the packet counter for normal packet ID
that includes the "weird" long format for long 64 bit packet ids used
in tls-auth and tls-crypt and a simplified implementation for AEAD that
only does 32 bit and 64 bit flat counters.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Instead of passing around a number of individual argument, use a data
holder class to describe all the settings. This will also allow adding
more data channel parameters in the future (tag location, 64 bit IV)
easier. This has a slight cost of something passing more parameters
than needed.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
In the code base three different syntaxes for overriding virtual member
functions could be found:
1) virtual ... override
2) virtual ...
3) ... override
This converts all of them to the third syntax, as recommended by the ISO
C++ core guidelines in C.128
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>
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>
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>
This causes only warnings with -Wpedantic, which we don't
intend to use. But doesn't hurt to fix anyway.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
The psid cookie defense is designed to thwart resource exhaustion and
amplification attacks wherein a malicious client sends the server a
flood of CONTROL_HARD_RESET_CLIENT_V2 packets with spooofed source
addresses. This patch allows the server to defer client tracking
state creation until the client responds to the server's
CONTROL_HARD_RESET_SERVER_V2 message.
Signed-off-by: Mark Deric <jmark@openvpn.net>
We need to ensure that there is only one copy of these algorithms defined
as we modify them with the method allow_default_dc_algs to set the
F_DC_ALLOW flag on. Having more than one copy means that we have different
copies with different flags which we want to avoid.
The use of inline to a normal variable is a C++17 feature.
From https://en.cppreference.com/w/cpp/language/inline
An inline function or variable (since C++17) with external linkage
(e.g. not declared static) has the following additional properties:
There may be more than one definition of an inline function or variable
(since C++17) in the program as long as each definition appears in a
different translation unit and (for non-static inline functions and
variables (since C++17)) all definitions are identical. For example,
an inline function or an inline variable (since C++17) may be defined
in a header file that is included in multiple source files.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
While there should be no problem of having these basically static
definition multiple times, avoiding multiple copies of it is a good
thing.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
In all other places we check only if USE_OPENSSL is defined but not
if it is 0/1. Use the same check in definitions.hpp
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Digest API calculates MD4/MD5 digests etc which
are deprecated. In order to use those one needs to
load OpenSSL legacy provider and EVP_MD_fetch() to
fetch digest implementation from all loaded providers.
EVP_MD_fetch() takes library context as an argument,
so we need to pass it there through the digest api stack.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Newer mbed TLS version changed the API. This fixes our usage of the API and
also removed the micro optimisation of reusing the buffer for plain and cipher
text.
It also adds a unit test to ensure the data is correctly encrypted/decrypted.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
- [ipv4.hpp, ipv6.hpp] In both v4 and v6 headers it is safe to cast the hex
so as to eliminate the spurious warnings.
- [lz4.hpp] Apply value clamp to the hint that is sent to the compressor
to prevent a potential conversion overflow.
- [zlib.hpp] In compress_gzip, zs.s.avail_in and zs.s.avail_out are
theoretically susceptable to overflow. To prevent this we use
numeric_cast. In decompress_gzip we do a similar thing for zs.s.avail_in
but only value clamp avail_out, since the read loop looks like it will
compensate
- [buffmt.hpp] It's safe to cast the result of the arithmentically caused
promotion back down to char.
- [base64.hpp] In Base64 CTOR, changed type of a couple variables to
match the type of the table they generate. In decode, perform a static
cast to the type of the template elements the function is
instantiated for.
- [core.hpp] Perform static cast long --> int on value representing
number of cores. If we run on systems where there are more cores than
int can represent this will behave oddly, but this circumstance
seems unlikely at the present time.
- [environ.hpp] The casts seem to be safe but I have added a todo ticket
to evaluate this change further.
- [hexstr.hpp] In render_hex_char there were two conversion warnings
and a bug involving out of range input. Those are addressed.
In dump_hex the result of some math and logic is now clamped
to the range of acceptable input values for string::spaces
In parse_hex the result of converting from a hex string to an
integral value is cast to the template value_type
- [hostport.hpp] The static_cast should be safe because the value
produced by validate_port is range checked.
- [split.hpp] Applied numeric cast to ensure output of lex.get stays
within acceptable type limit.
- [stop.hpp] In Stop::Scope It's extremely unlikely but was possible for
the vector size to exceed the limit of int. The size now has a much lower
limit applied and will throw if it is exceeded.
- [string.hpp] Changed the call to toupper/tolower so they call the
locale function template instead of the cctype C function. This
eliminates the warning and the need for the cast.
- [cliproto.hpp] The computation of mss_fix is stored in a size_t and
then assigned to an unsigned short. We clamp this assignment
to the range of unsigned short.
- [tempfile.hpp] In TempFile CTOR suffixLen is computed as one type
and consumed as another. Since the CTOR is already throwing
for a couple other error conditions, I have added a
numeric_cast to the conversion that also throws in case of a
value overflow.
- [unicode.hpp] In an 8 --> 16 bit string conversion we mask and assign
in a way the compiler can't be certain is safe even though it is safe.
Added static cast to let the compiler know it's safe. In the second case
the class uses unsigned int to store a size, and then uses it in with size_t
which generates conversion warnings. I have changed the type of size
to size_t
- [logperiod.hpp] in log_period changed return type specification to
match the actual return type.
- [usergroup_retain_cap.hpp] In the unlikely event the caps size (size_t)
exceeds the range cap_set_flag can accept, an exception will be thrown.
- [crypto_aead.hpp] StaticKey::size provides a size_t where unsigned int
is required. We use numeric_cast to check the size() value in the
extremely unlikely event it is manipulated to exceed the allowed value.
- [packet_id.hpp] Code packs a time_t into a uint32_t for replay packet
ID protection purposes. The warning is supressed by a mask and cast
since the 32 bit limit is baked into the protocol and the overflow itself
does not cause a severe breakage.
- [headredact.hpp] Altered code such that the type that stores the find
result is compatible with the result from find. Additionally used the
npos constant instead of -1. There is a commented out code block that
claims to be dropped due to requiring C++ '14 - consider just using
that.
- [csum.hpp] in csum fold and cfold one has a mask and cast, the
second is just a cast to undo a promotion. Both appear safe.
- [ipv4.hpp] Values are masked and shifted so the cast should be safe.
Added cast.
- [ping4.hpp] ICMP ID and sequence number function arguments are
changed to the same type as needed by the structure. For
IPv4 header version_len 2nd arg is int but sizeof is not, so we
cast it. IPv4 tot_len is a uint16_t so we clamp to that value
range and compute it once.
- [ping6.hpp] Enforces a value constraint on the len argument to
csum_icmp, then checks the result of some math to ensure
the result will fit in the type it has to fit. In generate_echo_request
the ICMP ID and sequence args are changed to match the
type they are assigned to in the struct, and added
numeric_cast to range check payload_len.
- [remotelist.hpp] In get_endpoint, endpoint.port is called with an
unsigned int where the function is expecting an unsigned short int.
Since parse_number_throw is a function template, we just ask it to
return the correct type now.
- [compress.hpp] In v2_push we accept an int value that is assigned to
an unsigned char we push to the buffer. I changed the function to
accept an unsigned char directly.
Added unit tests - thanks Mark Deric.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.net>
This is the result after running 'clang-format -i' on all C++ files and
headers, with the defined formatting rules in .clang-format.
Only the openvpn/common/unicode-impl.hpp has been excluded, as that is
mostly a copy of an external project.
Signed-off-by: David Sommerseth <davids@openvpn.net>
Probe cipher support in runtime (works in Windows Server 2022 and Windows 11)
and add it to the list of allowed ciphers.
White on it, add missing halt check in dco-win client code.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
With OpenSSL3, these algorithms are no longer allowed. With this change
we do the same regardless of the crypto library. Note that in contrast
to OpenSSL3, we include here 3DES into the legacy algorithms.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
We already load the certificates from the config and need the SSL
library context initialised there to allow loading of keys encrypted
with legacy algorithm. Also ensure that enable legacy provider is
set before actually attempting to load the private keys.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
This allows use to load non default providers while also not touching
the default library context. THis is necessary to have profile with and
without legacy library for example
Signed-off-by: Arne Schwabe <arne@openvpn.net>
The class CryptoOvpnHMACContext is used for handling the --tls-auth
option. Since tls-auth is a control channel feature, checking for
a valid data channel digest is wrong.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Moving interpretation of the flags into the class in preparation
of submitting only supported data channel ciphers in IV_CIPHERS.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
DCO only supports a limited set of ciphers, currently it is
discovered quite late if a unsupported algorithm is configured
(or pushed).
This introduces CryptoAlgs::allow_dc_algs() with which the
supported set of data channel algorithms can be specified.
The DCO code makes use of this, at the time a new_controller()
is created.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Remove constexpr in preparation for making it possible to modify the
data channel ciphers. Use std::array so the SIZE can be specified.
Remove the unused CryptoAlgs:get_index_ptr() function.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
This also changes the mbed TLS implementation from using the AES GCM
specific API to the generic AEAD API in mbed TLS. As result we can
refactor the commonly used parts of AEAD and normal cipher into a
common class.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
The CryptoDCSettings::digest() method returns SHA1 digest when the
cipher is an AEAD cipher. This is incorrect, as AEAD ciphers does not
use digests for authentication at all; the authentication is an
integral part of the AEAD cipher itself.
To solve this, the CryptoAlgs::AlgFlags has been extended with a new
F_NO_CIPHER_DIGEST flag which is expected to be set on ciphers not
depending on any digests for authentication, like AES-GCM/AEAD
ciphers. A new method, use_cipher_digest(), will return True if
the cipher depends on a digest for authentication.
Signed-off-by: David Sommerseth <davids@openvpn.net>
