CID 304509 - it does not affect behaviour because the called function
uses it as an out parameter and the result is never inspected.
Signed-off-by: Erik Boasson <eb@ilities.com>
Failure to generate a signature for in handshake tests attempted to free
the address of the pointer, instead of the pointed-to memory (CID
304462).
Signed-off-by: Erik Boasson <eb@ilities.com>
Triggered by CID 304462, 304471, 304517: dereference before null check.
Note that it is a second-order problem because it would require the
plug-in functions to be called with a null pointer for the plug-in
instance.
Signed-off-by: Erik Boasson <eb@ilities.com>
This addresses a number of issues with building Cyclone DDS including
DDS Security while using OpenSSL 1.0.2. Compatibility with 1.0.2 is a
courtesy towards those who are unable to move to 1.1.x or later because
of other libraries.
* On Windows, one must include Winsock2.h prior to including the OpenSSL
header files, or it'll pull in incompatible definitions from Winsock.h
and that breaks some of the files.
* OpenSSL 1.0.2 requires initializing the library (or more particular,
loading all the required algorithms) but this is no longer needed in
OpenSSL 1.1.x. It ends up being needed in a few places and having tons
of essentially dead initialization code lying around is unpleasant.
Hence this has been consolidated in a single function and protected
with ddsrt_once().
* One ought to undo the above initialization on 1.0.2g and older, but it
is impossible to know whether that can safely be done from a library.
This is also the reason OpenSSL deprecated all the initialization and
cleanup interfaces. So if one insists on trying it with such an old
version, let there be some leaks.
* Thread state cleanup is sort-of required prior 1.1.0, but that suffers
from the same problems; we'd have to do per-thread cleanup code for
OpenSSL for any thread that could call into it (which is pretty much
any thread). So once again, people should just use 1.1.0 or newer.
* There are some interfaces added in 1.1.0 that we use, but a few small
workarounds those can be made to work on 1.0.2 as well. These also
were replicated in a number of places and consolidated by this commit.
Signed-off-by: Erik Boasson <eb@ilities.com>
* Increase the matching timeout to 5s (there are some hints the failures
on Travis are timing related)
* Replace the relative timeout in the waitset by a timestamp so that it
gives up after the specified timeout regardless of the number of
events that occur
Signed-off-by: Erik Boasson <eb@ilities.com>
* Compute the time at which the handshake must have completed from the
initial timeout specification, rather than using it as a timeout for
the individual steps of the handshake
* If the handshake fails because an expected message is not present,
print this, including whether the timeout occured because the message
queue was empty or because the expected message could not be found in
a non-empty queue.
* Replace the 0.5s per-step timeout to a single 2s timeout.
Signed-off-by: Erik Boasson <eb@ilities.com>
The memory allocation in deserializing property lists within the crypto
code should not trust the deserialized length and try to allocate that
much memory but should first verify that the length is consistent with
the number of bytes remaining in the input. (Noted by Coverity as use
of tainted data.)
Signed-off-by: Erik Boasson <eb@ilities.com>
Introduced a test that checks for the correct matching behavious for combinations
of the read/write access control settings in the governance xml (enable read/write
access control in the topic rules) and in the permissions xml (the publish/subscribe
grants for a topic).
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Add validate_local_permissions to the set of access control plugin
hooks tests, and add discovery_protection_enabled as an additional
parameter for the access control hook tests.
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Add testing liveness protection to the existing discovery protection
test. The test checks if the P2P_BUILTIN_PARTICIPANT_MESSAGE_SECURE_WRITER
is using the encode_decode_submessage function of the crypto plugin
to secure liveliness messages.
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
A test that checks that the security handshake fails in case of non-matching
encoding settings in the governance xml. All combinations of values for
rtps, discovery and liveliness protection are checked. For meta-data and
payload encoding, this test checks that a reader and writer do not connect
in case of non-matching values.
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Adding a test that checks the effects of using different configuration settings
for discovery protection. This test set uses the cryptography wrapper to count
the number of invocations of the encode and decode functions during a session
and checks the counts with the expected values.
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Introduced a test that checks if all crypto tokens send by a node are
received correctly by the remote node. To support this test, the crypto
wrapper plugin is extended with a token_log mode, that stores all tokens
that are exchanged after the security handshake is finished.
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Add test cases for the join_access_control governance setting and for
the access control plugin check_create_ and check_remote_ hooks,
using a wrapper plugin that simulates failure for each of these,
to test the DDSI integration with the access control plugin.
This commit also contains fixes for:
- an assert on DDS_RETCODE_OK in dds_create_reader and
dds_create_writer that cased the application to terminate in case
creation of a reader or writer is not allowed by security
- do not match a proxy reader that has the 'relay_only' set to
true, which is currently unsupported
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
The initialization of remote participant's key material was not protected
by the remote_participant_crypto lock, which could result in using partially
initialized remote key material. This caused intermittent test failures
with assertions on key_size in crypto_cipher_decrypt_data. This commit fixes
this issue by adding locking for the remote key material.
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
expiry tests and add timestamps to test logging to get more stable
test results on Travis and enable analysing timeing issues.
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Impement the revoke identity callback in ddsi that is called when
the identity certificate of a participant expires. In case the
identity handle that expires is from a local participant, all
proxy pp connections will be dropped for this participant. In case
the identity that expires is from a remote participant, the
corresponding proxy participant is deleted.
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Implement handler for access control on_revoke_permissions. This callback
function disconnects and deletes all proxy participant that are using the
revoked permissions handle (in case of remote permissions expire) and
proxy participant that are connected with a participant for which the
permissions expire (local permissions expire).
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Refactoring security core tests and adding more tests:
- Dynamically generate ca and identity certificates in authentication tests, so that certificate expiry is tested.
Added writing/reading samples to these tests to ensure that nodes can (or cannot) communicate in a specific test case
- Secure communication tests: improved the validation of encryption in wrapper
- Added test for access control plugin settings
- Replaced the in-code test identities (and included ca private keys), added an additional identity
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Interpretation of the c.dsign_algo and c.kagree_algo properties must not
assume the binary property to be a null-terminated string.
Signed-off-by: Erik Boasson <eb@ilities.com>
* Trying not to assume an int is at least 32 bits.
* Technically speaking, comparing "unrelated" addresses is undefined
behaviour which can be avoided by a cast to uintptr_t.
* The early out if either local_crypto == 0 does work in context,
provided the nodes in tree never have local_crypto == 0. That implies
crypto_insert_endpoint_relation must never have a 0 in there, which I
think the callers do respect. Still I think it is better to not hide
these assumptions in the compare function and address the problem in
the lookup function instead.
These changes likely make the code fractionally slower, but I do think
they improve clarity.
Signed-off-by: Erik Boasson <eb@ilities.com>
