[RFC] Libsodium - Discussion View as Markdown

Hey Scott,

Hi PHP Internals Team,

Let's begin discussing the prospect of adding libsodium as a core
extension
in PHP 7.1. I've updated the RFC to explain why this would be a good idea
and the benefits it offers.

https://wiki.php.net/rfc/libsodium

If the subsequent discussion goes smoothly, I would like to open voting
on
June 15.

Together, let's make PHP cryptography so safe that it becomes boring.

First, thanks for providing better alternatives to crypto in PHP!

I also agree with Remi on naming: let's avoid calling the extension
libsodium.

I have some concerns that are just about code quality, not about
functionality. Consider that I didn't look at the underlying library (and I
really care little about it, from a consumer perspective).

1. is there a particular reason why abbreviations are used? For
   instance, why sodium_randombytes_buf() instead of
   sodium_random_bytes_buffer()?
2. from a naming perspective, I'd expect sodium_randombytes_buf() to
   give me a buffer of random bytes (probably as a stream), but it returns the
   actual string of random bytes. Again: confusing naming
3. can we avoid using "themed" naming? For example, instead of
   sodium_crypto_secretbox(), it would be best to express what it actually
   does, like sodium_encrypt_and_sign(). While the naming may be emerging
   from lower layers, I still (like I did with other RFCs) disagree with
   inheriting confusing naming. This will just cause users to look up the
   naming up when reading or writing code, and ultimately add up to silly
   bugs. I can already foresee that people will use the API incorrectly just
   because of the naming.
4. can't we just keep it namespaced under Sodium, instead of adding
   more stuff to the root level namespace? Does anyone have a reference to the
   coding standards that would cause the rename?

Cheers,

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

​I'd love to just keep the namespace personally
​ (
Ke
​eping \Sodium\foo() and \SODIUM\FOO means code I've written today will
work in 7.1 for non-PECL users
​, and less work we thrust on Frank Denis)​
​
but it was previously expressed that doing so violates the coding standard.
​ Changing to sodium_* would mean less bikeshedding and automatic "No"
votes.

Weird... I guess we could add a subsection to the vote?

As for the function names, that's what they were called in NaCl.
https://nacl.cr.yp.to/secretbox.html

I believe randombytes_buf() was named in a similar spirit to OpenBSD's
arc4random_buf().

Yeh, that is software archaeology though, not software design ;-)

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

I also agree with Remi on naming: let's avoid calling the extension
libsodium.

I agree here too.

1. is there a particular reason why abbreviations are used? For
   instance,
   why sodium_randombytes_buf() instead of sodium_random_bytes_buffer()?
2. from a naming perspective, I'd expect sodium_randombytes_buf() to
   give me a buffer of random bytes (probably as a stream), but it returns
   the
   actual string of random bytes. Again: confusing naming
3. can we avoid using "themed" naming? For example, instead of
   sodium_crypto_secretbox(), it would be best to express what it actually
   does, like sodium_encrypt_and_sign(). While the naming may be emerging
   from lower layers, I still (like I did with other RFCs) disagree with
   inheriting confusing naming. This will just cause users to look up the
   naming up when reading or writing code, and ultimately add up to silly
   bugs. I can already foresee that people will use the API incorrectly just
   because of the naming.

I agree here too but read on.

4. can't we just keep it namespaced under Sodium, instead of adding
   more
   stuff to the root level namespace? Does anyone have a reference to the
   coding standards that would cause the rename?

I was the person who brought this up because it is not desired according
to the existing CODING STANDARD:

https://github.com/php/php-src/blob/master/CODING_STANDARDS

Note that it also encourages this weird C style naming with
abbreviations, hence, I would be open for discussing it. That being
said, I am not a friend of putting procedural functions into namespaces
and prefer the establish prefix approach.

That being said, I see many opportunities here to create very nice
classes that enable dependency injection, single validation, and of
course I am +1000 for namespaces here. It would also help a lot to get
some of those extremely weird names out of the window.

namespace Sodium;

interface SodiumException extends \Throwable {}

class SignatureException
extends \UnexpectedValueException
implements SodiumException {}

class DetachedSignature {

public function __construct(string $detached_signature);

public function __toString(): string;

public function verify(string $message, PublicKey $public): bool;

}

class SignedMessage {

public function __construct(string $signed_message);

public function __toString(): string;

public function getSignature(): DetachedSignature;

}

interface Key {

}

class PrivateKey implements Key {

public function sign(string $message): SignedMessage;

}

class PublicKey implements Key {

public function verify(SignedMessage $message): bool;

}

class KeyPair {

public function __construct(PrivateKey $private, PublicKey $public);

public static function generate(): KeyPair;

public function getPrivate(): PrivateKey;

public function getPublic(): PublicKey;

}

This is of course an attempt of writing up some classes after looking at
the API for literally 5 minutes but I think it illustrated the
potential. It is also going to increase the security by a huge margin
because a private key suddenly has to be an instance of a PrivateKey and
not some arbitrary string that needs to be revalidated all the time.

The same software design principles apply as always and the current API
might be nice for C but it is definitely not for PHP in my opinion.

Of course I offer my help to find and define such an API if you guys are
interested in creating one. :)

--
Richard "Fleshgrinder" Fussenegger

​Well, for what it's worth, I did write https://github.com/paragonie/halite
​ as a high-level abstraction.

The goal of this RFC is to get the lower-level functionality (which is
still, I promise, a high-level cryptography API) available.​

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises https://paragonie.com/​

Fleshgrinder php@fleshgrinder.com schrieb am Mi., 1. Juni 2016 19:26:

I also agree with Remi on naming: let's avoid calling the extension
libsodium.

I agree here too.

1. is there a particular reason why abbreviations are used? For
   instance,
   why sodium_randombytes_buf() instead of sodium_random_bytes_buffer()?
2. from a naming perspective, I'd expect sodium_randombytes_buf() to
   give me a buffer of random bytes (probably as a stream), but it returns
   the
   actual string of random bytes. Again: confusing naming
3. can we avoid using "themed" naming? For example, instead of
   sodium_crypto_secretbox(), it would be best to express what it actually
   does, like sodium_encrypt_and_sign(). While the naming may be emerging
   from lower layers, I still (like I did with other RFCs) disagree with
   inheriting confusing naming. This will just cause users to look up the
   naming up when reading or writing code, and ultimately add up to silly
   bugs. I can already foresee that people will use the API incorrectly just
   because of the naming.

I agree here too but read on.

4. can't we just keep it namespaced under Sodium, instead of adding
   more
   stuff to the root level namespace? Does anyone have a reference to the
   coding standards that would cause the rename?

I was the person who brought this up because it is not desired according
to the existing CODING STANDARD:

https://github.com/php/php-src/blob/master/CODING_STANDARDS

Note that it also encourages this weird C style naming with
abbreviations, hence, I would be open for discussing it. That being
said, I am not a friend of putting procedural functions into namespaces
and prefer the establish prefix approach.

That being said, I see many opportunities here to create very nice
classes that enable dependency injection, single validation, and of
course I am +1000 for namespaces here. It would also help a lot to get
some of those extremely weird names out of the window.

namespace Sodium;

interface SodiumException extends \Throwable {}

Why does it directly extend throwable?

class SignatureException

extends \UnexpectedValueException
implements SodiumException {}

class DetachedSignature {

public function __construct(string $detached_signature);

public function __toString(): string;

public function verify(string $message, PublicKey $public): bool;

}

class SignedMessage {

public function __construct(string $signed_message);

public function __toString(): string;

public function getSignature(): DetachedSignature;

}

interface Key {

}

class PrivateKey implements Key {

public function sign(string $message): SignedMessage;

}

class PublicKey implements Key {

public function verify(SignedMessage $message): bool;

}

Just a short node: the keys shouldn't be responsible for signing /
verification.

class KeyPair {

public function __construct(PrivateKey $private, PublicKey $public);

public static function generate(): KeyPair;

public function getPrivate(): PrivateKey;

public function getPublic(): PublicKey;

}

This is of course an attempt of writing up some classes after looking at
the API for literally 5 minutes but I think it illustrated the
potential. It is also going to increase the security by a huge margin
because a private key suddenly has to be an instance of a PrivateKey and
not some arbitrary string that needs to be revalidated all the time.

The same software design principles apply as always and the current API
might be nice for C but it is definitely not for PHP in my opinion.

Of course I offer my help to find and define such an API if you guys are
interested in creating one. :)

--
Richard "Fleshgrinder" Fussenegger

I also agree with Remi on naming: let's avoid calling the extension
libsodium.

1. is there a particular reason why abbreviations are used? For instance,
   why sodium_randombytes_buf() instead of sodium_random_bytes_buffer()?
2. from a naming perspective, I'd expect sodium_randombytes_buf() to
   give me a buffer of random bytes (probably as a stream), but it returns the
   actual string of random bytes. Again: confusing naming
3. can we avoid using "themed" naming? For example, instead of
   sodium_crypto_secretbox(), it would be best to express what it actually
   does, like sodium_encrypt_and_sign(). While the naming may be emerging
   from lower layers, I still (like I did with other RFCs) disagree with
   inheriting confusing naming. This will just cause users to look up the
   naming up when reading or writing code, and ultimately add up to silly
   bugs. I can already foresee that people will use the API incorrectly just
   because of the naming.
   I agree here too but read on.

4. can't we just keep it namespaced under Sodium, instead of adding more
   stuff to the root level namespace? Does anyone have a reference to the
   coding standards that would cause the rename?

I was the person who brought this up because it is not desired according
to the existing CODING STANDARD:

https://github.com/php/php-src/blob/master/CODING_STANDARDS

Hi. https://github.com/php/php-src/commit/aa203477bc24b1fadc16d65533c1749162260592
was my commit, put together as a result of discussions on this list
(and sidebars). I can try to speak to it from memory.

Note that it also encourages this weird C style naming with
abbreviations, hence, I would be open for discussing it.

It was 2000, 16 years ago.

WRT: "C style naming": C style PHP, was a big part of the PHP
adoption base in 2000. No objects. No inheritance. No classes. No
namespaces. Folks were also coming to PHP from ColdFusion, from Perl,
from JavaScript, and bringing their language conventions with them.
Without many guide rails, there was a weird mix of CreaTive_nameing
(and spelling). Conventions like verbnoun, Noun_Verb,
parent_Verb_noun, Parent(Noun())....(and most imaginiable
permutations) were sprinkled throughout the code, throughout
documentation, throughout the community... and one of the earliest
public complaints about PHP was about about inconsitent function
naming. The goal was to bend the curve, over the years, to bring some
order and consistency, not just to internal function naming (as in
this case), but to the entire PHP ecosystem. Thus, the standards about
parent naming, about underscores and....

...Abbreviation: In 2000, most PHP code was typed, manually, No IDE
completion, no built-in syntax checking, no code generators, every
single keystoke was a human hammering away in emacs/vi, BBedit,
Notepad (etc.), and longer function names tended to decrease code
quality (due to increased human error potential), and take up valuable
screen space (800x600 pixels on emergency terminals, though a nice
developer setup could have had 1024x768 pixels on a screen, all at
the same time.) Coding houses still had rules like "maximum of 80
characters per line", to encourage brevity. Abbreviation in this
environment made for more compact code, with less typing errors, but
came with the costs of inconsistent abbreviations, and decreased
readability.

In 2016, using an editor with built in syntax highlighting,
auto-completion, while at coding desks measuring in thousands of
pixels across multiple screens, it seems a bit dated, but when
debugging text files with vi on a spare monochrome terminal session,
late night over a modem at 3,600 Kbps, those things mattered a great
deal. For folks who are still using modems, terminals, and updating
their PHP on 800 pixel screens, I imagine they might still matter.

-Ronabop

• memzero, memcmp, hex2bin

I am not totally convinced that memzero and maybe memcmp names are
good nor they should be there. Both would be very useful as operator
on variables. Given the simplicity of the implementations, it could be
very useful in many other areas in case this ext is not installed

IMO: memzero is fine; memcmp isn't that great.

memzero() stands out as unusual and interesting because PHP scripts
don't usually get to manipulate memory, only variables. From the "Using"
guide

 void \Sodium\memzero(&string $secret);

it looks like it's for zeroing strings.

What arg types does memzero() work with?

Does it check argument type?

Is it safe to use with opcache? Interned strings is an interesting case
but there might be others.

Tom

The stated goal is "You shouldn't need a Ph.D in Applied Cryptography to
build a secure web application." I fully agree with this goal. I however
feel that current implementation, while making admirable progress
towards this goal, still needs some work to actually achieve it.

I fully agree with you. As much as I think we need something like that, I
think these are stopping points.

I would very interested to hear from Scott about these questions and the
low level nature of the APIs make it not as friendly or future proof as it
could.

Cheers
Pierre

Hi Pierre,

My position on the low level nature of libsodium's APIs is as follows:
​That sounds like a call to action for https://wiki.php.net/rfc/php71-crypto
rather than a point of concern for adopting libsodium.​

Compare the following two snippets which accomplish the same "goal"
(anonymous public-key encryption).

<?php
/**
 * OpenSSL -- since the Diffie Hellman features in ext/openssl kind
 * of suck, I'm going to use RSA to encrypt the AES key using the
 * recipient's public key.
 */

## ENCRYPTION ##

$message = 'Prime Numbers Rock!';
$publicKey = openssl_pkey_get_public('file://path/to/public_key.pem');

$aesKey = random_bytes(32);
// Basically a poor-man's HKDF by just using HMAC
    $keyE = hash_hmac('sha256', 'Encryption Key', $aesKey, true);
    $keyA = hash_hmac('sha256', 'Authentication Key', $aesKey, true);

$iv = random_bytes(16);
$ciphertext = openssl_encrypt($message, 'aes-256-ctr', $keyE,

OPENSSL_RAW_DATA, $iv);
$mac = hash_hmac('sha256', $iv . $ciphertext, $keyA, true);

$combined = $mac . $iv . $ciphertext;
$rsaCipher = '';
openssl_public_encrypt($aesKey, $rsaCipher, $publicKey,

OPENSSL_PKCS1_OAEP_PADDING);
$sendMe = $rsaCipher . $combined;

## DECRYPTION ##

$privateKey = openssl_pkey_get_public('file://path/to/private_key.pem');
$rsaPart = mb_substr($sendMe, 0, 256, '8bit'); // Assuming 2048-bit RSA
$aesPart = mb_substr($sendMe, 256, null, '8bit');
$mac = mb_substr($aesPart, 0, 32, '8bit');
$iv = mb_substr($aesPart, 32, 16, '8bit');
$cipher = mb_substr($aesPart, 48, null, '8bit');

openssl_private_decrypt($rsaPart, $aesKey, $privateKey,

OPENSSL_PKCS1_OAEP_PADDING);
$keyE = hash_hmac('sha256', 'Encryption Key', $aesKey, true);
$keyA = hash_hmac('sha256', 'Authentication Key', $aesKey, true);

$calc = hash_hmac('sha256', $iv . $cipher, $keyA, true);
if (!hash_equals($calc, $mac)) {
    throw new Exception('MAC validation failure');
}

$decrypted = openssl_decrypt($cipher, 'aes-256-ctr', $keyE,

OPENSSL_RAW_DATA, $iv);
var_dump($decrypted); // string(19) "Prime Numbers Rock!"

​Can you count the foot-bullets in that snippet that you'd need to be a
cryptography engineer to successfully avoid?

Demo: https://3v4l.org/nYVPf

Here's a congruent implementation in libsodium:​

​ <?php
/**
* Libsodium
*/

## ENCRYPTION ##

$message = 'Prime Numbers Rock!';
$bob_public_key = "... populate here ...";
​
$nonce = random_bytes(24);
$sendMe = \Sodium\crypto_box_seal($message, $bob_public_key);

​ ## DECRYPTION ##​

​ ​$bob_kp = "... populate here ...";
$decrypted = \Sodium\crypto_box_seal_open($sendMe, $bob_kp);
var_dump($decrypted); // string(19) "Prime Numbers Rock!"

(No demo available, as 3v4l doesn't have ext/sodium installed.)

​Libsodium already ​knocks it out of the park compared to OpenSSL and
Mcrypt. If we want to talk about a higher-level abstraction-- such as
what's provided by paragonie/EasyRSA + defuse/php-encryption or
paragonie/halite-- I wholeheartedly endorse that discussion. But I don't
think we should try to solve that problem with this particular RFC.

In closing, I don't disagree that a simple crypto API is a good goal to
have. I just think the ideal you're discussing is:

A. Out of scope, and
B. Kind of belittling to how much of an improvement libsodium is to what we
already have.

Further reading: http://framework.zend.com/security/advisory/ZF2015-10

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises https://paragonie.com/
​

Scott Arciszewski scott@paragonie.com schrieb am So., 5. Juni 2016 10:13:

On Sat, Jun 4, 2016 at 6:15 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:

Hi!

Let's begin discussing the prospect of adding libsodium as a core
extension
in PHP 7.1. I've updated the RFC to explain why this would be a good
idea
and the benefits it offers.

https://wiki.php.net/rfc/libsodium

If the subsequent discussion goes smoothly, I would like to open
voting on
June 15.

Some notes based on the docs in
https://paragonie.com/book/pecl-libsodium:
TLDR: I think the idea of having something like this in core is very
nice, but I think it needs some tweaks in order to be a solution we can
recommend as core module.

• I would appreciate deeper namespacing. It is rarely that you would use
  all sub-modules (such as encryption, both symmetric and asymmetric,
  password hashing, decryption, random, etc.) in the same piece of code.
  So if I could just "use Sodium\Crypto;" and then use short names it
  would be much nicer than having to spell out the whole story.

This would entail a BC break against all software currently written
using libsodium.
Are you certain that deeper namespacing would be worth that trade-off?

• Also, I'm not sure why functions like
  crypto_aead_chacha20poly1305_encrypt() exist. Shouldn't algorythm be a
  parameter to encryption function and not function name? Putting
  parameter into function names makes it much harder to be flexible and
  configurable. Or, if there's only one, then why not just call it
  encrypt() and state the fixed algorithm in the docs?

Libsodium offers few primitives. For AEAD, you have:

• ChaCha20-Poly1305
• AES-256-GCM (but only with modern hardware)
• ChaCha20-Poly1305 (IETF version with a bigger nonce)

These are distinct functions in the C API to discourage "Hey, I'll add
another algorithm and users can access it by changing the string
that's passed".

When all is said and done, crypto_aead_encrypt() and
crypto_aead_decrypt() will be the CAESAR winner.
https://competitions.cr.yp.to/caesar.html

• The function names in general are kind of all over the place. I.e., to
  get a random, you get:
• for string: _buf
• for ranged (I assume unsigned?) integer: _uniform
• for 16-bit unsigned integer: _random16
  If you can notice a pattern here, I can't. Also, I don't see why you
  have a function for 16-bit int but not for 8-bit or 32-bit. Additionally
  weird that _uniform gets int, but has limit different from PHP int.

To be honest, most of these aren't necessary in PHP anymore. They were
mostly useful in PHP 5.x before we had random_bytes() and
random_int().

• Naming/namespacing of constant-time function should make it clear they
  are there because they are constant time, not just because somebody
  wanted to reimplement bin2hex. It's also unclear why we need memcmp as
  separate function from hash_equals().

Alternatively, if we can make bin2hex() and hex2bin() always
constant-time in PHP, we can eschew exposing these libsodium
functions. (Just make an alias for BC purposes.) I'd actually prefer
that.

Can't we have the aliases in userland for BC? I don't see a reason to
introduce aliases that are automatically deprecated.

Same for all other functions. If they're namespaced, a wrapper can be
created for BC. And we can have nice names in PHP.

Also, I'm still not convinced bin2hex in PHP even has a timing problem.

I haven't seen anything but vague generic theoretising in this regard.
Same with hex2bin (in fact, even more since hex2bin doesn't even have
index lookups).

These are the facts:

1. Index lookups based on the contents cryptographic secrets is a
   recipe for microarchitecture side-channels (e.g. FLUSH+RELOAD).
2. The current implementation of all RFC 4648 encoding functions that
   PHP has (which, strangely, doesn't include base32) violates rule 1.

https://cryptocoding.net/index.php/Coding_rules#Avoid_table_look-ups_indexed_by_secret_data

See how Halite stores keys for an example of "Yes, this does get used
for crypto secrets":

https://github.com/paragonie/halite/blob/6c026f7dc6a57ecd6c65e5944057acdefa8c9d67/src/KeyFactory.php#L604-L627

• What exactly memzero does in PHP? Looks like it accepts argument
  by-ref, which means if you do something like:

It overwrites every byte in a string with NUL characters.

function foo($secret_key) {
// do stuff with key
memzero($secret_key);
}

and then:

$key = get_key(); foo($key);

then $key still has the key. So I'm not sure how that all memory wiping
is still working in practice. I mean, it may work with random keys that
you generate, use once and immediately destroy, but in any other
scenario it's just wasting time. Even with random keys it's iffy since
it assumes your RNG state is either not in memory or is protected.

Right, it has to be used with care. But the existence of a
well-written cross-platform memory-zeroing function is almost reason
enough to vote in favor of libsodium.

See also: https://stackoverflow.com/a/29331937/2224584

E.g. in the example at

https://paragonie.com/book/pecl-libsodium/read/09-recipes.md#encrypted-cookies

keys are meticulously erased, but since they are all deterministically
derived from $this->key (and cookie name which is public) which stays in
memory, what exactly is the point?

That's a good question:

http://www.daemonology.net/blog/2014-09-06-zeroing-buffers-is-insufficient.html

• increment() needs clearer explanation what it does (e.g. does it
  accept any string? What exactly does it do to that string?) and also it
  seems to be it would be nicer to just return the value. Or at least have
  this option - mutation in place is not always what you want.

It's used for incrementing a nonce for, e.g. CTR mode. In little-endian.

That's a design issue that should be relatively non-controversial if
you want to raise it:
https://github.com/jedisct1/libsodium-php

• Why version functions are functions and not constants?

• Names like "secretbox" are weird to me - what exactly is "secretbox" -
  is it encryption? Decryption? hashing? Is it symmetric or asymmetric? It
  would be nicer to use established and more clear terminology. Same with
  other names - e.g. crypto_auth - what it does? Does it check the message
  for authenticity or generates the signature for somebody else to check?
  Turns out the latter, but it's not clear from the name.

See http://nacl.cr.yp.to

• Same with crypto_box() - actually it differs from crypto_secretbox()
  by... try to guess what. You would never guess. The former is
  asymmetric, the latter is symmetric.

The origins are academic. All you need to know is:

• crypto_box means public key encryption
• crypto_secretbox means secret key encryption

• It's not clear why keypair parameter is string - if it's two keys, how
  it is one string? Also, why you need a keypair to encrypt-decrypt if
  you'd use only one key in each case (public or private)? What you do if
  you only have public key - how would you decrypt a message? Can you
  decrypt a message at all if you don't have secret key - and if so, how?

• crypto_box_keypair_from_secretkey_and_publickey seems to use secret
  and public key from different people, so the result is not what is
  usually called a key pair (set of public+private keys belonging to one
  user) - in fact, I don't know what the result is. Could you explain?

A keypair is used in some APIs (crypto_box_seal_open comes to mind).
All that function does is concatenate the two.

It has nothing to do with key exchange.

• After looking into crypto_sign_open I understands that it separates
  the message from attached signature and verifies the signature. But it's
  a bit confusing as it doesn't actually open anything.

crypto_sign doesn't do detached signatures. You take a signed message
and a public key and, if the signature is good, you got the unsigned
message.

I personally prefer crypto_sign_detached for that reason.

• It is unclear why you need different functions to generate encryption
  keypair and signing keypair.

Different algorithms. One is Elliptic Curve Diffie Hellman, the other
is DSA over an Edwards Curve. You can transform keys from one to the
other, but using one in both places without conversion is something I
would encourage great caution.

• The API never seems to either document or allow any choice of the
  algorithms.

Yes, as mentioned above, that is INTENTIONAL.

Libsodium is an opinionated cryptography library that doesn't give you
a lot of levers to pull -- only secure defaults you can't change,
consisting of carefully-selected cryptography primitives.

While it can be a boon for simplicity, I do not see how such
code could be made to interoperate with code in other languages and
other systems.

That isn't its purpose. Its purpose is to be secure. (It also happens
to also be extremely fast.)

I mean, I can generate the signature easily, but how
would I check such signature in, say, Python or Java?

By using Libsodium. There are bindings for most popular programming
languages already.

The docs give no clue. It's OK to support only one algorithm if it's
considered superior (though what the users would do if it turns out
not to be the case?) but at least it should be thoroughly documented.

https://download.libsodium.org/libsodium/content/public-key_cryptography/authenticated_encryption.html#algorithm-details

• If crypto_pwhash_str highly recommends using provided constants, why
  not make these parameters optional with defaults being recommended
  constants?

• crypto_pwhash_scryptsalsa208sha256 looks awfully specific (and long).
  Is that the only algorithm supported? If yes, do we need to spell it out
  in the name (and not the docs)? If not, why it's not a parameter?

That's the long-form of "scrypt". Personally, I'd say exclude it and
just use crypto_pwhash (Argon2i). No disrespect to Colin Percival.

• Is there any support for importing/exporting keys in any of the
  established formats? Is the library inter-operable with any of the
  existing key/data interchange formats?

Keys are raw binary strings, unless you use a high level function.

• Function names like crypto_sign_ed25519_sk_to_curve25519 seem to not
  be very usable to common people. The docs says "Transform crypto_sign
  key into crypto_box key" - maybe the name should reflect it?

These are auxiliary functions, not mainline functions. Most PHP
developers will only be using:

• crypto_box
• crypto_secretbox
• crypto_sign
• crypto_auth
• crypto_generichash
• crypto_pwhash

Names like crypto_stream_xor have a similar problem - what it does is
encrypts stream with a key, according to docs. Looks like it uses XOR
for it, but is it the most important part for the user or the fact it
encrypts? Maybe it should be crypto_stream_encrypt or something? Also,
does it really do just XOR? Because XOR is not really the best idea to
do even if you don't care about authenticity - due to trivial chosen
plaintext attack. Probably it does more than mere XOR, but then I'm not
sure what exactly.

This is how a stream cipher works:

1. You take your key and nonce, and generate a long string of random
   bytes deterministically from your key and nonce.
2. To encrypt: You XOR every byte of plaintext with a corresponding
   byte of the keystream.
3. To decrypt: You XOR every byte of ciphertext with a corresponding
   byte of the keystream.

This is unauthenticated encryption; don't use it unless you're
building something with it and authenticate elsewhere.

• Why one would use crypto_scalarmult? That looks like a random piece of
  low level API without clear indication why it exists.

It's used to derive a shared secret from an X25519 secret key and an
X25519 public key.

• What is the difference between crypto_stream and randombytes_buf? Both
  seem to produce a random string of given length. I assume crypto_stream
  does more but the docs do not explain.

crypto_stream is deterministic; randombytes_buf is non-deterministic

• For crypto_kx, it is not clear what exactly this function does and why
  the same parameter is passed twice in different places. I mean, if you
  know the details of how DH key exchange works, it may make sense, but if
  we want to make it simple for the common user, it needs some better
  explanations and maybe better parameters.

This is basically a multi-step crypto_scalarmult in one complete pacakge.

• Names like CRYPTO_PWHASH_SCRYPTSALSA208SHA256_OPSLIMIT_INTERACTIVE
  look very painful. Yes, people have IDEs and copy-paste, but imaging to
  have to type it just once over ssh connection in vim, where copy-paste
  is not working. Can't we have something more concise and put the rest in
  the docs?

That's a point worth considering. (Alternatively: Eschew scrypt.)

This came out longer than I expected (hopefully somebody made this far?)
and more disorganised, so I provide a quick summary:

• Need better naming, both namespaces and functions
• Need better docs explaining what the functions do and why

The docs are currently my responsibility; I apologize for not putting
more time into them.

The stated goal is "You shouldn't need a Ph.D in Applied Cryptography to
build a secure web application." I fully agree with this goal. I however
feel that current implementation, while making admirable progress
towards this goal, still needs some work to actually achieve it.

--
Stas Malyshev
smalyshev@gmail.com

I hope I've addressed your important questions adequately.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

Hi!

This would entail a BC break against all software currently written
using libsodium.
Are you certain that deeper namespacing would be worth that trade-off?

This is certainly a concern. But this is solvable - we can have aliases,
for example, that would be compiled only in PECL build.

Libsodium offers few primitives. For AEAD, you have:

• ChaCha20-Poly1305
• AES-256-GCM (but only with modern hardware)
• ChaCha20-Poly1305 (IETF version with a bigger nonce)

These are distinct functions in the C API to discourage "Hey, I'll add
another algorithm and users can access it by changing the string
that's passed".

I think somehow separating primitives from recommended API would be
useful. Or maybe it's just documenting...

When all is said and done, crypto_aead_encrypt() and
crypto_aead_decrypt() will be the CAESAR winner.
https://competitions.cr.yp.to/caesar.html

That seems to be 1.5 years from now, so what the user would do now?

To be honest, most of these aren't necessary in PHP anymore. They were
mostly useful in PHP 5.x before we had random_bytes() and
random_int().

OK, then I assume we can mark those as something like "use
random_bytes/random_int in PHP 7.x"?

Alternatively, if we can make bin2hex() and hex2bin() always
constant-time in PHP

I think you mean time not dependent on argument content, but only on
length?

These are the facts:

1. Index lookups based on the contents cryptographic secrets is a
   recipe for microarchitecture side-channels (e.g. FLUSH+RELOAD).

That's the theory, and even that part of theory only applies to bin2hex,
as far as I can see. It also assumes lookups within 16 bytes are
distinguishable - and not just distinguishable but distinguishable
enough so that you can see them through layers of overhead of PHP, PHP
code, server infrastructure, etc., which sounds doubtful for me on a
modern architecture.
For hex2bin, even that doubtful theory seems to be not applicable.

2. The current implementation of all RFC 4648 encoding functions that
   PHP has (which, strangely, doesn't include base32) violates rule 1.

https://cryptocoding.net/index.php/Coding_rules#Avoid_table_look-ups_indexed_by_secret_data

That again is theory, as above. It says it depends on cache hit/miss.
Line size in most common Intel processor is 64 bytes, on ARM IIRC it's
32 bytes, while hexconvtab is 16 bytes long. So I'm not sure how would
you get any cache hit/miss sensitivity here. Am I missing something?

Right, it has to be used with care. But the existence of a
well-written cross-platform memory-zeroing function is almost reason
enough to vote in favor of libsodium.

The problem is that no amount of care would actually help in a common
scenario. Due to PHP's copy-on-write semantics, it just wouldn't do what
you think it would do, in most cases. The only way it would work if you
ensure no reference to this variable ever exists throughout the code
except for one passed to memzero. And this is usually not easy to
achieve. So I think you overestimate the usefulness of it,
unfortunately. It might work for temporary keys that are created and
destroyed in the same function without storing them anywhere, but in
most scenarios these keys are derived from stored keys anyway.

That's a good question:
http://www.daemonology.net/blog/2014-09-06-zeroing-buffers-is-insufficient.html

"Insufficient" is kind of misleading there - it's not merely
"insufficient", it is not adding practically anything, the level of
compromise is the same before and after.

It's used for incrementing a nonce for, e.g. CTR mode. In little-endian.

You mean treat the whole string as little-endian representation of the
arbitrary-sized integer and increment that integer and return the
result? What if the size of the resulting integer is larger - does it
reallocate or overflow? Should be documented.

See http://nacl.cr.yp.to

Are we bound by naming decisions of this project? Maybe, then we need
some good explanation of what they actually mean.

• crypto_box_keypair_from_secretkey_and_publickey seems to use secret
  and public key from different people, so the result is not what is
  usually called a key pair (set of public+private keys belonging to one
  user) - in fact, I don't know what the result is. Could you explain?

A keypair is used in some APIs (crypto_box_seal_open comes to mind).
All that function does is concatenate the two.

It has nothing to do with key exchange.

That is rather unfortunate reuse of the terminology. Can we use some
other term in PHP API or docs? Also, why concatenating two keys is
useful - are they then used as a single key? Or they are de-concatenated
internally? If the latter, it would be better to pass them as separate
parameters.

Libsodium is an opinionated cryptography library that doesn't give you
a lot of levers to pull -- only secure defaults you can't change,
consisting of carefully-selected cryptography primitives.

OK, I can see this as a valid approach, but then:

• The choices have to be very thoroughly documented, otherwise
  interoperating with such library is nearly impossible
• The approach should be consistent - i.e. the underlying algorithms
  should be clearly segregated.

While it can be a boon for simplicity, I do not see how such
code could be made to interoperate with code in other languages and
other systems.

That isn't its purpose. Its purpose is to be secure. (It also happens
to also be extremely fast.)

I would say such approach can have its place, but one has to be very
careful using it for any system that has any chance of interacting with
a wider world. I.e. encrypting internal data - fine, but any external
communication - one then would need to use something with a wider
appeal. But then the question one would ask is - why have two crypto
libraries in one code base? Not an argument against having this lib,
just food for thought.

https://download.libsodium.org/libsodium/content/public-key_cryptography/authenticated_encryption.html#algorithm-details

This is a 404 for me.

This is how a stream cipher works:

1. You take your key and nonce, and generate a long string of random
   bytes deterministically from your key and nonce.
2. To encrypt: You XOR every byte of plaintext with a corresponding
   byte of the keystream.
3. To decrypt: You XOR every byte of ciphertext with a corresponding
   byte of the keystream.

This is unauthenticated encryption; don't use it unless you're
building something with it and authenticate elsewhere.

OK, so the _xor function does both 1 and 2? That sounds sensible, but
probably needs to be explained in the docs, to avoid misunderstanding.
Also, name like stream_cypher or something like that may be better, I'm
not sure exposing the fact that XOR is being used is that important.

• Why one would use crypto_scalarmult? That looks like a random piece of
  low level API without clear indication why it exists.

It's used to derive a shared secret from an X25519 secret key and an
X25519 public key.

Ok, so this is a part of underlying low-level API? That's fine but it
could be nicer if one could somehow follow which functions are low and
high level and how they relate.

• What is the difference between crypto_stream and randombytes_buf? Both
  seem to produce a random string of given length. I assume crypto_stream
  does more but the docs do not explain.

crypto_stream is deterministic; randombytes_buf is non-deterministic

Ah, ok, now I see it. So it's the step 1 in crypto_stream_xor above.
That makes total sense, but I think should be explained. Note that for
you as the developer it may seem completely obvious and me asking
about it sounds like nitpicking. But you know the overall picture and
the intent of the API, you knew it in your mind before you wrote the
whole thing. I, on the other hand, have only the docs to rely on, and
have to discover that intent from those docs. So I think there should be
more clues to it :)

• For crypto_kx, it is not clear what exactly this function does and why
  the same parameter is passed twice in different places. I mean, if you
  know the details of how DH key exchange works, it may make sense, but if
  we want to make it simple for the common user, it needs some better
  explanations and maybe better parameters.

This is basically a multi-step crypto_scalarmult in one complete pacakge.

This seems to be leaking details of the implementation somewhat through
the abstraction. If we define the goal of API as to provide abstraction
that allows to move from set of keys to shared secret, then I think the
API should look like:

$shared_secret = crypto_shared_secret($my_public_key, $my_private_key,
$their_public_key);

or something like that. In fact for DH I'm not even sure one needs
$my_public_key? And it may be possible to derive from secret key anyway,
so maybe just ($my_private_key, $their_public_key) - unless common case
is that we have both private and public keys already.

Stas Malyshev
smalyshev@gmail.com