DSA cdf tests by karel-m · Pull Request #280 · libtom/libtomcrypt

karel-m · 2017-08-29T22:13:17Z

Another crypto test bench https://github.com/kudelskisecurity/cdf

Good news - our RSA implementation seems to pass. But we have couple of DSA related troubles.

The first commit fixes failures when ltc is on "validating" side. There are still some failures when ltc is on "signing" side, I'll try to analyze those later.

karel-m · 2017-08-29T22:37:47Z

Well, it turned out that the case when ltc is on "signing" side is OK. So this PR is complete.

karel-m · 2017-09-10T16:51:56Z

@sjaeckel ping

sjaeckel

Looks good, but somehow it seems now to be slightly inconsistent with what dsa_verify_key() checks, shouldn't this be updated as well? or should we probably have two versions of dsa_verify_key()? I'm just thinking that dsa_set_pqg_dsaparam() does the same as dsa_set_pqg() but lacks these newly added checks...

sjaeckel · 2017-09-11T17:22:48Z

We could e.g. have an internal (say LTC_SOURCE protected) dsa_check_key() which is then called from within dsa_verify_key(), dsa_set_pqg_dsaparam() and dsa_set_pqg() (and probably others that I missed?). This one would then only have these trivial checks without all the more complex stuff.

karel-m · 2017-09-11T21:37:06Z

something like this?

sjaeckel · 2017-09-11T22:04:36Z

Do we really need such extensive tests? (the 2 exptmod's?)
Besides that I realized that the name verify_key() is nonsense, therefore I've pushed my proposal to the pr/fix-dsa-cdf-2 branch. What do you think?

Can we make dsa_int_validate_key() a bit less computational expensive by moving the 2 exptmods to dsa_validate_key()?
Wouldn't it make sense to use LTC_MILLER_RABIN_REPS for the prime checks?
With your last commit you removed stuff that you added in the commit before (in dsa_set_pqg())
a. Was that accidental or itentional?
b. The 'newly added' was three mp_cmp_d() whereas dsa_verify_key_ex() has only two + a different logic, can you please check back which is correct!?

karel-m · 2017-09-11T22:14:49Z

we should perhaps keep the name dsa_verify_key as it is part of 1.17 API

sjaeckel · 2017-09-11T22:18:19Z

we should perhaps keep the name dsa_verify_key as it is part of 1.17 API

damn, sorry - sure

I'll just update the branch

karel-m · 2017-09-11T22:18:43Z

With your last commit you removed stuff that you added in the commit before (in dsa_set_pqg())

The thing is that at the end of dsa_set_pqg() we do not have a complete key, therefore we cannot do the key check/validation.

sjaeckel · 2017-09-11T22:19:56Z

The thing is that at the end of dsa_set_pqg() we do not have a complete key, therefore we cannot do the key check/validation.

but it looks to me like part of it was required for the cdf tests to pass, right?

sjaeckel · 2017-09-11T22:25:44Z

I'll just update the branch

pushed

karel-m · 2017-09-11T22:29:36Z

What we need is to avoid loading/setting/creating a DSA key with p or g or q (not sure what was the main trouble maker) with value 0.

It would be better to check it in dsa_set_pqg but it is IMO fine to do it a bit later during dsa_set_key (where we have the key complete).

karel-m · 2017-09-12T05:11:46Z

I have slightly updated your pr/fix-dsa-cdf-2 - which works and can be merged

sjaeckel · 2017-09-12T15:33:22Z

What we need is to avoid loading/setting/creating a DSA key with p or g or q (not sure what was the main trouble maker) with value 0.

Okay, but the different parts of the implementation I saw had checks for {p,g,q} > 1 |p| > 1 and I don't know what else. Reading quickly through FIPS 186-4 I couldn't determine what is the correct approach so can you please check again what we really have to do?

I think it'd make sense to have 2 private functions dsa_int_validate_group_params() and dsa_int_validate_key() and 1 API function dsa_verify_key(). This allows us to correctly verify each step of the key import/generation.

Flow could be something like this:

int dsa_verify_key() {
 check_prime(q);
 check_prime(p);
 return dsa_int_validate_key();
}

int dsa_int_validate_group_params() {
 ...
}

int dsa_int_validate_key() {
 if (dsa_int_validate_group_params()) return;
 ...
}

What do you think?

karel-m · 2017-09-14T08:41:01Z

I'll check FIPS 186-4 but meanwhile another approach for dsa_int_validate_*

karel-m · 2017-09-14T15:44:51Z

@sjaeckel could you please review this PR again?

sjaeckel

Looks very good! Thanks!

sjaeckel · 2017-09-14T16:18:25Z

+   void *tmp1, *tmp2;
+   int  err;
+
+   *stat = 0;


ew, de-referencing stat before checking for NULL

sjaeckel · 2017-09-14T16:21:50Z

 int dsa_export(unsigned char *out, unsigned long *outlen, int type, dsa_key *key);
 int dsa_verify_key(dsa_key *key, int *stat);
-
+#ifdef LTC_SOURCE


can you please add the obligatory /* internal helper functions */ comment

sjaeckel · 2017-09-14T16:24:39Z

+   err   = CRYPT_OK;
+   *stat = 1;
+error:
+   mp_clear_multi(tmp1, tmp2, NULL);


can you please (always) reverse the order when clearing MPI's

sjaeckel · 2017-09-14T16:31:02Z

+      goto LBL_ERR;
+   }
+   if (stat == 0) {
+      err = CRYPT_INVALID_PACKET;


Looks like we inconsistently return either CRYPT_INVALID_PACKET or CRYPT_INVALID_ARG, we should always use the same error code. I'd say we should go for CRYPT_INVALID_PACKET (an alternative could be to introduce a new error code, but not sure if that's really necessary).

sjaeckel · 2017-09-14T16:31:40Z

-   /* first make sure key->q and key->p are prime */
-   if ((err = mp_prime_is_prime(key->q, 8, &res)) != CRYPT_OK) {
+   /* key->q prime? */
+   if ((err = mp_prime_is_prime(key->q, LTC_MILLER_RABIN_REPS, &res)) != CRYPT_OK) {


karel-m · 2017-09-14T17:11:07Z

Oh sorry, I should perhaps wait for Travis-CI

sjaeckel · 2017-09-14T17:12:12Z

Too late, the build's going to fail ;)

sjaeckel · 2017-09-14T17:12:34Z

but nvm, as the merge will be built as well...

karel-m requested a review from sjaeckel August 30, 2017 09:12

karel-m force-pushed the pr/fix-dsa-cdf branch from f4e3820 to 719be6e Compare September 1, 2017 07:20

karel-m mentioned this pull request Sep 1, 2017

DSA vs. wycheproof #274

Closed

karel-m force-pushed the pr/fix-dsa-cdf branch from 719be6e to 0d090f0 Compare September 2, 2017 12:17

karel-m force-pushed the pr/fix-dsa-cdf branch from 0d090f0 to 656c57b Compare September 10, 2017 12:02

sjaeckel reviewed Sep 11, 2017

View reviewed changes

karel-m and others added 10 commits September 14, 2017 17:21

add basic validity tests to dsa_set

2505e3b

introducing dsa_verify_key_ex

053ba6d

re-factor & re-name internal dsa key validation

1625ce4

fix dsa_int_validate_key related compiler warnings

aa5b9da

another approach for dsa_int_validate_*

5fb4c9f

FIPS 186-4 DSA validity tests

1ea4fec

fix dsa_int_validate_xy

c806ea1

dsa_int_validate_primes & LTC_MILLER_RABIN_REPS

45b6b94

do dsa_int_validate_pqg in dsa_set_pqg_dsaparam

9765bef

move qord trest to dsa_int_validate_pqg

fd94e95

do dsa_int_validate_* in dsa_import

444d9f3

karel-m force-pushed the pr/fix-dsa-cdf branch from 0eb5481 to 444d9f3 Compare September 14, 2017 15:38

sjaeckel requested changes Sep 14, 2017

View reviewed changes

karel-m added 4 commits September 14, 2017 18:48

fix de-referencing stat before checking for NULL

d91d594

add comment #ifdef LTC_SOURCE + internal helper functions

6200f30

mp_clear_multi - reverse the order

a990a82

if dsa_int_validate_* fails return consistently CRYPT_INVALID_PACKET

bb6a7e1

sjaeckel approved these changes Sep 14, 2017

View reviewed changes

karel-m merged commit 5934eb3 into release/1.18.0 Sep 14, 2017

karel-m deleted the pr/fix-dsa-cdf branch September 14, 2017 17:10

sjaeckel added this to the v1.18.0 milestone Sep 14, 2017

Conversation

karel-m commented Aug 29, 2017

Uh oh!

karel-m commented Aug 29, 2017

Uh oh!

karel-m commented Sep 10, 2017

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

sjaeckel commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karel-m commented Sep 11, 2017

Uh oh!

sjaeckel commented Sep 11, 2017

Uh oh!

karel-m commented Sep 11, 2017

Uh oh!

sjaeckel commented Sep 11, 2017

Uh oh!

karel-m commented Sep 11, 2017

Uh oh!

sjaeckel commented Sep 11, 2017

Uh oh!

sjaeckel commented Sep 11, 2017

Uh oh!

karel-m commented Sep 11, 2017

Uh oh!

karel-m commented Sep 12, 2017

Uh oh!

sjaeckel commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karel-m commented Sep 14, 2017

Uh oh!

karel-m commented Sep 14, 2017

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

sjaeckel Sep 14, 2017

Choose a reason for hiding this comment

Uh oh!

sjaeckel Sep 14, 2017

Choose a reason for hiding this comment

Uh oh!

sjaeckel Sep 14, 2017

Choose a reason for hiding this comment

Uh oh!

sjaeckel Sep 14, 2017

Choose a reason for hiding this comment

Uh oh!

sjaeckel Sep 14, 2017

Choose a reason for hiding this comment

Uh oh!

karel-m commented Sep 14, 2017

Uh oh!

sjaeckel commented Sep 14, 2017

Uh oh!

sjaeckel commented Sep 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sjaeckel commented Sep 11, 2017 •

edited

Loading

sjaeckel commented Sep 12, 2017 •

edited

Loading