Opened 3 years ago

Closed 3 years ago

#19872 closed task (implemented)

Introduce prefixed sign/verify functions

Reported by: asn Owned by: asn
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-proposed, tor-hs, TorCoreTeam201608
Cc: Actual Points: 0.4
Parent ID: Points: 0.3
Reviewer: dgoulet Sponsor: SponsorR-can

Description

prop224 wants us to sign messages prefixed with a constant string. For example:

   SIG is a signature, using AUTH_KEY, of all contents of the cell, up
   to but not including SIG. These contents are prefixed with the string
   "Tor establish-intro cell v1".

dgoulet also has plans for adding a prefix to the signature of the HS descriptor.

So, it would be ideal if we had some crypto util functions that will do that for us. We are talking about two functions called ed25519_sign_prefixed() and ed25519_checksig_prefixed() that are basically wrappers over ed25519_sign() and ed25519_checksig() that also accept a const char *prefix_str.

If we have these functions in upstream tor, it will be easier for us to write code for the various prop224 subsystems that need to be implemented.

Child Tickets

Change History (13)

comment:1 Changed 3 years ago by asn

I have an initial branch for this at bug19872_no_checks. The only missing thing is that it has no overflow checks, so I'm not putting this in needs_review yet.

comment:2 Changed 3 years ago by asn

Actual Points: 0.3
Status: newneeds_review

Please check my branch bug19872 for an implementation of those functions.

https://gitweb.torproject.org/user/asn/tor.git/commit/?h=bug19872&id=ce0820755f5c4b6b74bade2299adeb0c89a52335

comment:3 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

Two small things:

  • Need documentation on the returned value that is 0 on success and -1 on error. Either that or we document the return value of ed25519_sign() which has none documented in crypto_ed25519.c.
  • There is lots of strlen(prefix_str), could be nicer to do it once and then use it. Also, is there a chance that we would like to use this for a non string prefix? Would it be bad to just go with uint8_t * and pass the prefix len as param as well even though it's a string?

comment:4 Changed 3 years ago by dgoulet

Reviewer: dgoulet

comment:5 in reply to:  3 Changed 3 years ago by asn

Status: needs_revisionneeds_review

Replying to dgoulet:

Two small things:

  • Need documentation on the returned value that is 0 on success and -1 on error. Either that or we document the return value of ed25519_sign() which has none documented in crypto_ed25519.c.
  • There is lots of strlen(prefix_str), could be nicer to do it once and then use it. Also, is there a chance that we would like to use this for a non string prefix? Would it be bad to just go with uint8_t * and pass the prefix len as param as well even though it's a string?

Pushed a fixup commit that addresses your comments!

I did not change the function prototype to accept non-string prefixes since we don't need those yet. If we ever need them, let's do the change. Let me know if you think this is a stupid decision.

comment:6 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:7 Changed 3 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

code exists; needed for other stuff; can consider in 029

comment:8 Changed 3 years ago by nickm

Status: merge_readyneeds_revision

The duplicate code in these two functions should be extracted into a static function.

The test code should verify not only that "checksig" is the counterpart of "sign", but that "sign" produces the same output as if you had manually prefixed and signed the string.

make check-spaces fails.

comment:9 Changed 3 years ago by nickm

Owner: set to asn
Status: needs_revisionassigned

(setting owner.)

comment:10 Changed 3 years ago by nickm

Status: assignedneeds_revision

comment:11 in reply to:  8 Changed 3 years ago by asn

Status: needs_revisionneeds_review

Replying to nickm:

The duplicate code in these two functions should be extracted into a static function.

The test code should verify not only that "checksig" is the counterpart of "sign", but that "sign" produces the same output as if you had manually prefixed and signed the string.

make check-spaces fails.

Great points.

Addressed all the above and pushed a new branch as bug19872_v2. Please check it out.

Thanks!

comment:12 Changed 3 years ago by asn

Actual Points: 0.30.4

comment:13 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merging!

Note: See TracTickets for help on using tickets.