From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 625A06EC40; Fri, 2 Jul 2021 16:43:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 625A06EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1625233395; bh=6zqCvv0EVTRvCVec+3JFZeqJAHEmZY3S02wVji1zQu0=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=TmsO9OG+sdKRN/cXFeR3mUzA3l6vspIP0+tfNV8T+emeeF5zxOyeCGYyefBvuSYBH rDjZzxq3MDNawMV8HhsKaEpQGFu4VHNxqCDi+CcHrWHVr9CS35MRQsT6t8SVCCSGxX oi1yOuXPztDM3+vkd2MKjSRRhEaYArdd6lepdR78= Received: from mail-lj1-f177.google.com (mail-lj1-f177.google.com [209.85.208.177]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1019E6EC40 for ; Fri, 2 Jul 2021 16:43:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1019E6EC40 Received: by mail-lj1-f177.google.com with SMTP id x20so13394785ljc.5 for ; Fri, 02 Jul 2021 06:43:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=R/StZ3gtNU8eSlV6n4YvP13oLTO1wr4SxThLYHL1a90=; b=eFLG66ybcjPwFBaU+kPzzOUnZ+vyKlwLdzQhNMZkkS8LsYKexHj5JvR96TxKBRVPKR MzIjd/4PcAToo11odydhfokMhUnvvRZMlOUNeZrf4T/KAnhQrzAfF/4rHeT7BvUeMi0+ 1voWbhn0qzabpjNOGZgZrGyVehryOKYVGQfsMIUxpuYDLj1LKnHawB3IZZbuu7wezuYd oeqvjSYVAdxUUPfJ3dBAQ6VfT6DGaDzWCAS24p9wQM5oXDbm6VNU0B/4fkycSVPeVBkY +PbwELPFpOz/GB3SXhIYkMvoIdEKqLyQRNhZVvwRPSjN468mfX0QXJWadA6xzsjzsFNL Rd7A== X-Gm-Message-State: AOAM533mXMPOGg8AR9CpM11V3UuYJhVrFwhIF7Pq71PPmnS0wmwIOGbE MsE0JS3mVX3tJuMTg6VBSXIj+5M1WLw= X-Google-Smtp-Source: ABdhPJy+ISIxFL1phJEv+YoNgr4MX1U+CHH/CHTAqyK1MhO6UZBqxRl2NSzrzQixdRy2i2lMvHoKLA== X-Received: by 2002:a2e:300e:: with SMTP id w14mr3950129ljw.147.1625233392892; Fri, 02 Jul 2021 06:43:12 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id e20sm349027ljk.67.2021.07.02.06.43.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Jul 2021 06:43:11 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id ED9BA5A001F; Fri, 2 Jul 2021 16:43:10 +0300 (MSK) Date: Fri, 2 Jul 2021 16:43:10 +0300 To: Vladislav Shpilevoy Cc: tml Message-ID: References: <20210625100707.87807-1-gorcunov@gmail.com> <9652278e-570e-40e5-b2d1-856fe58179fc@tarantool.org> <4b0f4d8f-4e0c-00d0-38ad-0b6abad3aafe@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4b0f4d8f-4e0c-00d0-38ad-0b6abad3aafe@tarantool.org> User-Agent: Mutt/2.0.7 (2021-05-04) Subject: Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Vlad, how about the below version? The branch is the same, gorcunov/gh-6067-raft-state-verify --- From: Cyrill Gorcunov 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 --- 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