Commit c16412e3 authored by Nick Mathewson's avatar Nick Mathewson 🎨
Browse files

Detect consensuses that are signed by the wrong authorities.

With this patch we don't consider a consensus to be even potentially
well-signed if the authorities that are listed as signing it don't
contain enough authorities we believe in.  Otherwise, we'd just try
fetching certs for them and failing forever.

(I found this by switching from a chutney network to the main
network without cleaning out my cache.)

Closes #44
parent 5916adac
...@@ -22,6 +22,10 @@ impl Authority { ...@@ -22,6 +22,10 @@ impl Authority {
pub fn new(name: String, v3ident: RSAIdentity) -> Self { pub fn new(name: String, v3ident: RSAIdentity) -> Self {
Authority { name, v3ident } Authority { name, v3ident }
} }
/// Return the v3 identity key of this certificate.
pub fn v3ident(&self) -> &RSAIdentity {
&self.v3ident
}
/// Return true if this authority matches a given certificate. /// Return true if this authority matches a given certificate.
pub fn matches_cert(&self, cert: &AuthCert) -> bool { pub fn matches_cert(&self, cert: &AuthCert) -> bool {
&self.v3ident == cert.id_fingerprint() &self.v3ident == cert.id_fingerprint()
......
...@@ -433,6 +433,7 @@ impl NoInformation { ...@@ -433,6 +433,7 @@ impl NoInformation {
None => return Ok(NextState::SameState(self)), None => return Ok(NextState::SameState(self)),
} }
}; };
let (consensus_meta, unvalidated) = { let (consensus_meta, unvalidated) = {
let string = consensus_text.as_str()?; let string = consensus_text.as_str()?;
let (signedval, remainder, parsed) = MDConsensus::parse(string)?; let (signedval, remainder, parsed) = MDConsensus::parse(string)?;
...@@ -444,6 +445,24 @@ impl NoInformation { ...@@ -444,6 +445,24 @@ impl NoInformation {
return Ok(NextState::SameState(self)); return Ok(NextState::SameState(self));
} }
}; };
// Make sure that we could in principle validate this
// consensus. If we couldn't, pretend it isn't cached at all.
let authority_ids: Vec<_> = config
.authorities()
.iter()
.map(|auth| auth.v3ident())
.collect();
if !unvalidated.authorities_are_correct(&authority_ids[..]) {
// This is not for us. Treat it as if we had no cached consensus.
// XXXX-A1: We should mark the cached as invalid somehow, or just
// remove it.
warn!("Found cached directory not signed by the right authorities. Are you using a mismatched cache?");
store.lock().await.delete_consensus(&consensus_meta)?;
return Ok(NextState::SameState(self));
}
let n_authorities = config.authorities().len() as u16; let n_authorities = config.authorities().len() as u16;
let unvalidated = unvalidated.set_n_authorities(n_authorities); let unvalidated = unvalidated.set_n_authorities(n_authorities);
Ok(NextState::NewState(UnvalidatedDir { Ok(NextState::NewState(UnvalidatedDir {
...@@ -541,12 +560,25 @@ impl NoInformation { ...@@ -541,12 +560,25 @@ impl NoInformation {
let unvalidated = parsed.check_valid_now()?; let unvalidated = parsed.check_valid_now()?;
let meta = ConsensusMeta::from_unvalidated(signedval, remainder, &unvalidated); let meta = ConsensusMeta::from_unvalidated(signedval, remainder, &unvalidated);
// Make sure that we could in principle validate this, and it doesn't
// have a totally different set of authorities than the ones
// we asked for.
let authority_ids: Vec<_> = config
.authorities()
.iter()
.map(|auth| auth.v3ident())
.collect();
if !unvalidated.authorities_are_correct(&authority_ids[..]) {
return Err(Error::Unwanted("Consensus not signed by correct authorities").into());
}
{ {
let mut w = store.lock().await; let mut w = store.lock().await;
w.store_consensus(&meta, true, &text)?; w.store_consensus(&meta, true, &text)?;
} }
let n_authorities = config.authorities().len() as u16; let n_authorities = config.authorities().len() as u16;
let unvalidated = unvalidated.set_n_authorities(n_authorities); let unvalidated = unvalidated.set_n_authorities(n_authorities);
Ok(UnvalidatedDir { Ok(UnvalidatedDir {
from_cache: false, from_cache: false,
consensus: unvalidated, consensus: unvalidated,
......
...@@ -271,20 +271,8 @@ impl SqliteStore { ...@@ -271,20 +271,8 @@ impl SqliteStore {
Ok(()) Ok(())
} }
/// Return the information about the latest non non-pending consensus, /// Return the information about the latest non-pending consensus,
/// including its valid-after time and digest. /// including its valid-after time and digest.
// TODO: XXXX-A1 Take a pending argument?
//
// WAIT HANG ON: XXXX-A1. This whole function is troubling and is
// used in troubling ways. It assumes that any non-pending
// consensus we find will be valid when checked against our
// current authorities. But what if we choose a different set of
// authorities?
//
// As implemented, this means that when switching from mainline
// Tor to/from chutney or a testnet, we might not actually fetch
// the real consensus that we want because we still have one from
// the old network that hasn't expired.
pub fn latest_consensus_meta(&self) -> Result<Option<ConsensusMeta>> { pub fn latest_consensus_meta(&self) -> Result<Option<ConsensusMeta>> {
let mut stmt = self.conn.prepare(FIND_LATEST_CONSENSUS_META)?; let mut stmt = self.conn.prepare(FIND_LATEST_CONSENSUS_META)?;
let mut rows = stmt.query(NO_PARAMS)?; let mut rows = stmt.query(NO_PARAMS)?;
...@@ -358,6 +346,20 @@ impl SqliteStore { ...@@ -358,6 +346,20 @@ impl SqliteStore {
Ok(()) Ok(())
} }
/// Remove the consensus generated from `cmeta`.
pub fn delete_consensus(&mut self, cmeta: &ConsensusMeta) -> Result<()> {
let d = hex::encode(cmeta.sha3_256_of_whole());
let digest = format!("sha3-256-{}", d);
// TODO: We should probably remove the blob as well, but for now
// this is enough.
let tx = self.conn.transaction()?;
tx.execute(REMOVE_CONSENSUS, params![digest])?;
tx.commit()?;
Ok(())
}
/// Save a list of authority certificates to the cache. /// Save a list of authority certificates to the cache.
pub fn store_authcerts(&mut self, certs: &[(AuthCertMeta, &str)]) -> Result<()> { pub fn store_authcerts(&mut self, certs: &[(AuthCertMeta, &str)]) -> Result<()> {
let tx = self.conn.transaction()?; let tx = self.conn.transaction()?;
...@@ -621,6 +623,12 @@ const MARK_CONSENSUS_NON_PENDING: &str = " ...@@ -621,6 +623,12 @@ const MARK_CONSENSUS_NON_PENDING: &str = "
WHERE digest = ?; WHERE digest = ?;
"; ";
/// Query: Remove the consensus with a given digest field.
const REMOVE_CONSENSUS: &str = "
DELETE FROM Consensuses
WHERE digest = ?;
";
/// Query: Find the authority certificate with given key digests. /// Query: Find the authority certificate with given key digests.
const FIND_AUTHCERT: &str = " const FIND_AUTHCERT: &str = "
SELECT contents FROM AuthCerts WHERE id_digest = ? AND sk_digest = ?; SELECT contents FROM AuthCerts WHERE id_digest = ? AND sk_digest = ?;
......
...@@ -1392,7 +1392,7 @@ impl UnvalidatedMDConsensus { ...@@ -1392,7 +1392,7 @@ impl UnvalidatedMDConsensus {
/// ///
/// (This is the case if the consensus claims to be signed by more than /// (This is the case if the consensus claims to be signed by more than
/// half of the authorities in the list.) /// half of the authorities in the list.)
pub fn authorities_are_correct(&self, authorities: &[RSAIdentity]) -> bool { pub fn authorities_are_correct(&self, authorities: &[&RSAIdentity]) -> bool {
self.siggroup.could_validate(authorities) self.siggroup.could_validate(authorities)
} }
} }
...@@ -1452,7 +1452,7 @@ impl SignatureGroup { ...@@ -1452,7 +1452,7 @@ impl SignatureGroup {
/// Given a list of authority identity key fingerprints, return true if /// Given a list of authority identity key fingerprints, return true if
/// this signature group is _potentially_ well-signed according to those /// this signature group is _potentially_ well-signed according to those
/// authorities. /// authorities.
fn could_validate(&self, authorities: &[RSAIdentity]) -> bool { fn could_validate(&self, authorities: &[&RSAIdentity]) -> bool {
let mut signed_by: HashSet<RSAIdentity> = HashSet::new(); let mut signed_by: HashSet<RSAIdentity> = HashSet::new();
for sig in self.signatures.iter() { for sig in self.signatures.iter() {
let id_fp = &sig.key_ids.id_fingerprint; let id_fp = &sig.key_ids.id_fingerprint;
...@@ -1460,7 +1460,7 @@ impl SignatureGroup { ...@@ -1460,7 +1460,7 @@ impl SignatureGroup {
// Already found this in the list. // Already found this in the list.
continue; continue;
} }
if authorities.contains(id_fp) { if authorities.contains(&id_fp) {
signed_by.insert(*id_fp); signed_by.insert(*id_fp);
} }
} }
...@@ -1531,10 +1531,7 @@ mod test { ...@@ -1531,10 +1531,7 @@ mod test {
let cert = cert?.check_signature()?.dangerously_assume_timely(); let cert = cert?.check_signature()?.dangerously_assume_timely();
certs.push(cert); certs.push(cert);
} }
let auth_ids: Vec<_> = certs let auth_ids: Vec<_> = certs.iter().map(|c| &c.key_ids().id_fingerprint).collect();
.iter()
.map(|c| c.key_ids().id_fingerprint.clone())
.collect();
assert_eq!(certs.len(), 3); assert_eq!(certs.len(), 3);
...@@ -1549,7 +1546,7 @@ mod test { ...@@ -1549,7 +1546,7 @@ mod test {
// If we only believe in an authority that isn't listed, // If we only believe in an authority that isn't listed,
// that won't work. // that won't work.
let bad_auth_id = (*b"xxxxxxxxxxxxxxxxxxxx").into(); let bad_auth_id = (*b"xxxxxxxxxxxxxxxxxxxx").into();
assert!(!consensus.authorities_are_correct(&[bad_auth_id])); assert!(!consensus.authorities_are_correct(&[&bad_auth_id]));
} }
let missing = consensus.key_is_correct(&[]).err().unwrap(); let missing = consensus.key_is_correct(&[]).err().unwrap();
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment