- [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 also eliminates the undefined behaviour when rekey_type_defined
was false and rekey_type was not defined but copied
Reported-By: Trail of Bits (TOB-OVPN3-11)
Signed-off-by: Arne Schwabe <arne@openvpn.net>
This commit adds two useful numeric limiting functions in
two headers plus a third supporting header and unit tests.
The unit tests cover all code paths and many conditions
but may not be 100% complete from a viewpoint of
covering all edge cases.
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.net>
Rewrote Win32 conversion routines to use Win32 native
conversion MultiByteToWideChar and WideCharToMultiByte.
When we go to a C++ version that supplies a non-
deprecated replacement we could revisit this.
This function were something doing 64 bit shifts on 64 bit integers
which is not defined. Ensure that all our shifts are between 0 and
63 and restructure the function to flatten the if conditions and
use recursion for the two shift instead of repeating the same logic
for the two shifts.
Reported-By: Trail of Bits (TOB-OVPN3-6)
The current mixing of signed and unsigned is undefined behaviour. Avoid
it by explicitly only using unsiged integers.
Also fix the same issue in the test_prefixlen unit test
Reported-By: Trail of Bits (TOB-OVPN3-5)
When agent-enabled client disconnects, it signals
destroy_tun event, which signals to agent that tun
has to be teared down. For dco-win, event handle is passed
to agent with /tun-open request.
Before sending /establish request, client closes previous
tun instance. Closing tun involves signaling destroy_tun event.
Event handle is closed after signaling, and here we have a problem:
- client calls /tun-open and passes event handle to agent
- client calls /establish, and before that it signals destroy_tun
event, which handle is now closed
- at some point client disconnects and signals tun_destroy event
Since event was already signaled and its handle is closed, nothing
happens and agent doesn't tear tun down. As a consequence, DNS
resolution might not work if DNS is overriden by VPN.
When client exits, agent tears tun down by failsafe logic. This doesn't
work for Connect client, which obviously doesn't exit on disconnect.
Fix this problem by avoiding signaling event between /tun-open
and /establish requests. This is done by not adding tun_setup
destructor (which signals event) to tun_persist right after /tun-open
call. There is nothing to tear down at that point yet since tun is
opened later by /establish call.
As a downside of this approach, we lose callback in client code
if agent process dies in between /tun-setup and /establish. This is
not a big problem IMO and can be fixed later.
In addition to that, send destroy_tun event also in /establish
request when using dco. This is needed to cover persist-tun case
when we reconnect and get new tun options. In this case we instantiate
new tun_setup instance, but don't call /tun-open since we keep tun
handle. Hence we have to pass destroy_tun event via /establish request.
Fixes https://github.com/OpenVPN/openvpn3/issues/257
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Previously, ConstBuffer was simply a BufferType with a const
data type. However this model, and the fact that BufferType
has a vtable, makes it difficult to efficiently cast Buffer
to ConstBuffer via static_cast without introducing an unsafe
downcast.
This commit tries a different approach by factoring out const
BufferType operations into a new base class ConstBufferType.
In the new model, BufferType inherits from ConstBufferType.
Member functions that treat the underlying data buffer as
const have been moved to ConstBufferType while member
functions that treat it as mutable remain in BufferType.
This makes casting BufferType to ConstBufferType a trivial
upcast while also greatly simplifying const_buffer_ref().
Signed-off-by: James Yonan <james@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>
Adds a library method C2os:cast() that converts an iterable container,
i.e., one that can be a range-expression in a range-based for loop,
into a type that can be inserted into an ostream. This only addresses
the container semantics in the ostream insertion. The underlying
contained type T (if the container were stl, the value_type) must work
with ostream<<.
The result of the operator<< insertion is a square bracket enclosed,
comma delimited string of the items in the container. Note that the
commit includes ideas on expanding choices of container rendering
details.
Attribution to James Yonan. Made significant contribution to
expanding the scope of collections. And reduced code complexity.
Also to Charlie Vigue; eliminated the "first" test inside the loop.
Signed-off-by: Mark Deric <jmark@openvpn.net>
The crypto library function from OpenSSL uses custom assembler code
and should be safe. Also the code has been excersised already by the
Android/iOS builds.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
This patch adds announcing support for control channel exit notification
and sending the "EXIT" control channel message.
This is only the client side. Servers support this since OpenVPN 2.6.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
This commit removes the ability to pass down the windows sizes for ack
windows down from the configuration. This capability was never used and
instead the receive and send window were both hardcoded at 4. Also
change the receive window to 12 and the send window to 6 like
OpenVPN 2.6 does.
Also to improve control channel reliability, resend previous ACKs in MRU
fashion if there is still room for them in a control channel packet.
This patch is based on a patch was written
by Charlie Vigue <charlie.vigue@openvpn.net>.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
throw() is the same as noexcept(true), which is the same as noexpect.
(https://en.cppreference.com/w/cpp/language/noexcept_spec)
noexpect is more standard nowadays and less likely to create confusion.
Single argument constructors should be marked explicit so they do not
end up being acidentially called.
This adds proper handling of AUTH_FAILED,TEMP server responses,
potentially modifying the restart delay time and which address
is to be used for the next connection attempt.
Changes the reconnect behavior so that all addresses of a remote
are tried in case of a connection error, instead of continuing
with the next remote immediately.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
By giving an advance type to RemoteList::next() optionally, the
behavior of the function can be modified to move to the next remote
instead of the next address, or not move at all.
Signed-off-by: Heiko Hund <heiko@openvpn.net>
Commit ae99307 ("tun: add persis-tun support for dco-win")
broke handling of premature exit of agent process. Introduced
"tun_persist->close_destructor()" call in "tun_start() " also closes
agent process handle within WinCommandAgent,
which triggers fail handler (without error code).
Fix by "re-arming" fail handler after "close_destructor()"
call in "tun_start()".
Signed-off-by: Lev Stipakov <lev@openvpn.net>
This commit changes the way the core deals with UV_ variables. They
now also respect push-peer-info (like in OpenVPN 2.x) and if variables
are present in both client.peerInfo and as setenv in the configuration
content, only the ones from peerInfo are send to the server.
The new behaviour can be tested with
ovpncli -I UV_TEST=cmdline conf.ovpn
and conf.ovpn having a setenv UV_TEST foobar in it as well
Signed-off-by: Arne Schwabe <arne@openvpn.net>
parent->transport_connecting() might trigger stop(),
reset device handle and set halt to true if TCP server
is down. In this case we should not queue read.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Add DcoTunPersist object to DCO::TunConfig.
DcoTunPersist stores:
- device handle
- tun settings
- adapter index/name
- pointer to TunSetup object, which itself
stores commands to undo tun settings
When intializing client options, instantiate DcoTunPersist
object within the scope of ClientConfig, which serves as
transport and tun factory for dco. Indicate that "sock" object
(device handle) should be preserved - not replaced when persisting
tun settings.
When establishing dco-win connection in OvpnDcoWinClient,
either use tun_persist created above (if persistance is enabled)
or instantiate it in-place (no persistance).
If nothing is stored in tun_persist (means this is first
connection or reconnect without persistance), acquire device
handle from tun_setup, wrap it into ASIO's basic_stream_handle
and store it in OvpnDcoWinClient - no need to persist it yet.
When starting tun, check if persisted tun session matches
to-be-created session. If no - clear previous tun settings,
set up tun and persist tun state. If device handle is already
stored in tun_persist, it won't be replaced.
On tun stop, send DEL_PEER command, which deletes peer
from the driver but keeps adapter in connected state. Then
close locally stored ASIO handle and reset tun_persist.
In case of "short term persistance" this will undo tun settings
and close device handle. For long term persistence, tun_persist
is also stored in ClientConfig and handle won't be closed yet.
In case of disconnect, ClientConfig::finalize(disconnect=true)
is called, which resets tun_persist, which in turn
undoes tun settings and closes device handle.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
