Skip to content

tor-keymgr: Fix bug in ArtiNativeKeystore::contains

gabi-250 requested to merge gabi-250/arti:fix-keystore-contains into main

This fixes a bug in ArtiNativeKeystore's Keystore::contains() implementation: previously, it called Path::exists() on a relative path (built by concatenating the key specifier and the extension), so unless your current directory happened to be the root of the keystore, contains() would always return false.

KeyMgr::generate uses Keystore::contains() under the hood, so it was affected by this bug too: if called overwrite = false, it would misbehave and overwrite any existing keys.

Internally, we call KeyMgr::generate in a couple of places:

  • tor-hsservice/src/lib.rs, to generate the hsid if it doesn't already exist. This callsite is not affected by the bug, because KeyMgr::generate is only called if KeyMgr::get returns None
  • tor-hsservice/src/ipt_mgr.rs, to generate KS_hss_ntor and KS_hs_ipt_sid keys for intro point establishment. This callsite is also unaffected (because it too calls get() before attempting to generate())

The bug affects any downstream users that use KeyMgr::generate with a key manager backed by ArtiNativeKeystore.


KeyMgr::get_or_generate is not affected, even though it calls Keymgr::generate (it performs a separate extra check before calling generate()). (Both suffer from a known TOCTOU race, but that's a separate matter.) As an aside, I'd like to somehow unify KeyMgr::get_or_generate and KeyMgr::get (I've had some attempts in the past but ended up abandoning them because the result was more unergonomic than the existing APIs).

Closes #1492 (closed)

Edited by gabi-250

Merge request reports