Accessing nonexistent elements in sentinel_2pc::controller

[mszulcz-mitre]

Affected Branch

trunk

Basic Diagnostics

• [X] I've pulled the latest changes on the affected branch and the issue is still present.

• [X] The issue is reproducible in docker

Description

In sentinel_2pc::controller, a nonexistent element in the vector m_opts.m_sentinel_endpoints can be accessed if m_sentinel_id > m_opts.m_sentinel_endpoints.size() - 1. The access occurs in Line 51 of src/ush/twophase/sentinel_2pc/controller.cpp:

if(ep == m_opts.m_sentinel_endpoints[m_sentinel_id]) {

and in Line 70:

m_opts.m_sentinel_endpoints[m_sentinel_id]);

Accessing a nonexistent element via the [] operator results in undefined behavior. In my tests, the 1st access (Line 51) doesn't throw an error, but the 2nd access sometimes throws an error. To trigger the nonexistent-element access, you can add this unit test to controller_test.cpp:

TEST_F(sentinel_2pc_test, out_of_range_sentinel_id) {
    constexpr uint32_t bad_sentinel_id = 1; 

    // Add private key for the bad sentinel ID to avoid triggering the error 
    // "No private key specified".
    constexpr auto sentinel_private_key
        = "000000000000000100000000000000000000000000000000000000000000000"
          "1";
    m_opts.m_sentinel_private_keys[bad_sentinel_id]
        = cbdc::hash_from_hex(sentinel_private_key);

    auto ctl = std::make_unique<cbdc::sentinel_2pc::controller>(bad_sentinel_id,
                                                                m_opts,
                                                                m_logger);
    ASSERT_FALSE(ctl->init());
}

Solution

A solution is to check whether sentinel_id is out of range of m_opts.m_sentinel_endpoints in sentinel_2pc::controller::init, and if it is, indicate that initialization failed by returning false. Something like this should work:

if(m_sentinel_id > (m_opts.m_sentinel_endpoints.size()-1)) {
    m_logger->error(“Sentinel ID is out of range of the sentinel endpoints vector”);
    return false;
}

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[mszulcz-mitre]

@HalosGhost As I was replying to your recent comment on Issue #141, I remembered this issue and thought I'd mention that it's similar. As with the bugs in #141, this bug allows an impossible state to be requested for sentinel_2pc::controller. To avoid it, it seems reasonable to do a check in ::init().

[HalosGhost]

It's funny, I had misread them and actually thought they were the same. Thank you for specifically calling my attention to it. And I agree that it's also quite reasonable to fix such things as a check (and subsequent failure) in ::init().