Add a simple example for aead #526

jaysonsantos · 2017-04-28T11:33:36Z

Hi, I've added this to try to help newcomers (like me) when using a cryptography library to avoid common errors and to have something to run ASAP.
Let me know if I wrote some BS and if this is the rigt place to put it.

Philipp91

I very very much appreciate you adding a code sample for what is probably the most important part of the ring API. Thank you!

Note that Brian plans on changing the API around (#436), though I don't know when. This might require changes to the code sample in the future, but that's fine, I guess.

Also, the way you (don't) use the output_size return value shows once again how confusing the current API is.

Philipp91 · 2017-04-28T11:57:32Z

src/aead/aead.rs

@@ -25,6 +25,82 @@
 //! [AEAD]: http://www-cse.ucsd.edu/~mihir/papers/oem.html
 //! [`crypto.cipher.AEAD`]: https://golang.org/pkg/crypto/cipher/#AEAD

+//!
+//! **Example (if you don't know what you are doing, read this first)**


You could explain what AEAD is (i.e. what most people need when they need "encryption"). To tell people "this is the right place for you if you believed you just wanted to encrypt."

Please follow the template used in ring::signature's module-level documentation. In particular, this should be:

//! # Example

No need for the "if you don't know what you are doing…" comment.

Do you have a suggestion for a start line?

Don't try to explain what an AEAD is in this example. We already try to do that and link to a paper about it above.

This module is what you need when you want to encrypt something symmetrically (using the same secret key for encryption and decryption). Instead of using unauthenticated encryption (AES-ECB, AES-CBC, AES-CTR and the like), you probably want to use authenticated encryption (aka. AEAD) as implemented in this module -- unless you know what you are doing.

The text deliberately mentions the things that it replaces (ECB, CBC) because experiments I conducted (with Rust/crypto beginners) have showed that people search for those (on the web and in the documentation) and find other implementations, so mentioning these helps potential ring users find what they thought they needed.

The scope of this PR is to add an example. If we want to add documentation beyond the example then let's do it as a separate PR. In particular, I don't want to expand the scope of what's required of @jaysonsantos way beyond what he originally was intending.

Philipp91 · 2017-04-28T11:58:30Z

src/aead/aead.rs

+//!    // The password will be used to generate a key and it is normally used when
+//!    // it is generated by the user. If you need a key which will be stored in a file you
+//!    // generate it directly.
+//!    let password = b"nice password";


You should make it exceedingly clear that the password should not be hard-coded and this is here only because it's sample code. Maybe change it to `b"DO NOT hard-code passwords, this is just an example".

Philipp91 · 2017-04-28T11:59:01Z

src/aead/aead.rs

+//!
+//!    // Usually the salt has some random data and something that relates to the user
+//!    // like an username and it must be 8 bytes (64 bits)
+//!    let salt = [0, 1, 2, 3, 4, 5, 6, 7];


Same: hard-code or not?

Philipp91 · 2017-04-28T11:59:20Z

src/aead/aead.rs

+//!    // like an username and it must be 8 bytes (64 bits)
+//!    let salt = [0, 1, 2, 3, 4, 5, 6, 7];
+//!
+//!    // Keys must have 32 bytes and if you want to use an user password for it you have


nit: a user

Philipp91 · 2017-04-28T12:02:22Z

src/aead/aead.rs

+//!    // to use an algorithm to derive it (in this case pbkdf2).
+//!    let mut key = [0; 32];
+//!    derive(&HMAC_SHA256, 100, &salt, &password[..], &mut key);
+//!


I'd cut off the example here (move the part above elsewhere) and show how to generate a random key here, instead. Many users of other libraries (not ring) fail to generate random keys and hard-code something instead.

Next to the line where the generation happens, you can then explain that the key can also be derived from the user's password or sth. like that and then point to the part of the code above (moved to a second code-block here or to the pbkdf2 module).

Added benefit: this code sample becomes shorter (it's a bit too long and users might skip it for that reason).

I agree with @Philipp91. I don't think we should teach people how to derive a key from a password in this example. This example scheme is actually not as secure as we'd like it to be w.r.t. the nonce handling anyway.

I agree with @Philipp91 that you can just generate a random key in the example, with a comment that indicates that that part of the example is unrealistic.

That was a problem when I started playing with crypto algos and would be nice to say that the password can be converted into keys using derive. Do you think it is worth it to remove thinking that it is a common problem users find when using cryptography for the first time?

Brian wrote:

I don't think we should teach people how to derive a key from a password in this example. This example scheme is actually not as secure as we'd like it to be w.r.t. the nonce handling.

I can't find that comment anymore here. Without understaind his exact reasons (again, I'm not a crypto expert, so I can't judge), I'd say the example code shouldn't demonstrate the derivation if it's not safe.

OTOH: Yes, it is a common problem, and it's common to derive a secret from a password and then encrypt something with it. So eventually, this (upper) part of the sample code will be relevant -- don't throw it away. I hope that ring will soon offer a safe solution to do that? Maybe it has to do with nonce-misuse-resistant ciphers? #413

Once the key derivation sample becomes relevant, it can be debated whether it should be placed here or elsewhere. Either way, I think it should not be in this same block of code.

Philipp91 · 2017-04-28T12:03:27Z

src/aead/aead.rs

+//!
+//!    // The input/output variable need some space to store the tag which will have the
+//!    // signature
+//!    println!("Tag len {}", CHACHA20_POLY1305.tag_len());


Remove this line?

Philipp91 · 2017-04-28T12:04:09Z

src/aead/aead.rs

+//!    }
+//!
+//!    // Opening key used to decrypt data
+//!    let opening_key = OpeningKey::new(&CHACHA20_POLY1305, &key).unwrap();


It is counter-intuitive to initialize the OpeningKey here -- it should be moved behind all the encryption code. In real-world code, this would be in a different function.

I think I didn't follow here. Do you think I should move it even being an example thing?

I think it should be in line 95, because after line 94 you can (mentally or actually) draw a line -- that's where the encryption is complete. The OpeningKey is not needed for the encryption operation, so it should be below that "line".

Philipp91 · 2017-04-28T12:04:56Z

src/aead/aead.rs

+//!    // The input/output variable need some space to store the tag which will have the
+//!    // signature
+//!    println!("Tag len {}", CHACHA20_POLY1305.tag_len());
+//!    for _ in 0..CHACHA20_POLY1305.tag_len() {


`in_out.resize(in_out.len() + CHACHA20_POLY1305.tag_len(), 0u8);

Philipp91 · 2017-04-28T12:09:52Z

src/aead/aead.rs

+//!    // Encrypt data into in_out variable
+//!    let output_size = seal_in_place(&sealing_key, &nonce, &additional_data, &mut in_out,
+//!                                    CHACHA20_POLY1305.tag_len()).unwrap();
+//!


You should use output_size here to .truncate() the ciphertext, which is now in in_out.

Wouldn't I lose the tag? I thought it had the signed data.

The tag's length is not guaranteed to be out_suffix_capacity. So it might be that you resize() the vector from 80 to 100 to make room for the tag, but then it turns out the tag is only 15 long, so your ciphertext is 95. That's the number returned from seal_in_place.

I don't know if that difference actually ever occurs -- it might well be that output_size always matches the size of the vector, so a .truncate() call wouldn't do anything at all. Nevertheless, that's how the API is designed (if I understand it correctly, that is).

So now, AFAIK you wouldn't loose the tag, it's included in that size. Just try out, if the decryption fails, something went wrong, otherwise it's correct.

done in https://github.com/briansmith/ring/pull/526/files#diff-049cb6a3c2f0c7b0a6d0ce40f58461ddR85

Philipp91 · 2017-04-28T12:20:16Z

src/aead/aead.rs

+//!    // Random data must be used only once per encryption and ideally if you encrypt blocks
+//!    // it should be generated on per block see an example of poorly generated nonces
+//!    // where a new nonce is generated at every few pixels at
+//!    // https://github.com/jaysonsantos/bad-encryption-example


This may be true (I'm not a crypto expert), but the explanation is too hard to understand.

I reckon average users wouldn't encrypt every pixel of an image individually, though (as you seem to do here https://github.com/jaysonsantos/bad-encryption-example/blob/master/src/main.rs#L69), but rather the whole thing at once. Similarly, they would encrypt an entire text at once, and not the individual characters. And average users don't have "blocks", they just have their data. So the only explanation needed here is: "Never reuse a nonce, that is, always generate a fresh nonce (e.g. with rand.fill()) for every seal_in_place call you make."

The idea of encrypting pixel by pixel is to have a visual analogy of what would happen if you encrypt many files/something else using the same nonce, I think it illustrates well what could happen because if you are not using with cryptography stuff you simply think that as it is not the secret itself you could keep reusing it

coveralls · 2017-04-28T15:05:33Z

Coverage remained the same at 95.325% when pulling ab4ffb93327eff4af64b0b2a4c97e00c2ee13ab2 on rusty-platter:aead-example into 60867f2 on briansmith:master.

briansmith

Sorry, I deleted the earlier comment about removing the derivation part because I thought it was just "pending".

You can't use PBKDF2 to derive a key from a password and then use that key for AEAD operations without having a secure way to generate nonces. (I don't want to encourage people to use random nonces with these algorithms.)

briansmith · 2017-04-28T18:33:50Z

src/aead/aead.rs

@@ -25,6 +25,82 @@
 //! [AEAD]: http://www-cse.ucsd.edu/~mihir/papers/oem.html
 //! [`crypto.cipher.AEAD`]: https://golang.org/pkg/crypto/cipher/#AEAD

+//!
+//! **Example (if you don't know what you are doing, read this first)**


Please follow the template used in ring::signature's module-level documentation. In particular, this should be:

//! # Example

No need for the "if you don't know what you are doing…" comment.

briansmith · 2017-04-28T18:40:16Z

src/aead/aead.rs

+//!    // to use an algorithm to derive it (in this case pbkdf2).
+//!    let mut key = [0; 32];
+//!    derive(&HMAC_SHA256, 100, &salt, &password[..], &mut key);
+//!


I agree with @Philipp91. I don't think we should teach people how to derive a key from a password in this example. This example scheme is actually not as secure as we'd like it to be w.r.t. the nonce handling anyway.

I agree with @Philipp91 that you can just generate a random key in the example, with a comment that indicates that that part of the example is unrealistic.

briansmith · 2017-04-28T21:44:22Z

src/aead/aead.rs

@@ -25,6 +25,82 @@
 //! [AEAD]: http://www-cse.ucsd.edu/~mihir/papers/oem.html
 //! [`crypto.cipher.AEAD`]: https://golang.org/pkg/crypto/cipher/#AEAD

+//!
+//! **Example (if you don't know what you are doing, read this first)**


Don't try to explain what an AEAD is in this example. We already try to do that and link to a paper about it above.

briansmith · 2017-04-28T21:50:44Z

First, thanks for doing this!

I accidentally submitted some of my draft feedback earlier than I intended to, so then I had to submit more before I intended to. I will review this carefully in a few days.

coveralls · 2017-04-28T22:38:46Z

Coverage remained the same at 95.325% when pulling e61a5f84cf727c5c793b77db223813b32977e856 on rusty-platter:aead-example into 60867f2 on briansmith:master.

jaysonsantos · 2017-04-29T09:18:54Z

@briansmith

You can't use PBKDF2 to derive a key from a password and then use that key for AEAD operations without having a secure way to generate nonces. (I don't want to encourage people to use random nonces with these algorithms.)

I changed it in my code but could you explain why this is a problem? If you derive from a password and generate nonces with SystemRandom isn't it enough? And it seems it is a common thing to do when creating keys from passwords.

jaysonsantos · 2017-04-29T09:23:38Z

I also changed the header to # Example but it will render it literally instead of a and I couldn't find the difference from this to signature's or pbkdf2's docs

coveralls · 2017-04-29T12:06:29Z

Coverage remained the same at 95.325% when pulling 20c9835364bdbfce80abe3f002037e336e4d13e0 on rusty-platter:aead-example into 60867f2 on briansmith:master.

briansmith · 2017-04-29T17:57:27Z

generate nonces with SystemRandom isn't it enoug

The nonces here are only 96 bits, which means random nonce collision probability is 1/2**48 assuming a perfect PRNG. Some people may be OK with that, but I don't want to advocate for something like that in the ring examples.

coveralls · 2017-04-30T16:09:07Z

Coverage remained the same at 95.325% when pulling acf91661d1a0f822b0066500c788fe29c6ffbb86 on rusty-platter:aead-example into 60867f2 on briansmith:master.

isonmad · 2017-05-07T21:58:05Z

src/aead/aead.rs

+//!
+//!    // Usually the salt has some random data and something that relates to the user
+//!    // like a username and it must be 8 bytes (64 bits)
+//!    let salt = [0, 1, 2, 3, 4, 5, 6, 7];


This variable is unused since 20c9835364bdbfce80abe3f002037e336e4d13e0.

coveralls · 2017-05-11T13:23:02Z

Coverage decreased (-0.6%) to 94.761% when pulling a604ebe27955f4d3b5fa36d197bd6cb5a9b9dc04 on rusty-platter:aead-example into 60867f2 on briansmith:master.

isonmad · 2017-05-12T18:14:20Z

src/aead/aead.rs

+//!extern crate ring;
+//!
+//!use ring::aead::*;
+//!use ring::rand::SystemRandom;


Oops, I missed this before. You need something like use ring::rand::{SystemRandom, SecureRandom}; to avoid build failures.

That is true, i saw some buildings that passed and I thought it was working.

And actually, cargo test works on my machine, I will try to figure out before changing it.

You probably will need the thing @isonmad suggested if/when you rebase it on the current master.

jaysonsantos · 2017-05-13T10:55:07Z

I just rebased it with upstream's master

coveralls · 2017-05-13T12:16:30Z

Changes Unknown when pulling dc23d0a on rusty-platter:aead-example into ** on briansmith:master**.

coveralls · 2017-05-13T20:02:44Z

Coverage remained the same at 95.481% when pulling dc23d0a on rusty-platter:aead-example into 0124565 on briansmith:master.

ghost · 2017-05-15T00:27:02Z

Sorry, I didn't see this, and submitted #536.

ghost · 2017-05-15T01:43:06Z

Why are you using CHACHA20_POLY1305 not one of the AES_GCM variants? Are they suspected to be compromised?

jaysonsantos · 2017-05-17T08:08:29Z

@cedenday

Why are you using CHACHA20_POLY1305 not one of the AES_GCM variants? Are they suspected to be compromised?
Because is the one I was using at the time and it is faster on systems that don't have processor instructions for AES.

ghost · 2017-06-10T01:33:15Z

@briansmith What is blocking the acceptance of this pull request?

briansmith · 2017-09-25T05:39:05Z

@briansmith What is blocking the acceptance of this pull request?

The AEAD API is getting replaced real soon now, which will obsolete this. Sorry.

Philipp91 reviewed Apr 28, 2017

View reviewed changes

briansmith reviewed Apr 28, 2017

View reviewed changes

isonmad reviewed May 7, 2017

View reviewed changes

isonmad reviewed May 12, 2017

View reviewed changes

jaysonsantos added 6 commits May 13, 2017 12:54

Add a simple example for aead

5dea9ef

Change some requested stuff from PR

516dec4

Change more of the example for the PR

eaa64d5

Remove strange statement

4a5b5b6

Remove unsused salt

c91b1f9

Add missing trait

dc23d0a

jaysonsantos force-pushed the aead-example branch from a03f62c to dc23d0a Compare May 13, 2017 10:54

briansmith closed this Sep 25, 2017

Add a simple example for aead #526

Add a simple example for aead #526

Conversation

jaysonsantos commented Apr 28, 2017

Philipp91 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

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

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

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

Choose a reason for hiding this comment

coveralls commented Apr 28, 2017 • edited Loading

briansmith 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

briansmith commented Apr 28, 2017

coveralls commented Apr 28, 2017 • edited Loading

jaysonsantos commented Apr 29, 2017 • edited Loading

jaysonsantos commented Apr 29, 2017 • edited Loading

coveralls commented Apr 29, 2017 • edited Loading

briansmith commented Apr 29, 2017

coveralls commented Apr 30, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented May 11, 2017 • edited Loading

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

jaysonsantos commented May 13, 2017

coveralls commented May 13, 2017 • edited Loading

coveralls commented May 13, 2017 • edited Loading

ghost commented May 15, 2017

ghost commented May 15, 2017

jaysonsantos commented May 17, 2017

ghost commented Jun 10, 2017

briansmith commented Sep 25, 2017

coveralls commented Apr 28, 2017 •

edited

Loading

coveralls commented Apr 28, 2017 •

edited

Loading

jaysonsantos commented Apr 29, 2017 •

edited

Loading

jaysonsantos commented Apr 29, 2017 •

edited

Loading

coveralls commented Apr 29, 2017 •

edited

Loading

coveralls commented Apr 30, 2017 •

edited

Loading

coveralls commented May 11, 2017 •

edited

Loading

coveralls commented May 13, 2017 •

edited

Loading

coveralls commented May 13, 2017 •

edited

Loading