Opened 2 years ago

Closed 2 years ago

#26815 closed enhancement (worksforme)

Use NSS for our symmetric crypto, digests, and PRNG.

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 035-roadmap-subticket, 035-triaged-in-20180711
Cc: nickm, gk, ahf, catalyst Actual Points:
Parent ID: #26631 Points:
Reviewer: catalyst Sponsor: Sponsor8-can


Child Tickets

Change History (8)

comment:1 Changed 2 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 2 years ago by nickm

Cc: ahf catalyst added
Status: acceptedneeds_review is the PR here, and it also includes #26816. The branch name is nss_symmetric

comment:3 Changed 2 years ago by asn

Reviewer: catalyst

comment:4 Changed 2 years ago by catalyst

Status: needs_reviewneeds_revision

Thanks for working on this! At first glance it seems reasonable. I'm still looking at the diffs more carefully.

I did notice that github is saying it can't auto-merge, so some rebasing might be needed. Also, when building the (unmerged) branch on Ubuntu 16.04.4 amd64 with libnss3-dev 2:3.28.4-0ubuntu0.16.04.3, I get this warning:

In file included from ../src/lib/crypt_ops/crypto_nss_mgt.c:20:0:
/usr/include/nss/ssl.h:397:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
 SSL_IMPORT unsigned int SSL_SignatureMaxCount();

This halts the build if configured with --enable-fatal-warnings. I don't know if this is a known bug in the headers, or if it's because of something else we did.

comment:5 Changed 2 years ago by nickm

That warning is of a problem in the header: It should say SSL_SignatureMaxCount(void), but it omits the void.

I had hoped to fix that by wrapping the includes in DISABLE/ENABLE_GCC_WARNINGS; I might have missed one, or maybe it isn't working. I'll check soon.

comment:6 Changed 2 years ago by nickm

Status: needs_revisionneeds_review

Ah, pfew. Looks like I just missed one. I've made the fix to this branch. (I'm not planning to rebase later branches till this one is merged, though.)

comment:7 Changed 2 years ago by catalyst

Status: needs_reviewneeds_revision

comment:8 Changed 2 years ago by nickm

Resolution: worksforme
Status: needs_revisionclosed

Closing these NSS tickets in favor of #26818 (the RSA one), since that one includes all the code from these.

Note: See TracTickets for help on using tickets.