Opened 5 years ago

Closed 5 years ago

#14188 closed defect (fixed)

OpenSSL 1.1.0-dev change: builds without deprecated functions by default

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: 0.2.6.2-alpha
Severity: Keywords: openssl tor-build teor-lorax nickm-patch
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by teor)

Due to the following OpenSSL change:

  *) config has been changed so that by default OPENSSL_NO_DEPRECATED is used.
     Access to deprecated functions can be re-enabled by running config with
     "enable-deprecated". In addition applications wishing to use deprecated
     functions must define OPENSSL_USE_DEPRECATED. Note that this new behaviour
     will, by default, disable some transitive includes that previously existed
     in the header files (e.g. ec.h will no longer, by default, include bn.h)
     [Matt Caswell]

Building tor git with the latest OpenSSL 1.1.0-dev git causes the following errors on OS X with clang (edited for brevity):

  CC       src/common/tortls.o
src/common/crypto.c:408:3: error: implicit declaration of function
      'ERR_remove_state' is invalid in C99
  ERR_remove_state(0);
src/common/crypto.c:1783:19: error: implicit declaration of function
      'DH_generate_parameters' is invalid in C99
  dh_parameters = DH_generate_parameters(DH_BYTES*8, DH_GENERATOR, NULL, NULL);
src/common/crypto.c:1783:19: note: did you mean 'DH_generate_parameters_ex'?
/test/tor/openssl-install-x86_64/include/openssl/dh.h:213:5: note: 
      'DH_generate_parameters_ex' declared here
int     DH_generate_parameters_ex(DH *dh, int prime_len,int generator, B...
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  CC       src/trunnel/pwbox.o
src/common/crypto.c:3131:3: error: implicit declaration of function
      'CRYPTO_set_id_callback' is invalid in C99
  CRYPTO_set_id_callback(tor_get_thread_id);
4 errors generated.
make[1]: *** [src/common/crypto.o] Error 1
src/common/tortls.c:675:27: error: implicit declaration of function 'BN_bin2bn'
      is invalid in C99
    if (!(serial_number = BN_bin2bn(serial_tmp, sizeof(serial_tmp), NULL)))
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/common/tortls.c:713:5: error: implicit declaration of function
      'BN_clear_free' is invalid in C99
    BN_clear_free(serial_number);
src/common/tortls.c:1069:16: error: implicit declaration of function
      'BN_num_bits' is invalid in C99
    if (rsa && BN_num_bits(rsa->n) == 1024)
src/common/tortls.c:1069:31: error: incomplete definition of type
      'struct rsa_st'
    if (rsa && BN_num_bits(rsa->n) == 1024)
/test/tor/openssl-install-x86_64/include/openssl/ossl_typ.h:147:16: note: 
      forward declaration of 'struct rsa_st'
typedef struct rsa_st RSA;
src/common/tortls.c:1072:7: error: implicit declaration of function 'RSA_free'
      is invalid in C99
      RSA_free(rsa);
src/common/tortls.c:1072:7: note: did you mean 'SSL_free'?
/test/tor/openssl-install-x86_64/include/openssl/ssl.h:2201:6: note: 'SSL_free'
      declared here
void    SSL_free(SSL *ssl);

Building OpenSSL with ./Configure enable-deprecated and including -DOPENSSL_USE_DEPRECATED in the CPPFLAGS seems to require a few tries to actually work. (I don't think it likes parallel builds.)

Building tor with this new version then works fine.

causes a linker error: (This is actually due to OpenSSL not working with parallel builds.)

Undefined symbols for architecture x86_64:
  "_EVP_aes_128_ctr", referenced from:
      _aes_new_cipher in libor-crypto.a(aes.o)

We should probably fix this by 0.2.6-final, otherwise it won't be able to be built with OpenSSL 1.1.0 dev out of the box.

But how are we going to cope with platforms that build OpenSSL without deprecated functions?
Conditionalise on #if OPENSSL_USE_DEPRECATEDs in the code?
Advise them not to?
It seems like this change could cause a huge mess.

Child Tickets

Change History (9)

comment:1 Changed 5 years ago by teor

Keywords: teor-lorax added
Priority: normalmajor

The link error appears to be associated with the known failure cases in tor's library detection.

Adding an explicit -L${OPENSSL_DIR}/lib to the linker flags then reveals the actual likner issue with 1.1.0-dev:

Undefined symbols for architecture x86_64:
  "_BN_is_word", referenced from:
      _crypto_pk_public_exponent_ok in libor-crypto.a(crypto.o)
      _crypto_get_stored_dynamic_dh_modulus in libor-crypto.a(crypto.o)

I'll try to work out why this is happening.

By the way, I'd like someone who knows a bit more about tor's makefiles and library detection to fix this one :-)

comment:2 Changed 5 years ago by teor

That's what I get for using the OpenSSL git sources - someone changed BN_is_word from a macro to a function, but for some reason it's not in the library.

comment:3 Changed 5 years ago by teor

Description: modified (diff)

It's the parallel builds that were causing the linking errors.

comment:4 in reply to:  description Changed 5 years ago by nickm

Replying to teor:

But how are we going to cope with platforms that build OpenSSL without deprecated functions?
Conditionalise on #if OPENSSL_USE_DEPRECATEDs in the code?
Advise them not to?
It seems like this change could cause a huge mess.

I think our best bet is to just move from using the deprecated functions to using whatever replaced them.

In some cases, the functions above aren't deprecated; we'll just need to add extra #includes to get them.

comment:5 Changed 5 years ago by teor

We probably want to wait for OpenSSL 1.1.0-dev to stabilise anyway.
I've just spent a few hours chasing down bugs in the ecp_nistp*.c files which cause tor's benchmarks to fail. Someone put an extra & before quite a few variables, and then ignored the resultant "incompatible pointer type" warnings.

comment:6 Changed 5 years ago by nickm

yeah. There's no harm in fixing what we can when we notice it, but we should probably expect a few more things to break along the way.

comment:7 Changed 5 years ago by nickm

Priority: majornormal
Status: newneeds_review

I think I have the above issues fixed in a branch 14188_part1 in my public repo.

There will be more issues before 1.1.0 goes stable, but why not merge this much now?

comment:8 Changed 5 years ago by nickm

Keywords: nickm-patch added

comment:9 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged as still-looks-right, very simple.

Please reopen if 1.1.0 still breaks. :)

Note: See TracTickets for help on using tickets.