这是indexloc提供的服务，不要输入任何密码
Skip to content

Signatures #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 13 commits into from
Jun 17, 2014
Merged

Signatures #2

merged 13 commits into from
Jun 17, 2014
+211 −120

Conversation

@dchest
Copy link
Owner

@dchest dchest commented Jun 14, 2014

Changes from https://github.com/devi/tweetnacl-js/tree/sha512 + new tests for signatures.
Signature test signs a random message in JS, then signs the same message with a C program using the latest tweetnacl.c, then compares the result and tries to open it.

To run sign test,

 cd tests
 make test

Currently sign test doesn't pass fully, e.g.:

Test #28 (Message length: 40)
sign - OK
open - OK

Test #29 (Message length: 40)
! signatures don't match
JS:  AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAg5/uin4OUr0A3BGf1d6EjsjCeR4mWMOULHTUBe6hEAA== 
C :  0/NJ+frDasaDY8Y1PH/MPQRCvS/bMND9z/GwYOCOJKZ4q1C8/IWjhVNqguenzXuWahVePtrjMQZVjldDqeyUBw==
open - OK

Test #30 (Message length: 40)
sign - OK
open - OK

Test #31 (Message length: 44)
sign - OK
open - OK

Test #32 (Message length: 44)
sign - OK
open - OK

Test #33 (Message length: 44)
sign - OK
open - OK

Test #34 (Message length: 48)
! signatures don't match
JS:  VFrc0cgwtkAED6wgjrGtzYGlWhR+53f8sfzYGLiuP5PZLrCs/S9YWe/d5F4HJ+3BEuGIsaC8YgfAW0jcba3RAA== 
C :  VFrc0cgwtkAED6wgjrGtzYGlWhR+53f8sfzYGLiuP5OZ5spi8J982VDamORfRRiSOfC3/dOGZkcCbFSmVmCpAw==
! verification failed

@dchest dchest mentioned this pull request Jun 14, 2014
Closed
@devi
Copy link
Contributor

devi commented Jun 16, 2014

I've found most of the culprit, need more test on other environment, should I create a new pull ?

@dchest
Copy link
Owner Author

dchest commented Jun 16, 2014

@devi thanks! I'm testing it right now. No need for a new PR, I'll just pull directly from you.

@dchest
Copy link
Owner Author

dchest commented Jun 16, 2014

Right now only one of the 40-byte messages fail:

Test #29 (Message length: 40)
! signatures don't match
JS:  AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAARCiQVUm8YtmjAwBqRm38I3/i/J8DDiF+4aw+6G4tECA== 
C :  ZUPAU8yIRM4Fzs3dizDYBzqzILi2HUixBXel9KtvB25679DwhECjDd+z9sgXC/Nd/EooOhGlkwOwQMeRVSxiCQ==

(AAAA... is zeros in base64)

(I'm using this version: https://github.com/devi/tweetnacl-js/commit/f9a72ffa3708c89c6574a13ae21bba1a709e18ec)

@dchest
Copy link
Owner Author

dchest commented Jun 16, 2014

I've replaced a random message with repeats of "x" like this in tests/sign.js check():

-var msg = crypto.randomBytes(i).toString('base64');
+var msg = new Array(i).join('x');

and it fails when length is 28:

Test #29 (Message length: 28)
! signatures don't match
JS:  AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACtTzhlkbOYaklFGb2EKe5nmHd+NjpYO+B/JLZqgIAABQ== 
C :  scWbBbC8PefhsB0gXRW6LcGDZNdOjZcr1peVoimzZFryO6fyDtvbzn89xJN0xnItcPg2dsbiU+zaSk+p6MunDw==

Do you think it's crypto_hash's or crypto_sign's fault? I should probably create a crypto_hash test like sign.js.

@dchest
Copy link
Owner Author

dchest commented Jun 16, 2014

Added hash test:

make test_hash

It passes, so it looks like SHA-512 implementation is OK.

@devi
Copy link
Contributor

devi commented Jun 16, 2014

@dchest Sorry I didn't saw your 22360c7 commit.
I've pushed another test to my sha512 branch.
It looks like sign test passes 28 bytes bugs.

@dchest
Copy link
Owner Author

dchest commented Jun 16, 2014

@devi Awesome! I've tried and it runs 100 sign tests successfully. I'll let it run with random strings for a few hours to be sure, then go through the code again, and then merge (not sure if I'll have time today, so give me a few days :). Thanks again for the great work! 👍

@devi
Copy link
Contributor

devi commented Jun 16, 2014

@dchest Thanks to you! You've teached me a lot from this pull.

dchest added 10 commits June 17, 2014 10:11
@dchest
Nits found by jshint.
ac75049
@dchest
Use for loops everywhere tweetnacl.c uses FOR for consistency and ease
04b0812 
of review.
@dchest
Reorganize signature code; make test fail again.
604d859 
* Reordered functions so that they appear in order similar to
  tweetnacl.c.

* Replaced while loops with for loops to match tweenacl.c.

* In pack25519, change t from empty array to new gf() -- which makes the
  test fail again on 29-byte messages unless we observe n or t with
  console.log. This needs investigation.
@dchest
Use hex numbers as in tweetnacl.c.
438ce27
@dchest
Add signature test vector.
8596730
@dchest
Use 'exit' instead of 'close' for child process.
54cef3a
@dchest
Workaround for potential Node bug (see Issue 5).
192e7cb
@dchest
Switch back to 'close' event of child process.
f406db9
@dchest
Back to testing random strings instead of 'xxxx..'
e2bca59
@dchest
Add chash to .gitignore.
242388c
@dchest dchest merged commit 242388c into master Jun 17, 2014
@dchest
Copy link
Owner Author

dchest commented Jun 17, 2014

All right, I merged it! Thanks again.

Also, I think one of the test failures was caused by a Node bug: #5

@dchest dchest deleted the sign branch June 17, 2014 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dchest @devi