Skip to content

themis: Bump PBKDF2 iterations to 314,110#976

Merged
ilammy merged 3 commits into
cossacklabs:masterfrom
ilammy:pbkdf2-adjustment
Feb 5, 2023
Merged

themis: Bump PBKDF2 iterations to 314,110#976
ilammy merged 3 commits into
cossacklabs:masterfrom
ilammy:pbkdf2-adjustment

Conversation

@ilammy
Copy link
Copy Markdown
Collaborator

@ilammy ilammy commented Jan 14, 2023 •
edited
Loading

It's been almost 3 years since this value was revised. Current OWASP recommendations are at least 310,000 iterations for SHA-256, so let's bump it up to be safe.

Checklist

  • Change is covered by automated tests
  • Benchmark results are attached
  • The coding guidelines are followed
  • Public API has proper documentation
  • Changelog is updated

@ilammy
themis: Bump PBKDF2 iterations to 400,000
362f1dd 
It's been almost 3 years since this value was revised. Current OWASP
recommendations are at least 310,000 iterations for SHA-256, so let's
maybe bump it up to 400,000 to be safe.
@ilammy ilammy added the core Themis Core written in C, its packages label Jan 14, 2023
@ilammy ilammy added this to the 0.15.0 milestone Jan 14, 2023
@vixentael
Copy link
Copy Markdown
Contributor

vixentael commented Jan 15, 2023

but it will break compatibility between themis versions

@Lagovas
Copy link
Copy Markdown
Collaborator

Lagovas commented Jan 16, 2023

but it will break compatibility between themis versions

no, it will not. iteration count serialized in ciphertext structure on encryption and deserialized on decryption. This constant specifies how much iterations should be used on every new encryption operations. All previously encrypted contains iteration count and will be decrypted.

Lagovas
Lagovas approved these changes Jan 16, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

lgtm

@ilammy
Copy link
Copy Markdown
Collaborator Author

ilammy commented Jan 16, 2023

but it will break compatibility between themis versions

no, it will not. iteration count serialized in ciphertext structure on encryption and deserialized on decryption

That's the plan 👍

It is not a breaking change, should be transparent to the users. Except for the expected x2 time to encrypt/decrypt. I'd like to do some benchmarks first before merging this (with whatever the new value would be).

G1gg1L3s
G1gg1L3s approved these changes Jan 16, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@G1gg1L3s G1gg1L3s left a comment

Choose a reason for hiding this comment

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

Yeah, moar secuwity!

It would be cool to see some random looking number, for example, totally randomly generated from the first try without any enumeration:

echo -n "oh dear Random Oracle, we request the number of pbkdf2 rounds for Secure Cell that we should use in 2023" | sha256sum | head -c 6
487847

But nevermind :)

@vixentael
Copy link
Copy Markdown
Contributor

vixentael commented Jan 16, 2023

iteration count serialized in ciphertext structure

oh, y'all are the best!! 🧡

@vixentael
Copy link
Copy Markdown
Contributor

vixentael commented Jan 16, 2023

i am not a fan of 2x performance penalty, i suggest using 311002 iterations instead of 400000.

@ilammy
Copy link
Copy Markdown
Collaborator Author

ilammy commented Jan 18, 2023

As expected, most of the cost is in PBKDF and it's proportional to the iteration count.

Sample with n = 1

200,000 iterations:

Secure Cell encryption - Seal, passphrase/64                                                                            
                        time:   [166.01 ms 166.05 ms 166.11 ms]
                        thrpt:  [385.29   B/s 385.42   B/s 385.51   B/s]

Secure Cell decryption - Seal, passphrase/64                                                                            
                        time:   [166.10 ms 166.27 ms 166.57 ms]
                        thrpt:  [384.22   B/s 384.92   B/s 385.31   B/s]

400,000 iterations:

Secure Cell encryption - Seal, passphrase/64                                                                            
                        time:   [333.14 ms 333.61 ms 334.18 ms]
                        thrpt:  [191.52   B/s 191.84   B/s 192.11   B/s]

Secure Cell decryption - Seal, passphrase/64                                                                            
                        time:   [392.68 ms 393.21 ms 393.97 ms]
                        thrpt:  [162.45   B/s 162.76   B/s 162.98   B/s]

3,14159 iterations:

Secure Cell encryption - Seal, passphrase/64                                                                            
                        time:   [262.01 ms 262.51 ms 263.29 ms]
                        thrpt:  [243.08   B/s 243.80   B/s 244.27   B/s]

Secure Cell decryption - Seal, passphrase/64                                                                            
                        time:   [264.99 ms 266.50 ms 268.19 ms]
                        thrpt:  [238.64   B/s 240.15   B/s 241.52   B/s]

Somewhat in the previous measurement of 120 ms for the same machine. New value bumps that to 330 ms. (OpenSSL 3.0 might have been involved in slight degradation.)

@vixentael, how about 314159 iterations then? 😉

@vixentael
Copy link
Copy Markdown
Contributor

vixentael commented Jan 18, 2023

@ilammy 314158

i'm suspicious about the odd (not even) round counts while I appreciate the 🥧.

@ilammy ilammy changed the title themis: Bump PBKDF2 iterations to 400,000 themis: Bump PBKDF2 iterations to 314,110 Jan 20, 2023
@ilammy ilammy merged commit 4e01e4f into cossacklabs:master Feb 5, 2023
@ilammy ilammy deleted the pbkdf2-adjustment branch February 5, 2023 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lagovas Lagovas Lagovas approved these changes

@G1gg1L3s G1gg1L3s G1gg1L3s approved these changes

@vixentael vixentael Awaiting requested review from vixentael vixentael is a code owner

Assignees

No one assigned

Labels

core Themis Core written in C, its packages

Projects

None yet

Milestone

0.15.0

Development

Successfully merging this pull request may close these issues.

4 participants

@ilammy @vixentael @Lagovas @G1gg1L3s