Code for libprio pilot #1

henrycg · 2018-06-25T21:57:43Z

These commits contain all of the libprio library code, broken down by functional component. The four commits contain:

the core code for the libprio client and servers,
the tests and example code form invoking libprio,
a copy of the NSS MPI bignum library (needed to use libprio as a standalone library), and
the master build file for scons and a README.

The first commit (with the core code) contains the header files and .c files that will need to go into the browser implementation of libprio, so this code should receive the most scrutiny.

These files contain the core cryptographic routines needed to implement the Prio client and Prio server.

Since NSS does not export the MPI bignum library, we ship a copy with the standalone version of libprio.

franziskuskiefer

First rounds of comments.

franziskuskiefer · 2018-06-26T07:05:07Z

SConstruct

+env.Append(CPPPATH = ["#include", "#."])
+
+SConscript('prio/SConscript', variant_dir='build/prio')
+SConscript('mpi/SConscript', variant_dir='build/mpi')


I'd like to see that we can use another copy of MPI here. That would allow us to use the copy in the Firefox tree. That should be relatively easy to do. We can keep the copy in the repo here to allow stand-alone builds but not vendor it into Firefox.

@rhelmer Rob: Is this something that you can take care of on your side? I'm happy to modify the SConstruct file in libprio to accommodate using an external copy of MPI, but I'm not sure what the best way to do this is.

franziskuskiefer · 2018-06-26T07:07:05Z

SConstruct

+Copyright (c) 2012-2014 Stanford University
+Copyright (c) 2018 Henry Corrigan-Gibbs
+
+Permission to use, copy, modify, and distribute this software for any 


When adding to Firefox you should check that the license is ok to use.

@rhelmer Rob: Let me know if you are going to need to include this file in the browser code. If using the ISC license here will be a problem, I can throw out this SConstruct file and write a new MPL-licensed one from scratch.

franziskuskiefer · 2018-06-26T07:15:28Z

include/mprio.h

+ * Opaque types
+ */
+typedef struct prio_config *PrioConfig;
+typedef const struct prio_config *const_PrioConfig;


Not sure I'm a fan of typedefing the const version of the struct. But nothing wrong with it I guess.

Ah, got it. For my future reference, what is the right way to do this? Would it be better to not use these typedefd struct at all?

I'd probably just use a const *PrioConfig etc. But not really a big deal.

Got it. Thanks.

franziskuskiefer · 2018-06-26T07:20:55Z

prio/prg.c

+#include "share.h"
+#include "util.h"
+
+#define AES_BLOCK_SIZE 16


You could take this from blapit.h instead of re-defining it here.

Great. Will do.

franziskuskiefer · 2018-06-26T07:25:58Z

prio/prg.c

+PRG 
+PRG_new (const PrioPRGSeed key_in)
+{
+  PRG prg = malloc (sizeof (*prg));


sizeof(PRG)?

These are equivalent, right? To make the code easier to read, I'll change to sizeof(PRG).

Whoops. After running valgrind, I realized that this change introduced a bug.

The sizeof(PRG) is the size of a struct prg* (i.e., a pointer) while sizeof(*prg) is the size of a struct prg (i.e., the struct itself). The latter one is what I wanted. To make this clear, I will change the code to say sizeof(struct prg).

franziskuskiefer · 2018-06-26T07:47:19Z

prio/util.h

+#define MIN(a, b) ((a) < (b) ? (a) : (b))
+
+// Check a Prio error code and return failure if the call fails.
+#define P_CHECK(s) \


Not a big fan of this style of macros for error checking. But nothing I'd change now.

Yeah, this feels ugly to me too. Is there a cleaner design pattern that I should use in the future?

For simple error check's I'd just do it explicitly. One of the issues with these macros is that the have hidden returns/goto, which is error-prone. Always doing the verbose error check and return/goto is a little more code to write but is better to read.
The rv = SECFailure; goto cleanup isn't great either but something that's really hard to avoid. (You return rv in this case and must make sure that it's always set correctly, which is hard.)

Right. So I'll leave this as is for now.

franziskuskiefer · 2018-06-26T07:48:09Z

prio/rand.c

+SECStatus
+rand_init (void)
+{
+  SECStatus rv = NSS_NoDB_Init (NULL);


Is this the first time you need NSS? How do you ensure that you initialise this only once?

Yes, this is the first time that we need NSS. The Prio_init() function calls this one when the library is initialized.

To make sure that NSS only gets initialized once, I will wrap the NSS_NoDB_Init call with a check to NSS_IsInitialized.

franziskuskiefer · 2018-06-26T07:48:43Z

prio/rand.c

+    return SECFailure;
+  }
+
+  SECStatus rv;


Init to SECFailure.

franziskuskiefer · 2018-06-26T07:49:58Z

prio/rand.c

+rand_int_rng (mp_int *out, const mp_int *max, 
+    RandBytesFunc rng_func, void *user_data)
+{
+  SECStatus rv = SECSuccess;


Generally, move these rv down to first use.

One question about this: Do you want me to try to do this throughout libprio? For consistency, I have been placing the rv initialization at the top of the function just to keep it out of the way of the meaningful/non-repetitive code. If it's worthwhile, I can go back through all of the code and move the rvs down, but I'm not sure if this will improve readability.

I generally like initialisations close to first use. Defining things on top is aa artefact from C89 where this was necessary.
I think readability is better then. But I wouldn't insist on it.

Okay, if it's acceptable, I will leave these as is.

franziskuskiefer · 2018-06-26T07:52:22Z

prio/serial.c

+  P_CHECKCB (obj->type == MSGPACK_OBJECT_STR);
+
+  msgpack_object_str s = obj->via.str;
+  MP_CHECKC (mp_read_unsigned_octets (n, (unsigned char *)s.ptr, s.size));


Make sure n and other arguments are not NULL.

franziskuskiefer · 2018-06-27T11:47:45Z

prio/util.h

+#define MIN(a, b) ((a) < (b) ? (a) : (b))
+
+// Check a Prio error code and return failure if the call fails.
+#define P_CHECK(s) \


For simple error check's I'd just do it explicitly. One of the issues with these macros is that the have hidden returns/goto, which is error-prone. Always doing the verbose error check and return/goto is a little more code to write but is better to read.
The rv = SECFailure; goto cleanup isn't great either but something that's really hard to avoid. (You return rv in this case and must make sure that it's always set correctly, which is hard.)

franziskuskiefer · 2018-06-27T11:52:53Z

prio/rand.c

+rand_int_rng (mp_int *out, const mp_int *max, 
+    RandBytesFunc rng_func, void *user_data)
+{
+  SECStatus rv = SECSuccess;


I generally like initialisations close to first use. Defining things on top is aa artefact from C89 where this was necessary.
I think readability is better then. But I wouldn't insist on it.

franziskuskiefer · 2018-06-27T12:00:58Z

prio/encrypt.c

+  if (dataLen != CURVE25519_KEY_LEN)
+    return SECFailure;
+
+  // TODO: Horrible hack. We should be able to import a public key


You can import keys with pk8 (private) and spki (public). See [1] for example.
This isn't great. But that's the official format 😞

[1] https://searchfox.org/nss/source/gtests/pk11_gtest/pk11_curve25519_unittest.cc#66

It would be really nice to be able to encode curve25519 public keys as fixed-length binary blobs (and also to be able to decode them from fixed-length blobs). If the keys have variable length, then parsing the ciphertexts that are output by PublicKey_encrypt will be more complicated and error-prone.

Since PublicKey_import is only used internally by the code in encrypt.c (to decode the ephemeral public key that's prepended to the ciphertext), I am wondering if there is maybe a simpler (though less portable) way to import the public key into NSS.

For example, I could hardcode an SPKI representation of the all-zeros curve25519 public key, import that using SECKEY_DecodeDERSubjectPublicKeyInfo and then set the bytes of the key as I am doing now. Would that work?

Since this has to go through PK11 NSS can't take byte blobs unfortunately.

For example, I could hardcode an SPKI representation of the all-zeros curve25519 public key, import that using SECKEY_DecodeDERSubjectPublicKeyInfo and then set the bytes of the key as I am doing now. Would that work?

That would be pretty ugly but should work 🙂

franziskuskiefer · 2018-06-27T12:04:06Z

prio/encrypt.c

+
+  paramItem->type = siBuffer;
+  paramItem->data = (void *)param;
+  paramItem->len = sizeof (CK_AES_CTR_PARAMS);


You probably want to use CK_GCM_PARAMS here or param.

Right. i will change this to sizeof(*param).

franziskuskiefer · 2018-06-27T12:07:25Z

prio/encrypt.c

+PublicKey_encryptSize (unsigned int inputLen)
+{
+  // public key, IV, tag, and input
+  return CURVE25519_KEY_LEN + GCM_IV_LEN_BYTES + GCM_TAG_LEN_BYTES + inputLen;


This can overflow and thus cause badness. You should generally check lengths before adding them up to make sure they not getting too large.

Oh yes, thanks for pointing this out. I will change PublicKey_encryptSize to return an error flag to catch values so large as to cause integer overflow.

Now we can import a 32-byte curve25519 public key into NSS without having to generate a new keypair from scratch.

- Public functions PublicKey_import_hex and PublicKey_export_hex - Tests for these functions

Add browser-test utility that 1) generates new server keypairs, 2) uses xpcshell to call the PrioEncoder DOM routines, 3) parses the output of PrioEncoder, 4) validates the encoded packet, and 5) checks that the submitted data is what we expected.

Henry Corrigan-Gibbs added 4 commits June 25, 2018 14:36

The core libprio code for Prio client and server.

647aed4

These files contain the core cryptographic routines needed to implement the Prio client and Prio server.

Tests and example code for libprio.

a3b265b

A copy of NSS's MPI library.

42c5ca2

Since NSS does not export the MPI bignum library, we ship a copy with the standalone version of libprio.

Build file and README for libprio.

432a37e

rhelmer assigned rhelmer and henrycg Jun 25, 2018

franziskuskiefer reviewed Jun 26, 2018

View reviewed changes

Edits per code review by franziskuskiefer

eaa258b

franziskuskiefer reviewed Jun 27, 2018

View reviewed changes

Henry Corrigan-Gibbs added 8 commits June 27, 2018 13:57

More edits per code review by franziskuskiefer

e56c919

Fix memory bugs found by clang-analyzer

0220a63

Remove ugly hack from PublicKey_import method

0149212

Now we can import a 32-byte curve25519 public key into NSS without having to generate a new keypair from scratch.

Replace SConstruct file with simpler one

6849de9

Update README to incorporate code review edits

4680e1c

Allow importing and exporting public keys in hex

f733616

- Public functions PublicKey_import_hex and PublicKey_export_hex - Tests for these functions

Fixes to make browser-test compile on Linux

6ef4ce6

rhelmer merged commit 5779b9c into mozilla:master Jul 17, 2018

Code for libprio pilot #1

Code for libprio pilot #1

Conversation

henrycg commented Jun 25, 2018

franziskuskiefer 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

henrycg Jun 26, 2018 • 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

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

henrycg Jun 26, 2018 •

edited

Loading