-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a test for the reordering attack on BinaryAgreement #270
Add a test for the reordering attack on BinaryAgreement #270
Conversation
tests/binary_agreement.rs
Outdated
if !self.sent_attack { | ||
self.sent_attack = true; | ||
for &our_node_id in &self.known_adversarial_ids { | ||
let f = self.known_node_ids.len() / 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.known_node_ids
doesn't change inside the loop, I think? Then computing f
could be moved outside of the loop.
tests/binary_agreement.rs
Outdated
#[test] | ||
fn test_binary_agreement_reordering() { | ||
let new_adversary = |_: usize, _: usize| ReorderingAdversary::default(); | ||
test_binary_agreement_different_sizes(new_adversary); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really make sense for this particular test to run with InputConfig
s other than Split
?
tests/binary_agreement.rs
Outdated
let value = match input { | ||
InputConfig::Fixed(b) => b, | ||
InputConfig::Random => rand::random(), | ||
InputConfig::Split => i < (2 * network.nodes.len() / 3), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this input true
for the first third? The comment and the code above suggest it should be the other way round.
tests/binary_agreement.rs
Outdated
use network::{Adversary, MessageScheduler, NodeId, SilentAdversary, TestNetwork, TestNode}; | ||
use network::{ | ||
Adversary, MessageScheduler, MessageWithSender, NodeId, SilentAdversary, TestNetwork, TestNode, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're currently in the process of transitioning from the old (network
) to the new (net
) test framework. I'm not sure how complicated it would be to port this to the new one?
tests/binary_agreement.rs
Outdated
} | ||
} | ||
} | ||
messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really reorder any messages, it just forges new messages from correct nodes, which the adversary can't do in reality. These messages may then arrive in any order (in addition to the original ones, sent by the correct nodes themselves), so I don't think it reliably replicates the attack.
I think the new net
framework provides better controls over message reordering. (Please correct me if I'm wrong, @mbr.)
Also, the attacker needs to reconstruct the coin value before anyone else, and then send the Aux
messages accordingly.
Once the test is complete, please try (without checking it in, or maybe just in a separate temporary branch) disabling the Conf
messages in binary_agreement
, and use aux_vals
itself instead of conf_values
to try and update the epoch. With that change, this test should fail (i.e. the attack works) but all the other tests should still pass (i.e. binary_agreement
is still working as originally described in the paper).
Ah, sorry, you already wrote that it's not an MITM adversary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this issue! Further to @afck's comments. I'm also not convinced the First
message scheduler is enough. I'd like you to continue adding stages of the attack which are not yet present:
- After
A0
andA1
broadcastAux(_)
and messages withinA
are delivered,x
sends bothBVal(false)
andBVal(true)
to every node inA
. x
attempts to compute the random coin value. This may be possible withoutConf
messages which were added as a defence agains this attack.
Also, I believe the divergence check can be done by finding repeated outputs rather than by counting up to a large number.
tests/binary_agreement.rs
Outdated
let value = match input { | ||
InputConfig::Fixed(b) => b, | ||
InputConfig::Random => rand::random(), | ||
InputConfig::Split => i < (2 * network.nodes.len() / 3), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only test constant input: true
for the set A
and false
for the set B
. There can be a second attack with these values negated. That possible attack is not covered here.
I'll rewrite this as a MITM adversary using the net framework if possible. I don't believe this is currently a cheating adversary, though. |
src/binary_agreement/mod.rs
Outdated
@@ -66,7 +66,7 @@ | |||
mod binary_agreement; | |||
mod bool_multimap; | |||
pub mod bool_set; | |||
mod sbv_broadcast; | |||
pub mod sbv_broadcast; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I was expecting you to comment on was this. It's necessary for my test, but I'm not sure if its API should normally be exposed. Should I do #[cfg_attr(test, pub)]
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't think that algorithm on its own is generally useful and maybe it would just be confusing to expose it. Maybe we should just re-export sbv_broadcast::Message as SbvMessage
from binary_agreement
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I think it's "potentially cheating" in the sense that it creates messages with correct nodes as their senders without verifying that those messages were actually sent.
Anyway, ideally it shouldn't create those messages, and instead just reorder the ones that are already pending.
If I understand correctly, net
should already support this kind of reordering, but possibly it may need slight extensions. Let me and @mbr know if you have any questions.
src/binary_agreement/mod.rs
Outdated
@@ -66,7 +66,7 @@ | |||
mod binary_agreement; | |||
mod bool_multimap; | |||
pub mod bool_set; | |||
mod sbv_broadcast; | |||
pub mod sbv_broadcast; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't think that algorithm on its own is generally useful and maybe it would just be confusing to expose it. Maybe we should just re-export sbv_broadcast::Message as SbvMessage
from binary_agreement
?
3ad7a8d
to
0fb454b
Compare
I rewrote the test using the net framework. I'll attempt to verify that it's working next (there's a good chance I still messed something up). |
86bd011
to
12d2436
Compare
I've fixed the test, and verified that it's working properly. If you apply this patch to disable the Conf round, the other binary agreement tests pass, but this test fails: diff --git a/src/binary_agreement/sbv_broadcast.rs b/src/binary_agreement/sbv_broadcast.rs
index 4829383..342cadf 100644
--- a/src/binary_agreement/sbv_broadcast.rs
+++ b/src/binary_agreement/sbv_broadcast.rs
@@ -123,11 +123,7 @@ impl<N: NodeIdT> SbvBroadcast<N> {
if count_bval == 2 * self.netinfo.num_faulty() + 1 {
self.bin_values.insert(b);
- if self.bin_values != bool_set::BOTH {
- step.extend(self.send(Message::Aux(b))?) // First entry: send `Aux(b)`.
- } else {
- step.extend(self.try_output()?); // Otherwise just check for `Conf` condition.
- }
+ step.extend(self.send(Message::Aux(b))?);
}
if count_bval == self.netinfo.num_faulty() + 1 { |
26352ef
to
9d5996d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disabled the Conf
round differently and the test passed. Can you please check with the diff below?
diff --git a/src/binary_agreement/binary_agreement.rs b/src/binary_agreement/binary_agreement.rs
index 5beac06..f28e4c6 100644
--- a/src/binary_agreement/binary_agreement.rs
+++ b/src/binary_agreement/binary_agreement.rs
@@ -186,8 +186,8 @@ impl<N: NodeIdT> BinaryAgreement<N> {
step.extend(self.try_update_epoch()?)
}
CoinState::InProgress(_) => {
- // Start the `Conf` message round.
- step.extend(self.send_conf(aux_vals)?)
+ // Bypass the `Conf` message round.
+ step.extend(self.try_finish_conf_round()?)
}
}
}
@@ -347,10 +347,6 @@ impl<N: NodeIdT> BinaryAgreement<N> {
/// Checks whether the _N - f_ `Conf` messages have arrived, and if so, activates the coin.
fn try_finish_conf_round(&mut self) -> Result<Step<N>> {
- if self.conf_values.is_none() || self.count_conf() < self.netinfo.num_correct() {
- return Ok(Step::default());
- }
-
// Invoke the coin.
let coin_step = match self.coin_state {
CoinState::Decided(_) => return Ok(Step::default()), // Coin has already decided.
tests/binary_agreement_mitm.rs
Outdated
sent_final_messages: bool, | ||
} | ||
|
||
const NODES_PER_GROUP: usize = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this value be randomized? For example, sampled randomly from a given range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand this is just a regression test for one specific case. I wonder whether we should just set this to 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, making it random will answer both questions :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but it's a pretty complicated attack, and I think most steps of it are still missing from the implementation.
I agree with @vkomenda: The above diff doesn't disable the Conf
messages, but just sends the Aux
earlier. I think sbv_broadcast
doesn't need to be changed. @vkomenda's diff looks right to me, except I think we need self.conf_values = Some(aux_vals)
before calling try_finish_conf_round
?
tests/binary_agreement_mitm.rs
Outdated
const BVAL_TRUE_MSG: Message = Message { | ||
epoch: 0, | ||
content: MessageContent::SbvBroadcast(SbvMessage::BVal(true)), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can also write:
MessageContent::SbvBroadcast(SbvMessage::BVal(false)).with_epoch(epoch)
tests/binary_agreement_mitm.rs
Outdated
_: NetMessage<BinaryAgreement<NodeId>>, | ||
) -> Result<Step<BinaryAgreement<NodeId>>, CrankError<BinaryAgreement<NodeId>>> { | ||
const BVAL_FALSE_MSG: Message = Message { | ||
epoch: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this attack needs to be repeated in every epoch, not just 0
.
tests/binary_agreement_mitm.rs
Outdated
sent_final_messages: bool, | ||
} | ||
|
||
const NODES_PER_GROUP: usize = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand this is just a regression test for one specific case. I wonder whether we should just set this to 1
?
I agree with @afck, we possibly need In that case changing |
tests/binary_agreement_mitm.rs
Outdated
.adversary(ReorderingAdversary::default()) | ||
.crank_limit(10000) | ||
.using(|info| { | ||
BinaryAgreement::new(Arc::new(info.netinfo), 0, info.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The third argument ("proposer_id
") needs to be the same in all nodes.
That's indeed a confusing API, of course, and we should definitely change it. But for now, you can just replace info.id
with 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
9d5996d
to
55d4ff7
Compare
It's now passing, but failing with this patch: diff --git a/src/binary_agreement/binary_agreement.rs b/src/binary_agreement/binary_agreement.rs
index 5beac06..e1c1265 100644
--- a/src/binary_agreement/binary_agreement.rs
+++ b/src/binary_agreement/binary_agreement.rs
@@ -247,7 +247,8 @@ impl<N: NodeIdT> BinaryAgreement<N> {
return Ok(self.try_finish_conf_round()?);
}
- self.send(MessageContent::Conf(values))
+ //self.send(MessageContent::Conf(values))
+ Ok(Step::default())
}
/// Multicasts and handles a message. Does nothing if we are only an observer. @afck pointed out that I'm still missing the last part of the attack, so I'm not sure why this is happening. |
Just not sending the |
But if you apply only the above 1-line patch, without adding the |
@d33a94975ba60d59 , sorry for confusion. I wasn't descriptive enough. You would need to return |
1 similar comment
@d33a94975ba60d59 , sorry for confusion. I wasn't descriptive enough. You would need to return |
55d4ff7
to
9f6a519
Compare
This has been much more complicated than I expected, but once again, I think it's working. It's passing, but failing with diff --git a/src/binary_agreement/binary_agreement.rs b/src/binary_agreement/binary_agreement.rs
index 61ac175..0db9e40 100644
--- a/src/binary_agreement/binary_agreement.rs
+++ b/src/binary_agreement/binary_agreement.rs
@@ -204,7 +204,9 @@ impl<N: NodeIdT> BinaryAgreement<N> {
}
CoinState::InProgress(_) => {
// Start the `Conf` message round.
- step.extend(self.send_conf(aux_vals)?)
+ //step.extend(self.send_conf(aux_vals)?)
+ self.conf_values = Some(aux_vals);
+ step.extend(self.try_finish_conf_round()?)
}
}
}
@@ -363,9 +365,11 @@ impl<N: NodeIdT> BinaryAgreement<N> {
/// Checks whether the _N - f_ `Conf` messages have arrived, and if so, activates the coin.
fn try_finish_conf_round(&mut self) -> Result<Step<N>> {
+ /*
if self.conf_values.is_none() || self.count_conf() < self.netinfo.num_correct() {
return Ok(Step::default());
}
+ */
// Invoke the coin.
let ts_step = match self.coin_state {
With that patch, the BinaryAgreement gets to around epoch 50 before the crank limit is exceeded, and logging the estimated values for each node, it seems clear to me that the attack is now working. I've also rewritten this to use a much more modular/configurable system. The STAGES array defines the attack, and the rest of the code just carries it out. |
f9f9349
to
459c428
Compare
04b224e
to
5aaf89a
Compare
tests/binary_agreement_mitm.rs
Outdated
msg_count: NODES_PER_GROUP * (NODES_PER_GROUP * 2), | ||
}, | ||
// Messages within A0 are delivered. | ||
// Thus nodes in A0 see |B|+|F|=f+1 votes for \neg v; so all nodes in A0 broadcast BVAL(\neg v) and all nodes in A0 see |A0|+|B|+|F|=2f+1 votes for \neg v; so all nodes in A0 broadcast AUX(\neg v). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's break the lines into fewer than 100 characters. (Also below.)
tests/binary_agreement_mitm.rs
Outdated
// x sends BVAL(\neg v) to the nodes in A0 | ||
Stage { | ||
source_groups: &[3], | ||
dest_groups: &[0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more readable to make the group numbers constants: &[F]
, &[A0]
, etc.
tests/binary_agreement_mitm.rs
Outdated
msg_count: NODES_PER_GROUP * 2, | ||
}, | ||
// !! Not mentioned in the GitHub issue, but seems necessary. | ||
// F sends Aux(_) to A, because nodes in A need 2f+1 Aux messages before they broadcasts their coins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Typo: "they broadcast")
tests/binary_agreement_mitm.rs
Outdated
/// Described here: https://github.com/amiller/HoneyBadgerBFT/issues/59#issue-310368284 | ||
/// Excluding the first node, which is F, | ||
/// A0 is the first third of nodes, A1 is the second third, and the rest are B. | ||
struct ReorderingAdversary { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give this a more specific name (also the test function below); there's a general random ReorderingAdversary
now, too.
Can't really think of a good one… maybe AbaCommonCoinAdversary
?
/// The estimated value for nodes in A. | ||
a_estimated: bool, | ||
// TODO this is really hacky but there's no better way to get this value | ||
netinfo_mutex: Arc<Mutex<Option<Arc<NetworkInfo<NodeId>>>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The network info shouldn't even change over the course of this test, and I think it's also clonable? Option<Arc<NetworkInfo<NodeId>>>
or even Option<NetworkInfo<NodeId>>
should work…?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting the networkinfo into the adversary is a surprisingly difficult task. The closure that gets the netinfo must be created after the adversary is created, but cannot have a mutable reference to the adversary because it needs to be moved into the NetBuilder. I tried to extract the adversary from the NetBuilder in the closure, but with only a mutable reference to a box I can't downcast it to my adversary type. If I could convert it to an Any it'd work, but even making Adversary a super-trait requiring Any, I run into https://github.com/rust-lang/rust/issues/5665
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is having the Adversary type as a type parameter of NetBuilder. This would make creating a network without an adversary more difficult, though we could just have a helper for that using NullAdversary. What do you think about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbr: Have you encountered this problem before? It sounds like making the adversary a parameter of NetBuilder
would be a decent solution?
(Anyway, that can certainly be cleaned up in a later PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is netinfo_mutex
only used to get the invocation_id
? If so, don't use NetworkInfo
inside the adversary. Pass only data that are needed to construct the nonce, or, in the current master, coin_id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, its primary usage is the line after that, where it's passed to ThresholdSign. I don't know what ThresholdSign does with it but it's probably not worth changing the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. Another option would be to clone the NetworkInfo
of the NewNodeInfo
. It's read-only in BinaryAgreement
, so it should be fine to clone it at the time of building the VirtualNet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what's currently done :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you clone, you don't need the enclosing Arc<Mutex<Option<_>>>
in the constructor argument of the adversary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do, because the adversary has to be created before the NewNodeInfo is present, unless we change the NetBuilder API.
@@ -11,7 +11,7 @@ use {DistAlgorithm, NetworkInfo, NodeIdT, Target}; | |||
/// The state of the current epoch's coin. In some epochs this is fixed, in others it starts | |||
/// with in `InProgress`. | |||
#[derive(Debug)] | |||
enum CoinState<N> { | |||
pub enum CoinState<N> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd almost prefer duplicating the CoinState
in the test, if necessary, rather than exposing this implementation feature.
src/binary_agreement/mod.rs
Outdated
@@ -146,7 +147,7 @@ impl rand::Rand for MessageContent { | |||
} | |||
|
|||
#[derive(Clone, Debug)] | |||
struct Nonce(Vec<u8>); | |||
pub struct Nonce(Vec<u8>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the merge conflict! 😬
The new session ID can also just be 0, though, so it shouldn't make a difference.
let src_group = if from == 0 { | ||
3 | ||
} else { | ||
(from - 1) / NODES_PER_GROUP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be simpler if the adversary were the last node, not the first.
(Not sure if it's worth changing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes other parts a bit more complicated, like iterating over the correct algorithms. I think having the faulty node first is fine.
5aaf89a
to
f5b3bc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The estimated value for nodes in A. | ||
a_estimated: bool, | ||
// TODO this is really hacky but there's no better way to get this value | ||
netinfo_mutex: Arc<Mutex<Option<Arc<NetworkInfo<NodeId>>>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbr: Have you encountered this problem before? It sounds like making the adversary a parameter of NetBuilder
would be a decent solution?
(Anyway, that can certainly be cleaned up in a later PR.)
tests/binary_agreement_mitm.rs
Outdated
/// The state of the current epoch's coin. In some epochs this is fixed, in others it starts | ||
/// with in `InProgress`. | ||
#[derive(Debug)] | ||
pub enum CoinState<N> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That probably doesn't need to be public.
f5b3bc8
to
4578967
Compare
4578967
to
1f591bd
Compare
Rebased. I added Algo and SessionId typedefs in the process. |
Looks fine by me. Please update to Rust 1.30.0. |
1f591bd
to
c132751
Compare
What do you mean by that? Did I miss a |
Sorry, I meant the formatting errors. The PR is ready for merging :) |
Fixes #228