Reports
MEDIA[OTROS]#3547349

Monero: un ternario invertido en peerlist_manager::filter() deja a un host llenar la lista blanca de nodos con distintos puertos

La función filter() del gestor de nodos de Monero limpiaba la lista gris cuando debía limpiar la blanca. Así, una sola IP podía tener entradas ilimitadas en la lista blanca usando puertos distintos.

Resumen
Resumen en castellano de un reporte público, no una traducción literal. El código y los comandos se mantienen como en el original.

Resumen

El nodo de Monero mantiene dos listas de pares en la red P2P: la lista blanca (nodos con los que ya ha conectado y que considera fiables) y la lista gris (direcciones conocidas pero sin verificar). El código intenta que cada host aparezca una sola vez, y para quitar las entradas sobrantes usa peerlist_manager::filter(), definida en src/p2p/net_peerlist.h.

El problema está en una sola línea. Al elegir sobre qué lista trabajar, el operador ternario tiene las ramas al revés:

peers_indexed::index<by_addr>::type& sorted_index = white ? m_peers_gray.get<by_addr>() : m_peers_white.get<by_addr>();

Cuando se llama con white=true, la función recorre y borra en la lista gris, y viceversa. Basta compararla con peerlist_manager::foreach(), del mismo fichero, que lo hace bien:

peers_indexed::index<by_time>::type& by_time_index = white ? m_peers_white.get<by_time>() : m_peers_gray.get<by_time>();

La función completa afectada es esta:

template<typename F> size_t peerlist_manager::filter(bool white, const F &f)
  {
    size_t filtered = 0;
    TRY_ENTRY();
    CRITICAL_REGION_LOCAL(m_peerlist_lock);
    peers_indexed::index<by_addr>::type& sorted_index = white ? m_peers_gray.get<by_addr>() : m_peers_white.get<by_addr>();
    auto i = sorted_index.begin();
    while (i != sorted_index.end())
    {
      if (f(*i))
      {
        i = sorted_index.erase(i);
        ++filtered;
      }
      else
        ++i;
    }
    CATCH_ENTRY_L0("peerlist_manager::filter()", filtered);
    return filtered;
  }

Las consecuencias son dos: la lista blanca nunca se depura por host, de modo que una misma IP puede acumular tantas entradas como puertos distintos anuncie, y a la vez se borran de la lista gris entradas que no debían tocarse. El fallo estaba en la rama master (commit f99ee72d4b5065ddcaba2b74fcc6c8dc9be385dc).

Pasos de reproducción

El investigador lo demostró con una prueba unitaria compilada dentro de un contenedor Docker.

  1. Añadir esta prueba a tests/unit_tests/test_peerlist.cpp:
// XXXKE peerlist_manager::filter() has inverted ternary
TEST(peer_list, filter_inverted_ternary_bug)
{
  nodetool::peerlist_manager plm;
  plm.init(nodetool::peerlist_types{}, false);

  {
    nodetool::peerlist_entry ple{};
    ple.adr = MAKE_IPV4_ADDRESS(93,184,216,34, 18080);
    ple.id = 9999;
    ple.last_seen = 1000;
    plm.append_with_peer_gray(ple);
  }
  ASSERT_EQ(plm.get_gray_peers_count(), 1u);

  {
    nodetool::peerlist_entry ple{};
    ple.adr = MAKE_IPV4_ADDRESS(45,33,32,1, 28080);
    ple.id = 8888;
    ple.last_seen = 1000;
    plm.append_with_peer_gray(ple);
  }
  ASSERT_EQ(plm.get_gray_peers_count(), 2u);

  {
    nodetool::peerlist_entry ple{};
    ple.adr = MAKE_IPV4_ADDRESS(45,33,32,1, 18080);
    ple.id = 1001;
    ple.last_seen = 2000;
    plm.append_with_peer_white(ple);
  }
  std::cout << "[Step 1] After adding 45.33.32.1:18080 to white:" << std::endl;
  std::cout << "  white_count = " << plm.get_white_peers_count()
            << " (expect 1)" << std::endl;
  std::cout << "  gray_count  = " << plm.get_gray_peers_count()
            <<  std::endl;

  EXPECT_EQ(plm.get_white_peers_count(), 1u);
  EXPECT_EQ(plm.get_gray_peers_count(), 2u)
      << "filter(true,...) evicted from gray list instead of white list. "
         "Gray entry 45.33.32.1:28080 was wrongly removed.";

  {
    nodetool::peerlist_entry ple{};
    ple.adr = MAKE_IPV4_ADDRESS(45,33,32,1, 18081);
    ple.id = 1002;
    ple.last_seen = 3000;
    plm.append_with_peer_white(ple);
  }
  std::cout << "[Step 2] After adding 45.33.32.1:18081 to white:" << std::endl;
  std::cout << "  white_count = " << plm.get_white_peers_count()
            << " (expect 1 if fixed, 2 if buggy)" << std::endl;
  std::cout << "  gray_count  = " << plm.get_gray_peers_count() << std::endl;

  EXPECT_EQ(plm.get_white_peers_count(), 1u)
      << "Same host (45.33.32.1) has multiple white list entries "
         "on different ports. evict_host_from_peerlist did not clean white list. ";

  nodetool::peerlist_manager plm2;
  plm2.init(nodetool::peerlist_types{}, false);

  for (int i = 1; i <= 3; i++)
  {
    nodetool::peerlist_entry ple{};
    ple.adr = MAKE_IPV4_ADDRESS(104,16,0,i, 18080);
    ple.id = 2000 + i;
    ple.last_seen = 1000 + i;
    plm2.append_with_peer_white(ple);
  }
  ASSERT_EQ(plm2.get_white_peers_count(), 3u);

  for (int port = 0; port < 20; port++)
  {
    nodetool::peerlist_entry ple{};
    ple.adr = MAKE_IPV4_ADDRESS(45,33,32,1, 18080 + port);
    ple.id = 5000 + port;
    ple.last_seen = 5000 + port;
    plm2.append_with_peer_white(ple);
  }

  std::cout << "After 3 honest peers + 20 attacker ports on 45.33.32.1:" << std::endl;
  std::cout << "  white_count = " << plm2.get_white_peers_count()
            << "" << std::endl;

  std::set<std::string> unique_hosts;
  size_t attacker_entries = 0;
  plm2.foreach(true, [&](const nodetool::peerlist_entry &pe) {
    unique_hosts.insert(pe.adr.host_str());
    if (pe.adr.host_str() == "45.33.32.1")
      attacker_entries++;
    return true;
  });

  std::cout << "  unique_hosts     = " << unique_hosts.size() << std::endl;
  std::cout << "  attacker_entries = " << attacker_entries
            << "" << std::endl;

}

La prueba hace tres cosas: mete en la lista gris dos direcciones (una de ellas del host 45.33.32.1 en el puerto 28080), añade ese mismo host a la lista blanca y comprueba qué lista se ha depurado; añade el host otra vez a la blanca con otro puerto; y, en un gestor nuevo, carga 3 nodos legítimos más 20 puertos del mismo host.

  1. Guardar este fichero como Dockerfile.test-peerlist:
FROM ubuntu:22.04 AS builder

RUN set -ex && \
    apt-get update && \
    DEBIAN_FRONTEND=noninteractive apt-get --no-install-recommends --yes install \
        build-essential \
        ca-certificates \
        cmake \
        git \
        pkg-config \
        libboost-all-dev \
        libssl-dev \
        libzmq3-dev \
        libunbound-dev \
        libsodium-dev \
        libreadline-dev \
        libexpat1-dev \
        libunwind-dev \
        liblzma-dev \
        libhidapi-dev \
        libusb-1.0-0-dev \
        libprotobuf-dev \
        protobuf-compiler \
        libgtest-dev

WORKDIR /src
COPY . .

RUN set -ex && \
    git submodule init && git submodule update && \
    mkdir -p build/release && cd build/release && \
    cmake -D CMAKE_BUILD_TYPE=Release -D BUILD_TESTS=ON ../.. && \
    make -j$(nproc) unit_tests

# Run only the peerlist filter bug test
CMD ["./build/release/tests/unit_tests/unit_tests", "--gtest_filter=peer_list.filter_inverted_ternary_bug"]
  1. Desde el directorio del código fuente de Monero, construir la imagen y ejecutarla. Con el código vulnerable, la prueba falla en los dos puntos clave:
$ docker build -f Dockerfile.test-peerlist -t monero-peerlist-test .
$  docker run --rm monero-peerlist-test

Note: Google Test filter = peer_list.filter_inverted_ternary_bug
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from peer_list
[ RUN      ] peer_list.filter_inverted_ternary_bug
[Step 1] After adding 45.33.32.1:18080 to white:
  white_count = 1 (expect 1)
  gray_count  = 1
/src/tests/unit_tests/test_peerlist.cpp:113: Failure
Expected equality of these values:
  plm.get_gray_peers_count()
    Which is: 1
  2u
    Which is: 2
filter(true,...) evicted from gray list instead of white list. Gray entry 45.33.32.1:28080 was wrongly removed.

[Step 2] After adding 45.33.32.1:18081 to white:
  white_count = 2 (expect 1 if fixed, 2 if buggy)
  gray_count  = 1
/src/tests/unit_tests/test_peerlist.cpp:129: Failure
Expected equality of these values:
  plm.get_white_peers_count()
    Which is: 2
  1u
    Which is: 1
Same host (45.33.32.1) has multiple white list entries on different ports. evict_host_from_peerlist did not clean white list.

After 3 honest peers + 20 attacker ports on 45.33.32.1:
  white_count = 23
  unique_hosts     = 4
  attacker_entries = 20
[  FAILED  ] peer_list.filter_inverted_ternary_bug (0 ms)
[----------] 1 test from peer_list (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] peer_list.filter_inverted_ternary_bug

En el primer paso desaparece de la lista gris la entrada 45.33.32.1:28080, que no debía tocarse. En el segundo, el mismo host queda dos veces en la lista blanca. Y en la última parte, de 23 entradas blancas, 20 pertenecen a una única IP: solo hay 4 hosts distintos.

Impacto

Un único host puede ocupar un número ilimitado de huecos en la lista blanca de un nodo simplemente presentándose desde puertos diferentes. En la prueba, una IP acaba con 20 de las 23 entradas, frente a 3 nodos legítimos. La lista blanca es la que el nodo considera de confianza, así que su diversidad de pares queda reducida a favor de quien controle esa IP. Además, el filtro borra por error entradas de la lista gris.

Remediación

El original no detalla la corrección aplicada. La forma natural de arreglarlo es invertir las ramas del ternario en filter() para que, como en foreach(), white=true opere sobre m_peers_white. El reporte consta como resuelto por el programa.

Qué aprender de este caso

  • Cuando una función recibe un booleano que elige entre dos estructuras gemelas (white ? A : B), compárala con sus hermanas del mismo fichero: aquí foreach() y filter() usaban el mismo patrón con las ramas en orden opuesto, y esa diferencia ya delataba el error.
  • En código P2P, revisa si los límites "uno por host" se aplican por IP o por pareja IP:puerto. Si el borrado de duplicados falla en silencio, un atacante solo necesita abrir más puertos para multiplicar su presencia.
  • Un test que inserte el mismo host con dos puertos y compruebe el contador de cada lista por separado habría detectado el fallo; probar solo el valor de retorno de filter() no basta, porque borraba entradas igualmente, solo que en la lista equivocada.
  • Para evitar este tipo de confusión, mejor un enum o dos funciones con nombre explícito (filter_white(), filter_gray()) que un parámetro bool cuyo significado hay que recordar en cada ternario.