DIPPE scheme #12

mlewa89 · 2019-08-01T16:17:16Z

Hello, we would like to contribute to the FENTEC library, by providing our implementation of the decentralized ABE scheme from here. We appreciate your feedback.

janhartman

Hi, thanks for this PR. Good job on your first contribution!
This seems quite nice and mostly in line the rest of the library. However, I do have a few suggestions on how to improve. See my comments below - the most important ones are about the order and initialization of parameters.

janhartman · 2019-08-02T09:07:40Z

include/cifer/abe/dippe.h

+ * @param gid String that represents a unique user; Required for collusion prevention
+ * @return Error code
+ */
+cfe_error cfe_dippe_keygen(cfe_dippe *dippe, cfe_dippe_user_secret_key *usk, unsigned int usk_id, cfe_dippe_pub_key *pks[], unsigned int pks_len, cfe_dippe_sec_key *sk, mpz_t *attrs, char gid[]);


You don't need to pass mpz_t variables to functions as pointers - even in the GMP library, they always pass them as values.

Good to know, I fixed it.

janhartman · 2019-08-02T09:09:27Z

include/cifer/abe/dippe.h

+ * @param dippe A pointer to a cfe_dippe struct
+ * @param assump_size The size of the underlying assumption
+ */
+void cfe_dippe_setup(cfe_dippe *dippe, unsigned int assump_size);


Please change setup to init to be consistent with the rest of the library.

janhartman · 2019-08-02T09:21:12Z

include/cifer/abe/dippe.h

+ * @param pk A pointer to a cfe_dippe_pub_key struct; Represents the public key that is about to be populated
+ * @param sk A pointer to a cfe_dippe_sec_key struct; Represents the secret key that is about to be populated
+ */
+void cfe_dippe_authsetup(cfe_dippe *dippe, cfe_dippe_pub_key *pk, cfe_dippe_sec_key *sk);


There are a few problems that occur in almost all of the functions here.

Order of parameters: We follow GMP's convention, which is func_name(return_value, op1, op2...). In this case, it should be cfe_dippe_auth_init(cfe_dippe_pub_key *pk, cfe_dippe_sec_key *sk, cfe_dippe *dippe)

Parameter initialization: Again, following another GMP's convention, we require that all function parameters are initialized. Thus, we have _init functions for each defined struct which need to be called by the user before calling the Look at other schemes, e.g. abe/fame or abe/gpsw as examples. Only the _ìnit functions are allowed to allocate memory they don't free (for this, each _init function has a _free function, which you've already implemented).

Please apply these two points to all of your implemented functions.

I swapped the order of function parameters, so that the return value is now always on the first position. Functions that allocates memory now also have the _init within their names.

janhartman · 2019-08-02T09:24:29Z

include/cifer/abe/dippe.h

+    ECP2_BN254   g2_sigma;
+	ECP_BN254*   g1_W_A;
+	FP12_BN254*  gt_alpha_A;
+} cfe_dippe_pub_key;


I recommend using an autoformatter - it can be an immense help as it saves you a lot of manual labor.

Yep, thanks. Now everything should be aligned.

janhartman · 2019-08-02T09:29:08Z

include/cifer/abe/dippe.h

+ * cfe_dippe represents the DIPPE scheme
+ */
+typedef struct cfe_dippe {
+    unsigned int assump_size;


size_t might be easier to read, also mostly used in the rest of CiFEr

I replaced all unsigned int with size_t.

janhartman · 2019-08-02T09:30:13Z

src/abe/dippe.c

+ * @param dippe A pointer to a cfe_dippe struct
+ * @param assump_size The size of the underlying assumption
+ */
+void cfe_dippe_setup(cfe_dippe *dippe, unsigned int assump_size) {


There is no need to copy these comments from the header files. Only the header files are used for creating the documentation and if you're using an IDE, you can quickly jump to a function's definition from its implementation. I don't necessarily recommend deleting all of these comments in the .c file, maybe you can keep just the descriptions (up to you).

You're right, I removed the comment blocks from the c-files.

janhartman · 2019-08-02T09:35:40Z

test/abe/dippe.c

+
+MunitSuite dippe_suite = {
+    (char *) "/abe/dippe", dippe_tests, NULL, 1, MUNIT_SUITE_OPTION_NONE
+};


Have you tried running these tests with Valgrind to find any potential memory leaks?

I tried running Valgrind's memcheck tool. It didn't report me any memory leaks for the implementation. However, I got some reports but for all defined tests. It seems to be the same as described here, so maybe its no real problem.

tilenmarc

Thank you for your contribution! Nice job on trying to implement in the same style as the existing library. I wrote you some additional comments what you could change, we would be very happy if you could do these improvements.

tilenmarc · 2019-08-05T13:00:08Z

src/abe/dippe.c

+ * @param in_buf_size Size of the buffer
+ */
+void cfe_dippe_hash_G2(ECP2_BN254 *out, char* in_buf, int in_buf_size) {
+    // rounds


Could you give some reference to where this way of hashing comes from?

I don't really have a reference. It looked like the buffer that is about to be hashed must be of size MODBYTES_256_56. I thought it of a way to hash greater buffers, that may occur during KeyGen. Maybe there's a better way.

In the meantime we a bit reorganized the library and we put all the implemented hashing functions in internal/hash.c file. There you can also find some helping functions such needed for hashing such as string concatenation and vector to string changer. Could you rebase your contribution and just use these functions (cfe_hash_G2) instead of defining a new one.

tilenmarc · 2019-08-05T13:00:19Z

src/abe/dippe.c

+    cfe_vec A;
+    cfe_vec_init(&A, ((assump_size+1) * assump_size));
+    cfe_uniform_sample_vec(&A, dippe->p);
+


You choose A to be a vector, but instead you could choose to be a matrix with cfe_mat. This way you could use already implemented functions as cfe_mat_transpose and cfe_mat_mul to make things more elegant.

You're right, I changed the code to utilize the already existent matrix struct.

tilenmarc · 2019-08-05T13:00:39Z

src/abe/dippe.c

+    }
+
+    // g1^U(T)A (k+1 x k)
+    dippe->g1_UA = (ECP_BN254*)cfe_malloc(sizeof(ECP_BN254) * (assump_size+1) * assump_size);


We have a struct representing a vector of ECP_BN254 values called cfe_vec_G1. You could use this here. It would be even nicer if you defined a matrix of ECP_BN254 values, say cfe_mat_G1 and use it here instead.

I replaced them and as you suggested, added additional structures for matrices of group elements.

tilenmarc · 2019-08-05T13:00:56Z

src/abe/dippe.c

+
+    // W (k+1 x k+1)
+    cfe_vec_init(&(sk->W), (dippe->assump_size+1) * (dippe->assump_size+1));
+    cfe_uniform_sample_vec(&(sk->W), dippe->p);


Same comment here as above, you can use matrices instead of vectors.

tilenmarc · 2019-08-05T13:01:12Z

src/abe/dippe.c

+ * @param gid String that represents a unique user; Required for collusion prevention
+ * @return Error code
+ */
+cfe_error cfe_dippe_keygen(cfe_dippe *dippe, cfe_dippe_user_secret_key *usk, unsigned int usk_id, cfe_dippe_pub_key *pks[], unsigned int pks_len, cfe_dippe_sec_key *sk, mpz_t *attrs, char gid[]) {


Choosing attrs to be a mpz_t seems like a strange choice. As far a I see (correct me if I am wrong) you are just changing a string to mpz_t and then back to string. I would suggest to have something like an array of owned attributes, for example int attrib[] = {0,3,4} indicating atribute 0, 3 and 4 are owned. We did it this way in FAME scheme.

tilenmarc · 2019-08-05T13:01:47Z

src/abe/dippe.c

+ * @param pattern String that is used as template for the attribute vector
+ * @return Error code
+ */
+cfe_error cfe_dippe_build_exact_threshold_attribute_vector(cfe_dippe *dippe, mpz_t *attrs, const char pattern[]) {


Isn't this just copy paste of cfe_dippe_build_conjunction_attribute_vector?

Correct, I removed one of them.

tilenmarc · 2019-08-05T13:01:57Z

test/abe/dippe.c

+    cfe_dippe_pub_key *pks[] = {&(pk[0]), &(pk[1]), &(pk[0]), &(pk[1]), &(pk[0]), &(pk[1])};
+
+    // unique GID
+    char gid[] = "TESTGID";


Shouldn't each user get its own unique GID?

That's right. But the test case here only simulates a single user. For each of the attribute vector components, the user has to request a decryption key from the respective authority. The GID for all requests has to be the same, so that the resulting keys fit together.

Aren't you simulating multiple users, each with a different set of attributes? Because in your interpretation the one user chooses different vectors as if he can choose which attributes he has? Maybe I am wrong...

Ah yea, now I get what you mean. I thought of it as independent test runs for a single user but your interpretation makes more sense. I changed it, so that the GID is modified a bit for each run.

tilenmarc · 2019-08-05T13:02:02Z

test/abe/dippe.c

+        munit_assert(err == CFE_ERR_NONE);
+
+        // Encrypt message under given policy vector
+        err = cfe_dippe_encrypt(&dippe, &cipher, pks, (sizeof(pks)/sizeof(cfe_dippe_pub_key*)), &pol, &msg);


I think you should put encryption before the loop. Encryption is done once, then different users try to decrypt it with different attributes.

You're right, I moved the encryption algorithm outside the loop.

tilenmarc · 2019-08-05T13:02:11Z

test/abe/dippe.c

+    cfe_dippe_pub_key *pks[] = {&pk0, &pk0, &pk0, &pk0, &pk0};
+
+    // unique GID
+    char gid[] = "TESTGID";


Same comment as above.

tilenmarc · 2019-08-05T13:02:16Z

test/abe/dippe.c

+        munit_assert(err == CFE_ERR_NONE);
+
+        // Encrypt message under given policy vector
+        err = cfe_dippe_encrypt(&dippe, &cipher, pks, (sizeof(pks)/sizeof(cfe_dippe_pub_key*)), &pol, &msg);


Same comment as above.

tilenmarc

Thanks for your work. Good job. I wrote you a couple more of trivial things to change, and two a bit bigger requests. I believe we are close to finishing this PR!

tilenmarc · 2019-08-21T10:04:37Z

include/cifer/abe/dippe.h

+typedef struct cfe_dippe_policy_vector {
+    size_t len;
+    cfe_vec pol;
+} cfe_dippe_policy_vector;


cfe_vec already has an atribute len, so as I see this struct is not needed.

You're right. I've removed this struct.

tilenmarc · 2019-08-21T10:04:51Z

include/cifer/abe/dippe.h

+ * @param threshold Theshold value
+ * @return Error code
+ */
+cfe_error cfe_dippe_threshold_policy_vector_init(cfe_dippe_policy_vector *pv, cfe_dippe *dippe, size_t vec_len, size_t pattern[], size_t pat_len, size_t threshold);


Correct me if I am wrong, but is this the algorithm for the exact threshold? If so, could you change threshold to exact_threshold whenever it appears, to not cause confusion.

Correct, I've renamed it to exact_threshold.

tilenmarc · 2019-08-21T10:04:56Z

include/cifer/abe/dippe.h

+ * @param pat_len Length of the pattern array
+ * @return Error code
+ */
+cfe_error cfe_dippe_conjunction_policy_vector_init(cfe_dippe_policy_vector *pv, cfe_dippe *dippe, size_t vec_len, size_t pattern[], size_t pat_len);


Maybe rename parameter vec_len to something more descriptive, like num_attrib for number of all attributes. Then vec_len = num_attrib + 1. Similarly with threshold.

tilenmarc · 2019-08-21T10:05:33Z

include/cifer/data/vec_curve.h

+ */
+void cfe_vec_GT_free(cfe_vec_GT *v);
+
+


Remove one empty line.

tilenmarc · 2019-08-21T10:06:24Z

src/abe/dippe.c

+ * @param in_buf_size Size of the buffer
+ */
+void cfe_dippe_hash_G2(ECP2_BN254 *out, char* in_buf, int in_buf_size) {
+    // rounds


In the meantime we a bit reorganized the library and we put all the implemented hashing functions in internal/hash.c file. There you can also find some helping functions such needed for hashing such as string concatenation and vector to string changer. Could you rebase your contribution and just use these functions (cfe_hash_G2) instead of defining a new one.

tilenmarc · 2019-08-21T10:06:34Z

src/abe/dippe.c

+        // g1^(W(T)As)
+        cfe_mat_G1_mul_vec(&g1_W_A_s, &(pks[m]->g1_W_A), &s);
+
+        // g1^(W(T)As)


This comment should be // g1^(xU(T)As) as far as I see.

tilenmarc · 2019-08-21T10:07:03Z

src/abe/dippe.c

+    return CFE_ERR_NONE;
+}
+
+cfe_error cfe_dippe_keygen(cfe_dippe_user_sec_key *usk, cfe_dippe *dippe, size_t usk_id, cfe_dippe_pub_key *pks[], size_t pks_len, cfe_dippe_sec_key *sk, cfe_dippe_attribute_vector *av, char gid[]) {


I finally understood that the way you implemented keygen and decrypt is to work with policies (vectors) obtained as threshold or conjunction instructions. Wouldn't it be nicer if it was implemented to work with general vectors, i.e. as an inner-product predicate scheme. In this sense the function cfe_dippe_attribute_vector_init would serve only as a change from a list of attributes to a vector (like the function cfe_dippe_threshold_policy_vector_init in essence already does) . If you could change it, we would be happy, otherwise we will change it latter.

Agree, I've removed the cfe_dippe_attribute_vector struct and replaced it with a cfe_vec.

tilenmarc · 2019-08-21T10:08:13Z

src/abe/dippe.c

+    cfe_vec_set_const(&(pv->pol), zero);
+
+    for (size_t i = 0; i < pat_len; i++) {
+        if (pattern[i] < vec_len) {


I believe that it should be < vec_len - 1. Similarly in the conjunction function and in cfe_dippe_attribute_vector_init.

tilenmarc · 2019-08-21T10:08:33Z

src/data/vec_curve.c

+
+void cfe_vec_GT_free(cfe_vec_GT *v) {
+    free(v->vec);
+}


Add a newline at the end.

tilenmarc · 2019-08-21T10:08:56Z

test/abe/dippe.c

+    cfe_dippe_pub_key *pks[] = {&(pk[0]), &(pk[1]), &(pk[0]), &(pk[1]), &(pk[0]), &(pk[1])};
+
+    // unique GID
+    char gid[] = "TESTGID";


Aren't you simulating multiple users, each with a different set of attributes? Because in your interpretation the one user chooses different vectors as if he can choose which attributes he has? Maybe I am wrong...

tilenmarc

Thank you again for your good work and sorry for a slow response. Before merging I wrote you two comments, one is a quick fix, the second is not your fault but a consequence of a typo in the original paper. After fixing this we can finaly merge.

tilenmarc · 2019-09-30T12:31:02Z

src/abe/dippe.c

+        ECP2_BN254_mul(&tmp_g2, tmp_big);
+
+        ECP2_BN254_toOctet(&oct, &tmp_g2);
+        hash_str = cfe_strings_concat(str_ele, "|", gid, "|", str_vec, NULL);


Can't you just put oct.val instead of str_ele here?

While changing this, I noticed that using the output of ECP2_BN254_toOctet directly with cfe_strings_concat doesn't work very well as it is an arbitrary byte array. I added another step to gain a properly null terminated string that can be concatenated. Please check again.

tilenmarc · 2019-09-30T12:40:18Z

src/abe/dippe.c

+    oct.val = (char *)&str_ele;
+
+    // mue
+    ECP2_BN254 mue;


After carefully rereading the paper I noticed that there is a typo in the original paper. I think this mue should be a vector of length k + 1 (so you do the same as you did here, just for k + 1 different hashes). Please check page 8 of the paper, where they define random oracle H as a function into Z_p^{k+1}. I know that then in the KeyGen process it is said that mue it is an element of Z_p, but this is probably a typo, since then latter in the definition of Ki it is treated as a vector.

Well spotted, I looked at the paper again and I think you're right. The mue should be a vector in this case.

tilenmarc · 2019-09-30T12:41:13Z

src/abe/dippe.c

+        ECP2_BN254_add(&(usk->Ki.vec[i]), &tmp_g2);
+
+        // add mue
+        ECP2_BN254_add(&(usk->Ki.vec[i]), &mue);


Here mue should be added as a vector, see my above comment.

mlewa89 · 2019-10-15T09:40:29Z

While looking for memory leaks I might found two minor mistakes which are not directly related to the implementation. Please check whether they are relevant.

CiFEr/src/internal/hash.c

Lines 25 to 26 in f1fdfdd

    
           void cfe_hash_G1(ECP_BN254 *g, char *str) { 
        
               octet tmp_str = {sizeof(str), 0, str};

At this point, I think it should be strlen instead of sizeof. Same with cfe_hash_G2

CiFEr/test/internal/big.c

Line 24 in f1fdfdd

MunitResult test_big(const MunitParameter *params, void *data) {

A mpz_clears(four, four_inv, p, NULL); seems to be missing in this function.

tilenmarc · 2019-10-16T08:46:51Z

Thanks for your contribution. I will merge your PR. Thanks also for your last comments on sizeofand mpz_clears, I will fix that.

janhartman self-assigned this Aug 2, 2019

janhartman reviewed Aug 2, 2019

View reviewed changes

tilenmarc reviewed Aug 5, 2019

View reviewed changes

janhartman removed their assignment Aug 12, 2019

tilenmarc reviewed Aug 21, 2019

View reviewed changes

Marco Lewandowsky added 6 commits August 27, 2019 16:09

DIPPE scheme added

28b0be1

Small fixes

31cef29

Doc fixes

b19954e

Feedback

ad508f9

Some adjustments

a9876ab

More feedback

e9c85e2

tilenmarc reviewed Sep 30, 2019

View reviewed changes

Changed mue to a vector

7b27140

tilenmarc merged commit 32b3981 into fentec-project:master Oct 16, 2019

DIPPE scheme #12

DIPPE scheme #12

Conversation

mlewa89 commented Aug 1, 2019

janhartman 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

janhartman Aug 2, 2019 • 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

tilenmarc left a comment • 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

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

tilenmarc 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

tilenmarc 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

mlewa89 commented Oct 15, 2019

tilenmarc commented Oct 16, 2019

janhartman Aug 2, 2019 •

edited

Loading

tilenmarc left a comment •

edited

Loading