niklas/cyclonedds
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Do not rewrite secure messages in retransmit queue (#456)

Browse source 
* Do not rewrite secure messages in retransmit queue

Messages to be retransmitted spend some time on a transmit queue, and
are subject to the rewriting of the destination information to reduce
the number of outgoing copies.  Self-evidently, altering the message
header does not sit well with encryption and/or authentication of
messages.

The way the rewriting works is that the offset of the "reader entity id"
in the DATA submessage is saved on message construction (the GUID prefix
is at a fixed location), so that it can be read and possibly zero'd out
later.  The crypto transformations move the message around and it so
happens that it can end up pointing to the key id in the encoded
message.  Zeroing that one out leads to uninterpretable messages.

This commit adds a message/event kind to distinguish between retransmit
that may and retransmit that may not be merged (and thus rewritten) and
gets used when the crypto plugin is invoked to transform a message.

Signed-off-by: Erik Boasson <eb@ilities.com>

* Update comment on changing REXMIT to REXMIT_NOMERGE

Signed-off-by: Erik Boasson <eb@ilities.com>
This commit is contained in:
eboasson 2020-03-30 10:32:58 +02:00 • committed by GitHub
parent c8d8d2f8e6
commit b18fd395d3
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 43 additions and 8 deletions
Download patch file Download diff file Expand all files Collapse all files

3
src/core/ddsi/include/dds/ddsi/q_xmsg.h
View file

@ -43,7 +43,8 @@ struct nn_xmsg_marker {
enum nn_xmsg_kind {
NN_XMSG_KIND_CONTROL,
NN_XMSG_KIND_DATA,
NN_XMSG_KIND_DATA_REXMIT
NN_XMSG_KIND_DATA_REXMIT,
NN_XMSG_KIND_DATA_REXMIT_NOMERGE
};
/* XMSGPOOL */

29
src/core/ddsi/src/q_xevent.c
View file

@ -103,6 +103,7 @@ enum xeventkind_nt
{
XEVK_MSG,
XEVK_MSG_REXMIT,
XEVK_MSG_REXMIT_NOMERGE,
XEVK_ENTITYID,
XEVK_NT_CALLBACK
};
@ -126,7 +127,7 @@ struct xevent_nt
struct nn_xmsg *msg;
size_t queued_rexmit_bytes;
ddsrt_avl_node_t msg_avlnode;
} msg_rexmit;
} msg_rexmit; /* and msg_rexmit_nomerge */
struct {
/* xmsg is self-contained / relies on reference counts */
struct nn_xmsg *msg;
@ -178,7 +179,7 @@ static void update_rexmit_counts (struct xeventq *evq, struct xevent_nt *ev)
#if 0
EVQTRACE ("ZZZ(%p,%"PRIuSIZE")", (void *) ev, ev->u.msg_rexmit.queued_rexmit_bytes);
#endif
assert (ev->kind == XEVK_MSG_REXMIT);
assert (ev->kind == XEVK_MSG_REXMIT || ev->kind == XEVK_MSG_REXMIT_NOMERGE);
assert (ev->u.msg_rexmit.queued_rexmit_bytes <= evq->queued_rexmit_bytes);
assert (evq->queued_rexmit_msgs > 0);
evq->queued_rexmit_bytes -= ev->u.msg_rexmit.queued_rexmit_bytes;
@ -205,9 +206,20 @@ static void trace_msg (UNUSED_ARG (struct xeventq *evq), UNUSED_ARG (const char
static struct xevent_nt *lookup_msg (struct xeventq *evq, struct nn_xmsg *msg)
{
assert (nn_xmsg_kind (msg) == NN_XMSG_KIND_DATA_REXMIT);
trace_msg (evq, "lookup-msg", msg);
return ddsrt_avl_lookup (&msg_xevents_treedef, &evq->msg_xevents, msg);
switch (nn_xmsg_kind (msg))
{
case NN_XMSG_KIND_CONTROL:
case NN_XMSG_KIND_DATA:
assert (0);
return NULL;
case NN_XMSG_KIND_DATA_REXMIT:
trace_msg (evq, "lookup-msg", msg);
return ddsrt_avl_lookup (&msg_xevents_treedef, &evq->msg_xevents, msg);
case NN_XMSG_KIND_DATA_REXMIT_NOMERGE:
return NULL;
}
assert (0);
return NULL;
}
static void remember_msg (struct xeventq *evq, struct xevent_nt *ev)
@ -1324,6 +1336,7 @@ static void handle_individual_xevent_nt (struct xevent_nt *xev, struct nn_xpack
handle_xevk_msg (xp, xev);
break;
case XEVK_MSG_REXMIT:
case XEVK_MSG_REXMIT_NOMERGE:
handle_xevk_msg_rexmit (xp, xev);
break;
case XEVK_ENTITYID:
@ -1563,7 +1576,7 @@ int qxev_msg_rexmit_wrlock_held (struct xeventq *evq, struct nn_xmsg *msg, int f
struct xevent_nt *ev;
assert (evq);
assert (nn_xmsg_kind (msg) == NN_XMSG_KIND_DATA_REXMIT);
assert (nn_xmsg_kind (msg) == NN_XMSG_KIND_DATA_REXMIT || nn_xmsg_kind (msg) == NN_XMSG_KIND_DATA_REXMIT_NOMERGE);
ddsrt_mutex_lock (&evq->lock);
if ((ev = lookup_msg (evq, msg)) != NULL && nn_xmsg_merge_rexmit_destinations_wrlock_held (gv, ev->u.msg_rexmit.msg, msg))
{
@ -1587,7 +1600,9 @@ int qxev_msg_rexmit_wrlock_held (struct xeventq *evq, struct nn_xmsg *msg, int f
}
else
{
ev = qxev_common_nt (evq, XEVK_MSG_REXMIT);
const enum xeventkind_nt kind =
(nn_xmsg_kind (msg) == NN_XMSG_KIND_DATA_REXMIT) ? XEVK_MSG_REXMIT : XEVK_MSG_REXMIT_NOMERGE;
ev = qxev_common_nt (evq, kind);
ev->u.msg_rexmit.msg = msg;
ev->u.msg_rexmit.queued_rexmit_bytes = msg_size;
evq->queued_rexmit_bytes += msg_size;

19
src/core/ddsi/src/q_xmsg.c
View file

@ -429,6 +429,7 @@ static int submsg_is_compatible (const struct nn_xmsg *msg, SubmessageKind_t smk
break;
case NN_XMSG_KIND_DATA:
case NN_XMSG_KIND_DATA_REXMIT:
case NN_XMSG_KIND_DATA_REXMIT_NOMERGE:
switch (smkind)
{
case SMID_PAD:
@ -551,6 +552,12 @@ void nn_xmsg_submsg_remove(struct nn_xmsg *msg, struct nn_xmsg_marker sm_marker)
{
/* Just reset the message size to the start of the current sub-message. */
msg->sz = sm_marker.offset;
/* Deleting the submessage means the readerId offset in a DATA_REXMIT message is no
longer valid. Converting the message kind to a _NOMERGE one ensures no subsequent
operation will assume its validity. */
if (msg->kind == NN_XMSG_KIND_DATA_REXMIT)
msg->kind = NN_XMSG_KIND_DATA_REXMIT_NOMERGE;
}
void nn_xmsg_submsg_replace(struct nn_xmsg *msg, struct nn_xmsg_marker sm_marker, unsigned char *new_submsg, size_t new_len)
@ -573,6 +580,17 @@ void nn_xmsg_submsg_replace(struct nn_xmsg *msg, struct nn_xmsg_marker sm_marker
/* Replace the sub-message. */
memcpy(msg->data->payload + sm_marker.offset, new_submsg, new_len);
/* The replacement submessage may have undergone any transformation and so the readerId
offset in a DATA_REXMIT message is potentially no longer valid. Converting the
message kind to a _NOMERGE one ensures no subsequent operation will assume its
validity. This is used by the security implementation when encrypting and/or signing
messages and apart from the offset possibly no longer being valid (for which one
might conceivably be able to correct), there is also the issue that it may now be
meaningless junk or that rewriting it would make the receiver reject it as having
been tampered with. */
if (msg->kind == NN_XMSG_KIND_DATA_REXMIT)
msg->kind = NN_XMSG_KIND_DATA_REXMIT_NOMERGE;
}
void nn_xmsg_submsg_append_refd_payload(struct nn_xmsg *msg, struct nn_xmsg_marker sm_marker)
@ -1614,6 +1632,7 @@ int nn_xpack_addmsg (struct nn_xpack *xp, struct nn_xmsg *m, const uint32_t flag
break;
case NN_XMSG_KIND_DATA:
case NN_XMSG_KIND_DATA_REXMIT:
case NN_XMSG_KIND_DATA_REXMIT_NOMERGE:
GVTRACE ("%s("PGUIDFMT":#%"PRId64"/%u)",
(m->kind == NN_XMSG_KIND_DATA) ? "data" : "rexmit",
PGUID (m->kindspecific.data.wrguid),