ecdh (Botan): bounds-check public point length before reading

[metsw24-max]

Bug

pgp::ecdh::load_public_key() in src/lib/crypto/ecdh.cpp (Botan backend) only verifies that key.p is non-empty and that its first byte is 0x04 (the SEC1 uncompressed-point prefix) before computing pointers into key.p for the two coordinates:

https://github.com/rnpgp/rnp/blob/main/src/lib/crypto/ecdh.cpp#L96-L103

if (!key.p.size() || (key.p[0] != 0x04)) {
    RNP_LOG("Failed to load public key");
    return false;
}

const size_t curve_order = curve->bytes();
rnp::bn      px(&key.p[1], curve_order);
rnp::bn      py(&key.p[1 + curve_order], curve_order);

rnp::bn(const uint8_t *val, size_t len) calls botan_mp_from_bin(bn_, val, len), which reads len bytes starting at val. For any non-25519 curve (P-256, P-384, P-521, brainpoolP{256,384,512}r1, secp256k1) an input whose key.p is shorter than 2 * curve_order + 1 bytes — in the worst case just the single byte {0x04} — causes both botan_mp_from_bin() calls to read past the end of the std::vector<uint8_t> storage backing key.p (heap out-of-bounds read of up to 64 bytes for P-256, more for larger curves).

Reproduction / observation

key.p is populated directly from MPI bytes inside ECKeyMaterial::parse() in src/lib/key_material.cpp (via pgp_packet_body_t::get(pgp::mpi&)), which only enforces an upper bound (PGP_MPINT_SIZE) and that the MPI is non-empty. An attacker can therefore deliver a public ECDH key packet whose p MPI is a single 0x04 byte. ECDHKeyMaterial::validate_material -> ecdh::validate_key -> load_public_key then performs the OOB read above before any other check rejects the malformed key.

The two sibling implementations in the same directory already reject this exact case explicitly:

• src/lib/crypto/ecdsa.cpp lines 48-51:

  const size_t curve_order = curve->bytes();
  if (keydata.p.size() != 2 * curve_order + 1) {
      return false;
  }

• src/lib/crypto/sm2.cpp line 48:

  if (!sz || (sz != (2 * sign_half_len + 1)) || (keydata.p[0] != 0x04)) {
      return false;
  }

so the ECDH backend was the outlier. The OSSL ECDH backend (src/lib/crypto/ecdh_ossl.cpp) is unaffected because it delegates to EC_POINT_oct2point(), which validates the encoded length internally.

Fix

Add the same key.p.size() != 2 * curve_order + 1 length check in load_public_key() before constructing the two rnp::bn coordinates. The check is local to the callee, mirrors the established pattern in ecdsa.cpp/sm2.cpp, and changes no public API or write paths (own-key generation always produces correctly-sized p).

Build evidence

Configured and built with the Botan 3.12.0 backend:

cmake -DCRYPTO_BACKEND=botan3 -DBUILD_TESTING=OFF -DDOWNLOAD_GTEST=OFF -DENABLE_SM2=OFF ..
cmake --build . --target librnp
...
[100%] Linking CXX static library librnp.a
[100%] Built target librnp

No other source files are modified.

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.45%. Comparing base (6f8a677) to head (deec39b).

Files with missing lines	Patch %	Lines
src/lib/crypto/ecdh.cpp	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2390      +/-   ##
==========================================
- Coverage   85.46%   85.45%   -0.01%     
==========================================
  Files         126      126              
  Lines       22711    22714       +3     
==========================================
+ Hits        19409    19410       +1     
- Misses       3302     3304       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.