Skip to content
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

Sapling note encryption implementation #3324

Merged
merged 6 commits into from Jul 11, 2018

Conversation

ebfull
Copy link
Contributor

@ebfull ebfull commented Jun 6, 2018

Closes #3055

Implemented along with @gtank and @Eirik0

DH key exchange was implemented in zcash/librustzcash#18

@bitcartel bitcartel mentioned this pull request Jun 8, 2018
@str4d str4d added this to the v2.0.0 milestone Jun 13, 2018
@str4d str4d added this to In Progress in Arborist Team Jun 13, 2018
@ebfull ebfull changed the title [WIP] Sapling note encryption implementation Sapling note encryption implementation Jun 14, 2018
@str4d str4d moved this from In Progress to In Review in Arborist Team Jun 15, 2018
@bitcartel
Copy link
Contributor

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Jun 15, 2018

⌛ Trying commit c03e226 with merge b69131dbf4e882acdf33909c012e8406bf4a8229...

@zkbot
Copy link
Contributor

zkbot commented Jun 15, 2018

💔 Test failed - pr-try

@ebfull
Copy link
Contributor Author

ebfull commented Jun 16, 2018

@zkbot retry

@zkbot
Copy link
Contributor

zkbot commented Jun 16, 2018

⌛ Trying commit c03e226 with merge ef181f08fc326e29442b99b2b6eb234808d761b2...

@zkbot
Copy link
Contributor

zkbot commented Jun 16, 2018

☀️ Test successful - pr-try
State: approved= try=True

Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ut(ACK+cov), assuming that my comments are addressed (either in this PR, or in a subsequent one).

#define NOTEENCRYPTION_AUTH_BYTES 16
// Ciphertext for the recipient to decrypt
typedef std::array<unsigned char, ZC_SAPLING_ENCCIPHERTEXT_SIZE> SaplingEncCiphertext;
typedef std::array<unsigned char, ZC_SAPLING_ENCPLAINTEXT_SIZE> SaplingEncPlaintext;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SaplingEncPlaintext should be a subclass of BaseNotePlaintext, with appropriate internal structure and serialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are typedefs just like the Sprout ciphertexts/plaintexts effectively are.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sprout ciphertexts are typedefs. Sprout plaintexts are classes, specifically subclasses of BaseNotePlaintext.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @ebfull is talking about these Sprout typedefs: https://github.com/zcash/zcash/blob/master/src/zcash/NoteEncryption.hpp#L31

    typedef std::array<unsigned char, CLEN> Ciphertext;
    typedef std::array<unsigned char, MLEN> Plaintext;

@str4d Yes, there will be a subclass, I had one sketched out before this PR started, see snippet here:

class SaplingNotePlaintext : public BaseNotePlaintext {
public:
    diversifier_t d;
    uint256 r;


// Ciphertext for outgoing viewing key to decrypt
typedef std::array<unsigned char, ZC_SAPLING_OUTCIPHERTEXT_SIZE> SaplingOutCiphertext;
typedef std::array<unsigned char, ZC_SAPLING_OUTPLAINTEXT_SIZE> SaplingOutPlaintext;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SaplingOutPlaintext should be a class with appropriate internal structure and serialization.

@@ -13,6 +14,58 @@ void clamp_curve25519(unsigned char key[crypto_scalarmult_SCALARBYTES])
key[31] |= 64;
}

void PRF_ock(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this into src/zcash/prf.cpp (below PRF_ovk).

@@ -13,6 +14,58 @@ void clamp_curve25519(unsigned char key[crypto_scalarmult_SCALARBYTES])
key[31] |= 64;
}

void PRF_ock(
unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name this ock.

@@ -17,6 +18,154 @@ class TestNoteDecryption : public ZCNoteDecryption {
}
};

TEST(noteencryption, sapling_api)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No underscores in test names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That gets us every time.

}

// Invalid diversifier
ASSERT_FALSE(SaplingNoteEncryption::FromDiversifier({1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? I seem to recall that boost::optional doesn't get correctly coerced to a boolean in Boost Test's macros; does it work in Google Test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test passes but is the behaviour correct? Hmm. To remove any doubt, consider using a variable to store the boost::optional and then assert on the value of the variable itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Violation of substitutability, and for a pure expression at that, makes me very sad.

image

Copy link
Contributor

@bitcartel bitcartel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some minor points raised.

auto sk = SaplingSpendingKey(uint256()).expanded_spending_key();
auto vk = sk.full_viewing_key();
auto ivk = vk.in_viewing_key();
SaplingPaymentAddress pk_1 = *ivk.address({0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style. I prefer using .get() rather than *ivk

}

// Invalid diversifier
ASSERT_FALSE(SaplingNoteEncryption::FromDiversifier({1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test passes but is the behaviour correct? Hmm. To remove any doubt, consider using a variable to store the boost::optional and then assert on the value of the variable itself.

ivk,
epk_1
);
ASSERT_TRUE(message == plaintext_1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== on arrays is deprecated in C++20 but since we use this syntax everywhere, we can deal with it later when we get to C++17...

memcpy(block+96, epk.begin(), 32);

unsigned char personalization[crypto_generichash_blake2b_PERSONALBYTES] = {};
memcpy(personalization, "Zcash_Derive_ock", 16);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this in the protocol spec? Don't see it in the current revision (from 28 days ago, dated May 22)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memcpy(block+32, epk.begin(), 32);

unsigned char personalization[crypto_generichash_blake2b_PERSONALBYTES] = {};
memcpy(personalization, "Zcash_SaplingKDF", 16);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use constexpr or macro for "Zcash_SaplingKDF"

memcpy(block+96, epk.begin(), 32);

unsigned char personalization[crypto_generichash_blake2b_PERSONALBYTES] = {};
memcpy(personalization, "Zcash_Derive_ock", 16);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use constexpr or macro for "Zcash_Derive_ock"

uint256 esk;

SaplingNoteEncryption(uint256 epk, uint256 esk) : epk(epk), esk(esk) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove redundant line of whitespace, and have { } on the same line after function sig.

);

ASSERT_THROW(tmp_enc.encrypt_to_recipient(
pk_1.pk_d,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change this line to pk_2.pk_d the call still throws the same error due to already_encrypted_enc having been set, so we're not really testing nonce reuse, but the fact that encrypt_to_recipient can be only called once successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the intent of the nonce reuse resistant API; if you call encrypt_to_recipient twice (which you should never do, ephemeral keys are not reused) then the ChaCha20 nonce will be reused, which we cannot allow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Note that encrypt_to_recipient could be called twice because the method is not thread safe with regards to the boolean flag. Perhaps document that the note encryption class should not be used by multiple threads concurrently.

@str4d str4d moved this from In Review to In Progress in Arborist Team Jun 22, 2018
@daira
Copy link
Contributor

daira commented Jun 22, 2018

Currently reviewing this for consistency with the spec (since there were fairly recent spec changes), so please hold off merging for a bit.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flushing pending comments.

}

// Invalid diversifier
ASSERT_FALSE(SaplingNoteEncryption::FromDiversifier({1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Violation of substitutability, and for a pure expression at that, makes me very sad.

image

uint256(),
epk_2
));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also test malleating the ciphertext.

unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE],
const uint256 &ovk,
const uint256 &cv,
const uint256 &cm,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now called cmu. (It was always the u-coordinate.)

memcpy(block+96, epk.begin(), 32);

unsigned char personalization[crypto_generichash_blake2b_PERSONALBYTES] = {};
memcpy(personalization, "Zcash_Derive_ock", 16);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint256 esk;

// Pick random esk
librustzcash_sapling_generate_r(esk.begin());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the beta-21 spec says that esk should not be 0. (This is a consequence of the implementation already checking that epk is not of small order.) This has negligible probability so it's not so important.


// Construct the symmetric key
unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE];
KDF_Sapling(K, dhsecret, epk);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts the output parameter first, whereas for librustzcash_sapling_ka_agree for example it is last. I don't know which one should change; crypto_aead_chacha20poly1305_ietf_encrypt also puts the output parameter first.

SaplingOutCiphertext SaplingNoteEncryption::encrypt_to_ourselves(
const uint256 &ovk,
const uint256 &cv,
const uint256 &cm,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to also be cmu, given that it's passed directly to PRF_ock.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK as far as it goes, but missing a lot of the checks in the spec. I assume these will be in another PR?

const SaplingOutCiphertext &ciphertext,
const uint256 &ovk,
const uint256 &cv,
const uint256 &cm,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also cmu.

return boost::none;
}

return plaintext;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only does the Cout decryption. In the spec (section 4.17.3), that is combined with:

  • checking esk and pkd
  • reconstructing the sharedSecret and using it to decrypt Cenc
  • checking rcm and gd
  • checking that epk is consistent with gd and esk
  • checking the commitment of the reconstructed note against cmu.

return boost::none;
}

return plaintext;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only does the Cenc decryption. In the spec (section 4.17.2), that is combined with:

  • checking rcm and gd
  • checking the commitment of the reconstructed note against cmu.

@ebfull
Copy link
Contributor Author

ebfull commented Jun 23, 2018

Yup, this abstraction does not deal with checks of the contents, only encrypting and decrypting.

@str4d str4d modified the milestones: v1.1.2, v2.0.0 Jul 2, 2018
@bitcartel
Copy link
Contributor

I've reviewed minor edits in pairing with @ebfull

@ebfull
Copy link
Contributor Author

ebfull commented Jul 11, 2018

Comments about renaming and refactorings will be addressed at a later date, and this PR is not responsible for plaintext serialization and the other kinds of Receive-like checks, just the encryption/decryption.

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Jul 11, 2018

📌 Commit 7478876 has been approved by ebfull

@zkbot
Copy link
Contributor

zkbot commented Jul 11, 2018

⌛ Testing commit 7478876 with merge d86f60f...

zkbot added a commit that referenced this pull request Jul 11, 2018
Sapling note encryption implementation

Closes #3055

Implemented along with @gtank and @Eirik0

DH key exchange was implemented in zcash/librustzcash#18
@zkbot
Copy link
Contributor

zkbot commented Jul 11, 2018

☀️ Test successful - pr-merge
Approved by: ebfull
Pushing d86f60f to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Arborist Team
  
Released (Merged in Master)
Development

Successfully merging this pull request may close these issues.

None yet

5 participants