Opened 4 years ago

Closed 4 years ago

#17663 closed enhancement (implemented)

Add SHA512 support in crypto.c

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: gtank Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

gtank added SHA512 support to crypto.c in https://github.com/gtank/tor/tree/8961-big

This ticket tracks the SHA512 support separately to the replay cache work in #8961.

Child Tickets

Change History (4)

comment:1 Changed 4 years ago by teor

Status: newneeds_revision

Thanks for this feature, and thanks for the unit tests!

Code review:

The code looks great overall. Just a few minor suggestions.

In crypto_digest_smartlist_prefix:

  • if an unknown digest is passed to the function, let's warn, tor_fragile_assert(), and wipe memory like crypto_digest_get_digest; rather than providing SHA256 as a default.

In crypto_digest512:

  • the function returns 1 on failure, not -1. (This is likely a copy-paste of a similar error on crypto_digest256 that's fixed in #17655.)

In crypto_digest_local:

  • Using this function will give 32-bit platforms less protection. (Typically, we just let them take the performance hit.)
  • I don't like the idea of truncation to 160 bits, although it might be a while before attacks on 160 bits become feasible. We could let the user specify a length, perhaps by adding SHA_LOCAL_* equivalents of the SHANNN_* constants.
  • the function never returns -1. If you can check for failures and return -1, please do, otherwise, just document it as always returning 0. (This is likely a copy-paste of a similar error on crypto_digest256 that's fixed in #17655.)

Are you happy if we delete crypto_digest_local? Or is there a specific circumstance where we might use it?

comment:2 Changed 4 years ago by gtank

Here's a branch for just SHA512: https://github.com/gtank/tor/tree/feature17663

From the '8961-big' branch, it excludes the whole crypto_digest_local thing and fixes both the crypto_digest_smartlist_prefix unknown alg behavior and the crypto_digest512 comment error.

comment:3 Changed 4 years ago by teor

Status: needs_revisionneeds_review

Thanks, looks great. Since this is a new feature that comes with unit tests, let's get it merged.

comment:4 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks good to me; merged, with a minor tweak. (By convention, tor's xyz_free() functions always check for NULL.)

Note: See TracTickets for help on using tickets.