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>
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>
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>
Fixed one issue while at it, with parse() not clearing
the username and password arguments.
The general issue that overflow doesn't throw is reflected in
a disabled test. This will need to be fixed in SplitLines,
probably.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
This really has very different implications than the
others overloads. So make it distinct.
I would also rename the others to parse_opt, but feel
that causes too much churn in the code. parse_file has
only one use in our own code base.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
They might not be identical (e.g. Win32).
Since everything in this function is limited by
block_size, the casts are all safe.
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
read_fast_dict() is a convenience function that reads JSON
from a file, but also validates that the top-level construct
of the file is a JSON dictionary.
Signed-off-by: James Yonan <james@openvpn.net>
The Option constructor is quite general and therefore
prone to implicit matching. Make it explicit to prevent
that.
Signed-off-by: James Yonan <james@openvpn.net>
At the moment meta options are parsed only from
content. This doesn't work well with iOS where
config is imported via content_list. The config might
contain meta options, which currently won't be
recognized as meta and connection won't be established
due to "unknown option" error.
This adds meta options parsing to content_list.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
- Used static_cast instead of direct type conversions in places where
it's safe
- Used numeric_cast where failure is possible
- Changed types of arguments and locals when practical
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
This commit mostly reverts a previous commit:
commit b7bc687396
Avoid compiler warning with gcc by not using move semantics
The previous commit changed the semantics of the client
callback to use copy instead of move semantics on the
filename string to placate a compiler warning which was
later determined to be a false positive.
We revert to calling the client-provided func()
with move semantics on the filename parameter.
We also retain the use of std::invoke to call the
client-provided callback.
Signed-off-by: James Yonan <james@openvpn.net>
This is though to be a bug in the GCC compiler.
Ignore these warnings on GCC 12/13 to avoid breaking
Werror.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
gcc 12+ warn about temporary that used after its lifetime when we use
the move semantics here. Since the code here is not super performance
critical just remove the move semantics to be able to compile with
Werror.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
The code for browser opening on Unix platforms assumes some includes to
be always present but nulltun + FreeBSD actually do not have include them.
So include them explicitly. And since this break windows, ensure that
process.hpp does nothing on that platform.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Including cstdint is required by uintmax_t.
Fixes the following:
openvpn/common/numeric_cast.hpp:66:25: error: ‘uintmax_t’ does not name a type
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
Copy/move construction/assignment are no-ops because
we always want to default-construct the refcount from
scratch. We never want to actually copy or move
the refcount because that would break any
referencing smart pointers.
Signed-off-by: James Yonan <james@openvpn.net>
with the new C++17 capability of inline for variables, we can avoid
having to ifdef tricks to only include the variables into one compilation
unit. Also remove the extern.hpp that serves no purpose now anymore.
Signed-off-by: Arne Schwabe <arne@openvpn.net>
Password is not echoed and submitted when Enter is pressed.
This requires not removing ENABLE_PROCESSED_INPUT and ENABLE_LINE_INPUT
flags.
Signed-off-by: Lev Stipakov <lev@openvpn.net>
we often have configuration files where a directive is duplicated and
the later one wins. This is quite common and should not rais an error. We
still warn about these as this might an error/oversight.
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 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>
