Problem: didn't verify secrets against public key in DirectPath #2018

yihuang · 2020-07-24T19:53:05Z

Solution:

verify decrypted secret against public key
also verify leaf keypackage against leaf secret

tomtau

would be good to add a test + document (some explanation why) the "non-obvious" part:

kp_secret_pending has newer signature key but older kem key, new signature key is generated when preparing update proposal.
kp_secret has newer kem key but older signature key, new kem key is generated when commit the self-updating proposal.

leejw51crypto · 2020-07-27T02:40:09Z

chain-tx-enclave-next/mls/src/group.rs

+        let self_update_proposal = commit_content
+            .updates
+            .iter()
+            .filter_map(|(sender, update)| {


can use find? because it's finding the first one?

leejw51crypto

lgtm

tomtau · 2020-07-27T02:48:15Z

chain-tx-enclave-next/mls/src/group.rs

@@ -835,6 +848,8 @@ pub enum ProcessCommitError {
    CommitError,
    #[error("decrypted path secret don't match the public key")]
    PathSecretPublicKeyDontMatch,
+    #[error("cached secret key don't match commit leaf keypackage")]
+    LeafSecretPublicKeyDontMatch,


seems this error is not used?

yes, let me remove it, this error case is now keypackage verify error.

tomtau · 2020-07-27T03:32:58Z

chain-tx-enclave-next/mls/src/tree.rs

@@ -570,6 +570,28 @@ impl TreePublicKey {
        }
        self.nodes[path[0].node_index()].compute_parent_hash(self.cs)
    }
+
+    fn verify_node_private_key(


is it ok to make this pub(crate)? I may need to use this -- just document the intended usage/args

…pto-com#1797) Solution: sketched out core of "NACK" mechanism which involves revealing shared secrets from invalid message parts and including DLEQ proofs. -- currently, needs: 1) latest master of p256 which contains scalar arithmetic (not yet released) 2) for the high-level API, it needs to directly decrypt HPKE ciphertext from a shared secret -- this may not ever be released also needs "verify_node_private_key" from crypto-com#2018

yihuang · 2020-07-28T01:16:44Z

Added pending_updates(record pending credential private keys for self update proposals) and pending_commit (record pending init private keys for pending commits), it's more clear to handle pending private keys this way.

tomtau · 2020-07-28T02:03:40Z

chain-tx-enclave-next/mls/src/tree.rs

+            if !bool::from(
+                private_key
+                    .public_key()
+                    .marshal()
+                    .ct_eq(&node.public_key.marshal()),
+            ) {
+                false
+            } else {
+                true
+            }


could this be rewritten as:

bool::from(private_key .public_key() .marshal() .ct_eq(&node.public_key.marshal())

or private_key.public_key().marshal().ct_eq(&node.public_key.marshal().into() without this extra if branching?

yes, nice catch

tomtau · 2020-07-28T02:11:33Z

chain-tx-enclave-next/mls/src/tree.rs

+                }
+            }
+
+            self.path_secrets.truncate(overlap_pos);


was a bit puzzled about truncate here -- I guess the meaning is that it'll keep secrets up to the overlap start (as they are unchanged), and then replace with was decrypted / computed?
a comment could be good here

yes, added a comment now.

tomtau

looks all right, but would be good to add a test where the encrypted secret doesn't match, so ProcessCommitError is correctly returned (and group state etc. wasn't changed)

yihuang · 2020-07-28T02:30:35Z

looks all right, but would be good to add a test where the encrypted secret doesn't match, so ProcessCommitError is correctly returned (and group state etc. wasn't changed)

Another issue is, for multiple pending commits, there'll be only one is valid, the pending secrets for other commits will not cleaned up. don't know when or how to cleanup the pending_updates/pending_commits of unconfirmed commits currently.

tomtau · 2020-07-28T02:34:40Z

looks all right, but would be good to add a test where the encrypted secret doesn't match, so ProcessCommitError is correctly returned (and group state etc. wasn't changed)

Another issue is, for multiple pending commits, there'll be only one is valid, the pending secrets for other commits will not cleaned up. don't know when or how to cleanup the pending_updates/pending_commits of unconfirmed commits currently.

cleanup after applying the "committed" commit (block time when it was included + NACK timeout)?

yihuang · 2020-07-28T02:37:21Z

looks all right, but would be good to add a test where the encrypted secret doesn't match, so ProcessCommitError is correctly returned (and group state etc. wasn't changed)

Another issue is, for multiple pending commits, there'll be only one is valid, the pending secrets for other commits will not cleaned up. don't know when or how to cleanup the pending_updates/pending_commits of unconfirmed commits currently.

cleanup after applying the "committed" commit (block time when it was included + NACK timeout)?

I guess just remove all the pending secrets after processing any commit, because all the pending commits will be invalid anyway(the epoch is old)?

tomtau · 2020-07-28T02:37:44Z

needs to be documented crypto-org-chain/chain-docs#141 (comment)

in here https://github.com/crypto-com/chain-docs/blob/master/docs/modules/tdbe.md#abci-processing-sketch , it'll be in transitions 8. and 10.

tomtau · 2020-07-28T02:39:27Z

I guess just remove all the pending secrets after processing any commit, because all the pending commits will be invalid anyway(the epoch is old)?

Yes, I think so -- if e.g. you happen to generate self update proposal while there was another on-going update/removal, it'll be rejected... so you'll need to re-generate and re-submit.

codecov · 2020-07-28T03:05:15Z

Codecov Report

Merging #2018 into master will increase coverage by 0.13%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master    #2018      +/-   ##
==========================================
+ Coverage   65.01%   65.15%   +0.13%     
==========================================
  Files         213      212       -1     
  Lines       26520    26649     +129     
==========================================
+ Hits        17242    17363     +121     
- Misses       9278     9286       +8

Impacted Files	Coverage Δ
chain-tx-enclave-next/mls/src/key.rs	`71.25% <ø> (+4.58%)`	⬆️
chain-tx-enclave-next/mls/src/keypackage.rs	`77.43% <80.95%> (-4.51%)`	⬇️
chain-tx-enclave-next/mls/src/tree.rs	`79.87% <81.42%> (-0.39%)`	⬇️
chain-tx-enclave-next/mls/src/message.rs	`67.74% <85.71%> (+0.27%)`	⬆️
chain-tx-enclave-next/mls/src/group.rs	`85.77% <93.23%> (+0.83%)`	⬆️
chain-tx-enclave-next/mls/src/ciphersuite.rs	`78.23% <100.00%> (+0.52%)`	⬆️
chain-abci/src/app/end_block.rs	`97.29% <0.00%> (-2.71%)`	⬇️
client-common/src/storage/sled_storage.rs	`50.28% <0.00%> (-0.87%)`	⬇️
...ommon/src/tendermint/rpc_client/sync_rpc_client.rs	`0.00% <0.00%> (ø)`
... and 16 more

yihuang · 2020-07-29T00:56:43Z

Fixed partial mutation when process invalid commit.

Solution: - verify decreted secret against public key - also verify leaf keypackage against leaf secret

tomtau · 2020-07-29T03:14:00Z

bors r+

bors · 2020-07-29T04:26:04Z

Build succeeded:

…pto-com#1797) Solution: sketched out core of "NACK" mechanism which involves revealing shared secrets from invalid message parts and including DLEQ proofs. -- currently, needs: 1) latest master of p256 which contains scalar arithmetic (not yet released) 2) for the high-level API, it needs to directly decrypt HPKE ciphertext from a shared secret -- this may not ever be released also needs "verify_node_private_key" from crypto-com#2018

2029: Problem: no way for mls member to prove invalid ciphertext (fixes #1797) r=tomtau a=tomtau Solution: sketched out core of "NACK" mechanism which involves revealing shared secrets from invalid message parts and including DLEQ proofs. -- currently, needs: 1) latest master of p256 which contains scalar arithmetic (not yet released) 2) for the high-level API, it needs to directly decrypt HPKE ciphertext from a shared secret -- this may not ever be released also needs "verify_node_private_key" from #2018 Co-authored-by: Tomas Tauber <[email protected]>

yihuang requested review from calvinaco, devashishdxt and tomtau as code owners July 24, 2020 19:53

tomtau suggested changes Jul 27, 2020

View reviewed changes

tomtau requested review from leejw51crypto and linfeng-crypto July 27, 2020 00:51

leejw51crypto reviewed Jul 27, 2020

View reviewed changes

leejw51crypto approved these changes Jul 27, 2020

View reviewed changes

tomtau reviewed Jul 27, 2020

View reviewed changes

tomtau mentioned this pull request Jul 27, 2020

Problem: no way for mls member to prove invalid ciphertext (fixes #1797) #2029

Merged

yihuang force-pushed the verify-path-secrets branch from fab1218 to 169fffa Compare July 28, 2020 01:15

tomtau reviewed Jul 28, 2020

View reviewed changes

yihuang force-pushed the verify-path-secrets branch from 169fffa to 4e7cae6 Compare July 28, 2020 02:15

tomtau reviewed Jul 28, 2020

View reviewed changes

yihuang force-pushed the verify-path-secrets branch from 4e7cae6 to 4deeb28 Compare July 28, 2020 02:17

yihuang force-pushed the verify-path-secrets branch 2 times, most recently from 7106922 to 2d6c0b8 Compare July 29, 2020 00:51

yihuang requested a review from tomtau July 29, 2020 00:52

yihuang force-pushed the verify-path-secrets branch from 2d6c0b8 to fb59df2 Compare July 29, 2020 00:56

yihuang force-pushed the verify-path-secrets branch from fb59df2 to b817a6b Compare July 29, 2020 00:58

Problem: didn't verify secrets against public key in DirectPath

7566540

Solution: - verify decreted secret against public key - also verify leaf keypackage against leaf secret

yihuang force-pushed the verify-path-secrets branch from b817a6b to 7566540 Compare July 29, 2020 02:01

bors bot merged commit d6fe742 into crypto-com:master Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: didn't verify secrets against public key in DirectPath #2018

Problem: didn't verify secrets against public key in DirectPath #2018

yihuang commented Jul 24, 2020 •

edited

Loading

tomtau left a comment

leejw51crypto Jul 27, 2020

yihuang Jul 28, 2020

leejw51crypto left a comment

tomtau Jul 27, 2020

yihuang Jul 27, 2020

yihuang Jul 28, 2020

tomtau Jul 27, 2020

yihuang Jul 28, 2020

yihuang commented Jul 28, 2020 •

edited

Loading

tomtau Jul 28, 2020 •

edited

Loading

yihuang Jul 28, 2020

tomtau Jul 28, 2020

yihuang Jul 28, 2020 •

edited

Loading

tomtau left a comment

yihuang commented Jul 28, 2020

tomtau commented Jul 28, 2020

yihuang commented Jul 28, 2020 •

edited

Loading

tomtau commented Jul 28, 2020

tomtau commented Jul 28, 2020 •

edited

Loading

codecov bot commented Jul 28, 2020 •

edited

Loading

yihuang commented Jul 29, 2020

tomtau commented Jul 29, 2020

bors bot commented Jul 29, 2020

Problem: didn't verify secrets against public key in DirectPath #2018

Problem: didn't verify secrets against public key in DirectPath #2018

Conversation

yihuang commented Jul 24, 2020 • edited Loading

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang commented Jul 28, 2020 • edited Loading

tomtau Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

yihuang commented Jul 28, 2020

tomtau commented Jul 28, 2020

yihuang commented Jul 28, 2020 • edited Loading

tomtau commented Jul 28, 2020

tomtau commented Jul 28, 2020 • edited Loading

codecov bot commented Jul 28, 2020 • edited Loading

Codecov Report

yihuang commented Jul 29, 2020

tomtau commented Jul 29, 2020

bors bot commented Jul 29, 2020

yihuang commented Jul 24, 2020 •

edited

Loading

yihuang commented Jul 28, 2020 •

edited

Loading

tomtau Jul 28, 2020 •

edited

Loading

yihuang Jul 28, 2020 •

edited

Loading

yihuang commented Jul 28, 2020 •

edited

Loading

tomtau commented Jul 28, 2020 •

edited

Loading

codecov bot commented Jul 28, 2020 •

edited

Loading