Modified fuzz tests about field operations #1407

YafeiXie1 · 2023-08-21T12:09:04Z

Thanks for the previous review.
This PR:
Added a function to construct valid field elements with random magnitude.
Added the constraint in each field operation test based on the magnitude and normalization.
Could you give me some suggestions?

sipa · 2023-08-21T14:25:50Z

@YafeiXie1 Just FYI, you don't need to open a new pull request when you make changes. You can push again to the same branch with more commits (or if you modified existing commits, you'd need a force push, but it's still possible).

src/fuzz.c

sipa · 2023-08-24T21:43:48Z

src/fuzz.c

+    }
+}
+
+#ifdef VERIFY


sipa · 2023-08-24T21:47:00Z

src/fuzz.c

+    if (size >= 32) {
+        secp256k1_fe a;    
+        fuzz_field_construct(data, size, &a);
+        if (a.normalized) { 


The test could be a lot more efficient if fuzz_field_construct had an input argument that can guarantee it only produces normalized field elements (because as-is, in many cases no test will be performed at all). Alternatively, there could be a separate function for just that.

sipa · 2023-08-24T21:48:28Z

src/fuzz.c

+        int rand_magnitude = data[31] % 33;
+        secp256k1_fe_normalize(r);
+        if (rand_magnitude == 0) {
+            return;


In this case, the returned field element will not have magnitude 0. In fact, it'll be unspecified, making the code that uses it below have undefined behavior. For magnitude 0, you should set the output to just be the 0 field element.

sipa · 2023-08-24T21:49:02Z

src/fuzz.c

+    if (size>=32) {
+        secp256k1_fe a, zero;      
+        secp256k1_fe_set_b32_mod(r, data);
+        int rand_magnitude = data[31] % 33;


I don't think it's a good idea to reuse fuzz input bytes for two distinct meanings (here: magnitude and the lowest byte of the field element representation). It may also be better to let the caller decide the magnitude (or at least max magnitude), and then let the function construct it.

sipa · 2023-08-24T21:50:18Z

src/fuzz.c

+        }
+        secp256k1_fe_clear(&zero);
+        secp256k1_fe_negate(&zero, &zero, 0);
+        secp256k1_fe_mul_int_unchecked(&zero, rand_magnitude - 1);


The approach here does not reach all possible field element representations; only ones that can be represented as "normalized_number times integer in range 1 to 31".

I think it would be better if you'd read a full 40 bytes, and then use masking (using bitwise AND to set certain bits to zero) and/or rejection (just give up if the resulting value has bad magnitude) to obtain field elements of the desired magnitude.

Thanks for your suggestions! I will modify this fuzz_field_construct function and other errors.

sipa · 2023-08-24T21:52:50Z

src/fuzz.c

+    if (size >= 32) {        
+        secp256k1_fe a, negate;
+        fuzz_field_construct(data, size, &a);
+        if (a.magnitude >= 32) {


This test is unnecessary; fuzz_field_construct only constructs values with magnitude at most 32.

sipa · 2023-08-24T21:53:55Z

src/fuzz.c

+    if (size >= 64) {        
+        secp256k1_fe a, b, r1, r2;
+        fuzz_field_construct(data, size, &a);
+        fuzz_field_construct(data + 32, size, &b);         


The size pass here is incorrect, because the array pointed to by data + 32 only has length size - 32. See also my comment about responsibility of guaranteeing length on fuzz_field_construct.

and fuzzing tests of group operations

YafeiXie1 · 2023-08-29T14:12:02Z

Thanks for the previous review.
This PR:
Modified the field construct function to build more valid field elements.
Modified and added the fuzzing tests for the group operations.
Could you give me some suggestions?

sipa · 2023-08-29T14:31:03Z

.vscode/settings.json

@@ -0,0 +1,5 @@
+{


Don't add this file, please. It's for your local setup, not everyone's.

sipa · 2023-08-29T17:46:46Z

src/fuzz.c

+    if (r->normalized) {
+        if ((r->n[4] == 0x0FFFFFFFFFFFFULL) && ((r->n[3] & r->n[2] & r->n[1]) == 0xFFFFFFFFFFFFFULL)) {
+            uint64_t mask3 = 0xFFFFEFFFFFC2FULL - 1;
+            r->n[0] &= mask3;


I believe this is inaccurate, as it will unconditionally wipe some bits in r->n[0] even when unnecessary, making certain values unreachable. E.g. it'll turn r->n[0] = 0x1000003d1 into r->n[0] = 0.

sipa · 2023-08-29T18:08:25Z

src/fuzz.c

+* If 'normalized' = 0, this function will construct normalized or non-normalized field elements depending on the fuzzer data.
+* On output, r will be a valid field element
+**/
+static void fuzz_field_construct(const uint8_t *data, int normalized, secp256k1_fe *r) {


I think the effectiveness of some of the tests below could be increased further by passing a max magnitude argument to fuzz_field_construct, so that things like

fuzz_field_construct(data, 0, &a); fuzz_field_construct(data + 42, 0, &b); if (a.magnitude + b.magnitude <= 32) {

could become

fuzz_field_construct(data, 0, 32, &a); fuzz_field_construct(data + 42, 0, 32 - a.magnitude, &b);

It'd also remove the need for fuzz_ge_fe_construct.

sipa · 2023-08-29T18:14:33Z

src/fuzz.c

+}
+
+/* Test commutativity of addition on two field elements */ 
+static void fuzz_field_add_commutativty(const uint8_t *data, size_t size) {


sipa · 2023-08-29T18:19:04Z

src/fuzz.c

+        if (a.magnitude <= 10) {
+            int m = a.magnitude;
+            r1 = a;
+            secp256k1_fe_mul_int(&r1, 3);


It's a bit limiting to only test multiplication by 3.

sipa · 2023-08-29T19:02:48Z

src/fuzz.c

+    fuzz_ge_fe_construct(data, 1, &x);
+    if (secp256k1_ge_x_on_curve_var(&x)) {
+        int odd = data[42] % 2;
+        secp256k1_ge_set_xo_var(&ge, &x, odd);           


It would be nice if the y coordinate could also be given a nontrivial normalization/magnitude here.

As a stretch, it could be interesting to (optionally) also construct the group element by reading the Y coordinate from the fuzz input, and then construct the corresponding X coordinate (it needs a cube root, but that's relatively simple for our field). I can give you some pointers if you're interested in that.

Thanks for the comments! Yes, I’m interested in constructing the group element by reading the Y coordinates from fuzzer data (find X from the elliptic curve equation).

In addition, I changed the fuzz_field_construct function (add another input argument to determine the max magnitude) and fixed the mask errors. Currently, I’m working on the fuzzing test about the function related to point multiplication operation, is this a good part to carry on, or do you have a better recommendation?

I'll have a look over your changes soon. In the mean time, yes, having some fuzzing of EC multiplication would be good.

To compute cube roots modulo 2^256-2^32-977 (or field size), see this explanation here: https://github.com/bitcoin/bips/blob/cc177ab7bc5abcdcdf9c956ee88afd1052053328/bip-0324/reference.py#L165L191

src/fuzz.c

field and group elements

YafeiXie1 · 2023-08-30T17:55:25Z

This PR:
Field element construct function:
Added another input argument to determine the max magnitude.
Changed the mask for special case when the field element is normalized.

Group element construct function:
Remove fuzz_ge_fe_construct function.
Give Y coordinate the normalization/magnitude depending on the fuzz input.

I found an interesting situation: when I run the fuzzing test ‘fuzz_field_equal’ for the ‘secp256k1_fe_equal(a, b)’ (the magnitude of a and b cannot exceed 1 and 31 respectively), the test failed when the magnitude of ‘b’ equals to 31.
In the implementation of ‘secp256k1_fe_equal’ (in ‘field_impl.h’ file), after the negating function, the magnitude of ‘a’ increases to 2. Maybe the max magnitude for ‘b’ should be 30 in this function?

yafei-xie added 2 commits August 21, 2023 19:48

modify the fuzzing tests of field operations

3874a93

modified the fuzzing tests of field operations.

a0b3d0a

real-or-random added the assurance label Aug 21, 2023

sipa mentioned this pull request Aug 21, 2023

add fuzz tests about field operations. #1385

Closed

sipa reviewed Aug 24, 2023

View reviewed changes

YafeiXie1 and others added 5 commits August 29, 2023 14:27

Merge branch 'bitcoin-core:master' into master

bd19a5f

Merge branch 'master' of https://github.com/YafeiXie1/secp256k1

a89cc38

1

6f4f782

Merge branch 'master' of https://github.com/YafeiXie1/secp256k1

f6930b2

modified the function to construct field

ebfb2cf

and fuzzing tests of group operations

sipa reviewed Aug 29, 2023

View reviewed changes

Changed functions for the construction of

76d1fa5

field and group elements

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified fuzz tests about field operations #1407

Modified fuzz tests about field operations #1407

YafeiXie1 commented Aug 21, 2023

sipa commented Aug 21, 2023

sipa Aug 24, 2023

sipa Aug 24, 2023

sipa Aug 24, 2023

sipa Aug 24, 2023

sipa Aug 24, 2023

YafeiXie1 Aug 25, 2023

sipa Aug 24, 2023

sipa Aug 24, 2023

YafeiXie1 commented Aug 29, 2023

sipa Aug 29, 2023

sipa Aug 29, 2023

sipa Aug 29, 2023

sipa Aug 29, 2023

sipa Aug 29, 2023

sipa Aug 29, 2023

YafeiXie1 Aug 30, 2023

sipa Aug 30, 2023

YafeiXie1 commented Aug 30, 2023

Modified fuzz tests about field operations #1407

Are you sure you want to change the base?

Modified fuzz tests about field operations #1407

Conversation

YafeiXie1 commented Aug 21, 2023

sipa commented Aug 21, 2023

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

YafeiXie1 commented Aug 29, 2023

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

YafeiXie1 commented Aug 30, 2023