Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Vlad, how about the below version? The branch is the same,
gorcunov/gh-6067-raft-state-verify
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH] raft: more precise verification of incoming request state

When new raft message comes in from the network we need
to be sure that the payload is suitable for processing,
in particular `raft_msg::state` must be valid because
our code logic depends on it.

Since the `raft_msg::state` is declared as enum the
its processing is implementation defined an a compiler
might treat it as unsigned or signed int. In the latter
case the `if` statement won't be taken which will lead
to undefined behaviour. So

1) When packet is decoded lets check the values are
   not trimmed, in particular vote and state shall
   fit uint32_t storage;
2) In case if compiler treats enums as signed integers
   we start using potential underflows via LE operator;
2) Use panic() instead of unreacheable() macro;
3) Extend testing.

Closes #6067

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/xrow.c        | 16 ++++++++++++++--
 src/lib/raft/raft.c   |  6 ++++--
 test/unit/raft.c      | 10 +++++++++-
 test/unit/raft.result |  4 +++-
 4 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/src/box/xrow.c b/src/box/xrow.c
index 16cb2484c..75f5c94af 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1055,6 +1055,7 @@ xrow_decode_raft(const struct xrow_header *row, struct raft_request *r,
 		if (mp_typeof(*pos) != MP_UINT)
 			goto bad_msgpack;
 		uint64_t key = mp_decode_uint(&pos);
+		uint64_t val;
 		switch (key) {
 		case IPROTO_RAFT_TERM:
 			if (mp_typeof(*pos) != MP_UINT)
@@ -1064,12 +1065,17 @@ xrow_decode_raft(const struct xrow_header *row, struct raft_request *r,
 		case IPROTO_RAFT_VOTE:
 			if (mp_typeof(*pos) != MP_UINT)
 				goto bad_msgpack;
-			r->vote = mp_decode_uint(&pos);
+			val = mp_decode_uint(&pos);
+			if (val > UINT_MAX)
+				goto bad_vote;
+			r->vote = val;
 			break;
 		case IPROTO_RAFT_STATE:
 			if (mp_typeof(*pos) != MP_UINT)
 				goto bad_msgpack;
-			r->state = mp_decode_uint(&pos);
+			if (val > UINT_MAX)
+				goto bad_state;
+			r->state = val;
 			break;
 		case IPROTO_RAFT_VCLOCK:
 			r->vclock = vclock;
@@ -1088,6 +1094,12 @@ xrow_decode_raft(const struct xrow_header *row, struct raft_request *r,
 bad_msgpack:
 	xrow_on_decode_err(begin, end, ER_INVALID_MSGPACK, "raft body");
 	return -1;
+bad_vote:
+	xrow_on_decode_err(begin, end, ER_INVALID_MSGPACK, "raft vote overflow");
+	return -1;
+bad_state:
+	xrow_on_decode_err(begin, end, ER_INVALID_MSGPACK, "raft state overflow");
+	return -1;
 }
 
 int
diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
index eacdddb7e..769b1a6ef 100644
--- a/src/lib/raft/raft.c
+++ b/src/lib/raft/raft.c
@@ -309,7 +309,8 @@ raft_process_msg(struct raft *raft, const struct raft_msg *req, uint32_t source)
 	say_info("RAFT: message %s from %u", raft_msg_to_string(req), source);
 	assert(source > 0);
 	assert(source != raft->self);
-	if (req->term == 0 || req->state == 0 || req->state >= raft_state_MAX) {
+
+	if (req->term == 0 || req->state <= 0 || req->state >= raft_state_MAX) {
 		diag_set(RaftError, "Invalid term or state");
 		return -1;
 	}
@@ -406,7 +407,8 @@ raft_process_msg(struct raft *raft, const struct raft_msg *req, uint32_t source)
 			raft_sm_become_leader(raft);
 			break;
 		default:
-			unreachable();
+			panic("RAFT: unreacheable state hit");
+			break;
 		}
 	}
 	if (req->state != RAFT_STATE_LEADER) {
diff --git a/test/unit/raft.c b/test/unit/raft.c
index 6369c42d3..b9bc49b78 100644
--- a/test/unit/raft.c
+++ b/test/unit/raft.c
@@ -247,7 +247,7 @@ raft_test_recovery(void)
 static void
 raft_test_bad_msg(void)
 {
-	raft_start_test(9);
+	raft_start_test(11);
 	struct raft_msg msg;
 	struct raft_node node;
 	struct vclock vclock;
@@ -294,6 +294,14 @@ raft_test_bad_msg(void)
 	is(raft_node_process_msg(&node, &msg, 2), -1, "bad state");
 	is(node.raft.term, 1, "term from the bad message wasn't used");
 
+	msg = (struct raft_msg){
+		.state = -1,
+		.term = 10,
+		.vote = 2,
+	};
+	is(raft_node_process_msg(&node, &msg, 2), -1, "bad negative state");
+	is(node.raft.term, 1, "term from the bad message wasn't used");
+
 	raft_node_destroy(&node);
 	raft_finish_test();
 }
diff --git a/test/unit/raft.result b/test/unit/raft.result
index 598a7e760..f89cd1658 100644
--- a/test/unit/raft.result
+++ b/test/unit/raft.result
@@ -45,7 +45,7 @@ ok 1 - subtests
 ok 2 - subtests
 	*** raft_test_recovery: done ***
 	*** raft_test_bad_msg ***
-    1..9
+    1..11
     ok 1 - state can't be 0
     ok 2 - term from the bad message wasn't used
     ok 3 - node can't be a candidate but vote for another node
@@ -55,6 +55,8 @@ ok 2 - subtests
     ok 7 - term can't be 0
     ok 8 - bad state
     ok 9 - term from the bad message wasn't used
+    ok 10 - bad negative state
+    ok 11 - term from the bad message wasn't used
 ok 3 - subtests
 	*** raft_test_bad_msg: done ***
 	*** raft_test_vote ***
-- 
2.31.1