Opened 6 years ago

Closed 5 years ago

#16189 closed defect (fixed)

Ensure our scrypt interoperates with openssl's scrypt

Reported by: nickm Owned by: rl1987
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In e98aa30d555cb5a656d320a0f86ab5b3b1dce2db, openssl 1.1 adds an scrypt algorithm. Let's make sure we output the same results as them, so we can eventually not ship our own, and use theirs when it's available.

Child Tickets

Attachments (2)

test_scrypt.c (3.1 KB) - added by rl1987 5 years ago.
output.txt (1.7 KB) - added by rl1987 5 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:2 Changed 5 years ago by rl1987

Hey, does tor ship its own scrypt implementation? Code seems to use libscrypt.

comment:3 Changed 5 years ago by nickm

Ah, that's right. We do use libscrypt. But in that case we should make sure that libscrypt interoperates with openssl. :)

comment:4 Changed 5 years ago by rl1987

Status: acceptedneeds_review

I have preliminary patch for this: https://github.com/rl1987/tor/compare/libscrypt_eq_openssl

However, I did found one mismatch: OpenSSL seems to fail with the last test vector from draft-josefsson-scrypt-kdf-00 section 11.

comment:5 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

I have preliminary patch for this: ​https://github.com/rl1987/tor/compare/libscrypt_eq_openssl

Looks promising. We'll need to pull it out eventually, once we add some logic to stop linking libscrypt when openssl has scrypt... but for now it should be fine.

There's a logic bug, though: The code that calls EVP_PBE_scrypt() needs to be disabled entirely when openssl doesn't have that function, or our tests won't compile.

OpenSSL seems to fail with the last test vector from draft-josefsson-scrypt-kdf-00 section 11.

Hmmm. Does libscrypt pass with that test vector? If so, we should submit a bug to the openssl people so they don't release a broken scrypt implementation.

comment:6 in reply to:  5 Changed 5 years ago by rl1987

Replying to nickm:

I have preliminary patch for this: ​https://github.com/rl1987/tor/compare/libscrypt_eq_openssl

Looks promising. We'll need to pull it out eventually, once we add some logic to stop linking libscrypt when openssl has scrypt... but for now it should be fine.

There's a logic bug, though: The code that calls EVP_PBE_scrypt() needs to be disabled entirely when openssl doesn't have that function, or our tests won't compile.

I pushed one more commit that fixes this.

OpenSSL seems to fail with the last test vector from draft-josefsson-scrypt-kdf-00 section 11.

Hmmm. Does libscrypt pass with that test vector? If so, we should submit a bug to the openssl people so they don't release a broken scrypt implementation.

It seems to, since libscrypt is being tested independently in test_crypto_scrypt_vectors() and this test does not fail. I have isolated the failing part using #if 0 so that others could take a look. If you enable that part of the code, you will see that EVP_PBE_scrypt() returns the failure status if you try to compute key with the last test vector in Section 11.

comment:7 Changed 5 years ago by nickm

great; merged!

I'm going to leave this open till one of us can report a bug in openssl.

comment:8 Changed 5 years ago by rl1987

I wrote a small C program that demonstrates the OpenSSL bug. Furthermore, I have noticed that testcases I wrote don't always use scrypt APIs right. I have patch that fixes this: https://github.com/rl1987/tor/compare/bug16189_redux

Changed 5 years ago by rl1987

Attachment: test_scrypt.c added

Changed 5 years ago by rl1987

Attachment: output.txt added

comment:9 Changed 5 years ago by rl1987

test_scrypt.c was compiled with OpenSSL 1.1.0-dev (from their Github master) as follows:

gcc test_scrypt.c -I/usr/local/ssl/include -L/usr/local/ssl/lib -lcrypto -lscrypt

comment:10 Changed 5 years ago by nickm

Thanks! Have you reported the bug to the openssl team, or would you like me to do it?

comment:11 Changed 5 years ago by rl1987

I'd prefer you to do the bug reporting, if you already have experience of dealing with OpenSSL team. Also, I'd like you to review what we have here and ACK that I'm not wrong about anything.

comment:12 Changed 5 years ago by nickm

Ah, I've figured out the problem! It's not an openssl bug at all.

You just need to set maxmem to a much bigger value for this test to work. Otherwise, OpenSSL sees that it needs to allocate about 1408 Mb, and decides that's crazy and gives up.

comment:13 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Squashed and merged your bug16189_redux branch. Thanks!

Note: See TracTickets for help on using tickets.