Opened 2 years ago

Closed 2 years ago

#20148 closed enhancement (implemented)

Add AES256 support to crypto_cipher_t

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: crypto
Cc: Actual Points: 0.2
Parent ID: Points: 0.2
Reviewer: dgoulet Sponsor: SponsorR-can

Description

Proposal 224 specifies the use of AES256 for encrypting hidden service descriptors. We need an aes256 backend.

Child Tickets

Change History (6)

comment:1 Changed 2 years ago by nickm

Sponsor: SponsorR-can

See branch aes256 in my public repository. It adds aes256-ctr support, and cleans up the surrounding code a bit.

comment:2 Changed 2 years ago by nickm

Status: newneeds_review

comment:3 Changed 2 years ago by dgoulet

Keywords: crypto added
Status: needs_reviewneeds_information

Global observation. If the goal is to keep this change as minimal as possible, I guess the current code is fine.

However, I see that we have crypto_cipher_new() that only does 128 bits. Then we have crypto_cipher_new_with_iv() that also only does 128 bits and then this new one crypto_cipher_new_with_bits() (and the with_iv) that we can control the bits.

Wouldn't be simpler to have all crypto_cipher_new* to take a bit size? Would make the entire API more symmetric. I see very few callsites of those, actually 6 outside of crypto.c.

comment:4 Changed 2 years ago by nickm

Status: needs_informationneeds_review

Hm. I think that merging the old and new APIs together should probably be another ticket.

comment:5 Changed 2 years ago by dgoulet

Actual Points: .20.2
Points: .20.2
Reviewer: dgoulet
Status: needs_reviewmerge_ready

I guess yes, some sort of ticket to "unify" our aes API.

lgtm;

actual-review-point: 0.2

comment:6 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

okay. merging and hoping for the best

Note: See TracTickets for help on using tickets.