Skip to content

2.7: New password-based authenticated encryption sample program pbcrypt#2472

Closed
gilles-peskine-arm wants to merge 8 commits intoMbed-TLS:mbedtls-2.7from
gilles-peskine-arm:pbcrypt-2.7
Closed

2.7: New password-based authenticated encryption sample program pbcrypt#2472
gilles-peskine-arm wants to merge 8 commits intoMbed-TLS:mbedtls-2.7from
gilles-peskine-arm:pbcrypt-2.7

Conversation

@gilles-peskine-arm
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Feb 26, 2019 •
edited
Loading

This PR introduces programs/cipher/pbcrypt, a new sample program to demonstrate password-based authenticated encryption.

This is meant to illustrate good practice, so please review accordingly. A few caveats:

  • The program works in a specific way: streaming AEAD. Since the library doesn't support streaming CCM, the program doesn't support it.
  • The use of PKCS#12 key derivation is hard-coded because the library doesn't have a generic interface to password-based key derivation.
  • The program should build in all configurations for which the library builds, but it may not run usefully in some “exotic” configurations (e.g. GCM enabled but no block cipher that supports GCM).
  • The program relies on argv and stdio. It does password-based key derivation so it isn't meant for microcontrollers anyway.

Provide a demo usage script and run it from all.sh.

Remove the badly designed aescrypt2. Fix #1906.

This PR is for 2.7. I'll forward-port it to 2.16+ once it's approved. I think this will mean the following changes;

  • Add ChachaPoly to the #ifdef about main and to cipher_is_aead.
  • Replace mbedtls_zeroize by mbedtls_platform_zeroize.
  • Even in development, this is a sample program for the mbedtls API. In the PSA API, there's no password-based key derivation yet and multi-part AEAD is not implemented yet, but key_ladder_demo demonstrates symmetric key derivation and single-part AEAD.

@gilles-peskine-arm gilles-peskine-arm added bug enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces labels Feb 26, 2019
@gilles-peskine-arm gilles-peskine-arm force-pushed the pbcrypt-2.7 branch 2 times, most recently from 5b98db7 to 78ea903 Compare February 27, 2019 16:30
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Jun 14, 2019
Draft
@gilles-peskine-arm gilles-peskine-arm force-pushed the pbcrypt-2.7 branch 2 times, most recently from d538625 to 593f68e Compare April 3, 2020 09:36
@mpg
Copy link
Copy Markdown
Contributor

mpg commented Apr 23, 2020

@gilles-peskine-arm There appears to be a significant intersection with #2698 - adding sample usage scripts for programs and testing them in the CI. Does that mean the two PRs conflicts with each other? If that's the case, I'd suggest giving priority to #2698 and labeling this one as "needs: preceding PR".

@gilles-peskine-arm
New sample program pbcrypt
54bc79e 
Sample program for authenticated encryption using a key derived from a
password.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Add sample usage script with pbcrypt
82128a6 
Test it in all.sh.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Remove the sample program aescrypt2
88c8824 
The sample program aescrypt2 shows bad practice: hand-rolled CBC
implementation, CBC+HMAC for AEAD, hand-rolled iterated SHA-2 for key
stretching, no algorithm agility. The new sample program pbcrypt does
the same thing, but better. So remove aescrypt2.

Fix Mbed-TLS#1906

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Update Visual Studio files for aescrypt2 replacement by pbcrypt
d12011d 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Add changelog entry for aescrypt2 replacement by pbcrypt
66bb361 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Declare dependencies of the demo script
143bf4d 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Copy link
Copy Markdown
Contributor Author

gilles-peskine-arm commented Apr 23, 2020

The intersection was only adding something to all.sh. I've removed that part: #2698 is a better place for it since that's its main job. Now the PRs are fully independent, except insofar as the format of pbcrypt_demo.sh only makes full sense in combination with #2698.

@mpg
Copy link
Copy Markdown
Contributor

mpg commented Apr 23, 2020

Ok, thanks!

gilles-peskine-arm
gilles-peskine-arm commented Apr 23, 2020
View reviewed changes
programs/cipher/pbcrypt_demo.sh
@@ -0,0 +1,87 @@
#!/bin/sh
set -e -u
Copy link
Copy Markdown
Contributor Author

@gilles-peskine-arm gilles-peskine-arm Apr 23, 2020

Choose a reason for hiding this comment

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

When #2698 is ready and backported, include demo_common.sh and adapt the code here accordingly.

mpg
mpg reviewed Apr 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

I've made a design review and I'm happy with the design. In particular, using a temporary file avoids releasing possibly unauthentic plaintext, which would otherwise be a risk with streaming AEAD decryption.

dgreen-arm
dgreen-arm reviewed Apr 27, 2020
View reviewed changes
programs/cipher/pbcrypt.c Outdated
(unsigned long long) header[7] ),
payload_size <= SIZE_MAX,
"Payload too large" );
metadata->payload_size = payload_size;
Copy link
Copy Markdown
Contributor

@dgreen-arm dgreen-arm Apr 27, 2020

Choose a reason for hiding this comment

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

This line is causing failures on Windows due to the conversion from unsigned long long to size_t. Given the check above it should be fine, but the compiler is complaining about it.

@gilles-peskine-arm
Make unsigned long long to size_t cast explicit
8223cb7 
This is a cast to a smaller type on 32-bit platforms. It's ok because
the size was checked just above, but Visual Studio wants the cast to
be explicit.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Make size_t to int cast explicit
c1cfbd1 
This cast is needed because mbedtls_cipher_setkey uses int instead of
size_t for the key size. It's safe because the key size fits in an
int. Visual Studio wants the cast to be explicit.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm changed the title Backport 2.7: New password-based authenticated encryption sample program pbcrypt 2.7: New password-based authenticated encryption sample program pbcrypt Jan 6, 2021
@gilles-peskine-arm
Copy link
Copy Markdown
Contributor Author

gilles-peskine-arm commented Mar 10, 2021

Due to the imminent retirement of 2.7, I am closing this pull request. I have ported the program to 2.16 in #4216. Compared to the 2.7 version, I added ChaChaPoly support and fixed a couple of minor bugs, all in new commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mpg mpg mpg left review comments

@yanesca yanesca Awaiting requested review from yanesca

+1 more reviewer

@dgreen-arm dgreen-arm dgreen-arm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug component-crypto Crypto primitives and low-level interfaces enhancement needs-review Every commit must be reviewed by at least two team members,

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gilles-peskine-arm @mpg @dgreen-arm