radareorg / radare2

UNIX-like reverse engineering framework and command-line toolset
https://www.radare.org/
GNU Lesser General Public License v3.0
20.37k stars 2.97k forks source link

radare2 5.8.0: Does not compile with --with-openssl #21193

Closed nmeum closed 1 year ago

nmeum commented 1 year ago

While upgrading the Alpine Linux radare2 package from 5.7.0 to 5.8.0 I noticed that the 5.8.0 release no longer compiles with the --with-openssl configure flag using the following commands:

./configure \
    --prefix=/usr \
    --sysconfdir=/etc \
    --mandir=/usr/share/man \
    --localstatedir=/var \
    --with-syscapstone \
    --with-openssl \
    --with-syszip
make

Results in the following error message:

In file included from rvc_rvc.c:7:
../crypto/hash/sha2.c: In function 'r_sha256_init':
../crypto/hash/sha2.c:288:24: error: 'RSha256Context' {aka 'struct SHA256state_st'} has no member named 'state'
  288 |         memcpy (context->state, sha256_initial_hash_value, R_SHA256_DIGEST_LENGTH);
      |                        ^~
../crypto/hash/sha2.c:289:28: error: 'RSha256Context' {aka 'struct SHA256state_st'} has no member named 'buffer'
  289 |         r_mem_zero (context->buffer, R_SHA256_BLOCK_LENGTH);
      |                            ^~
../crypto/hash/sha2.c:290:16: error: 'RSha256Context' {aka 'struct SHA256state_st'} has no member named 'bitcount'
  290 |         context->bitcount = 0;
      |                ^~
../crypto/hash/sha2.c: At top level:
../crypto/hash/sha2.c:388:13: error: conflicting types for 'SHA256_Transform'; have 'void(RSha256Context *, const uint32_t *)' {aka 'void(struct SHA256state_st *, const unsigned int *)'}
  388 | static void SHA256_Transform(RSha256Context *context, const ut32 *data) {
      |             ^~~~~~~~~~~~~~~~
In file included from /home/soeren/src/aports/community/radare2/src/radare2-5.8.0/libr/include/r_hash.h:15,
                 from ../crypto/hash/sha2.h:41,
                 from ../crypto/hash/sha2.c:35:
/usr/include/openssl/sha.h:77:28: note: previous declaration of 'SHA256_Transform' with type 'void(SHA256_CTX *, const unsigned char *)' {aka 'void(struct SHA256state_st *, const unsigned char *)'}
   77 | OSSL_DEPRECATEDIN_3_0 void SHA256_Transform(SHA256_CTX *c,
      |                            ^~~~~~~~~~~~~~~~
../crypto/hash/sha2.c: In function 'SHA256_Transform':
../crypto/hash/sha2.c:393:32: error: 'RSha256Context' {aka 'struct SHA256state_st'} has no member named 'buffer'
  393 |         W256 = (ut32 *) context->buffer;
      |                                ^~
../crypto/hash/sha2.c:396:20: error: 'RSha256Context' {aka 'struct SHA256state_st'} has no member named 'state'
  396 |         a = context->state[0];
      |                    ^~
../crypto/hash/sha2.c:397:20: error: 'RSha256Context' {aka 'struct SHA256state_st'} has no member named 'state'
  397 |         b = context->state[1];
      |                    ^~
…

The issue seems to be a refactoring conducted in b2ff7de0f0b2d67e23f3887f63668f2f68271858. Since this commit rvc_rvc.c includes ../crypto/hash/sha2.c directly:

https://github.com/radareorg/radare2/blob/cd153c0260bc22d6e8ebc16db7557c4bbc9fb55b/libr/util/rvc_rvc.c#L5-L7

The sha2.c file seems to be radare2's internal SHA2 implementation. This implementation assumes that RSha256Context is defined according to the radare2-specific implementation of this struct in libr/include/r_hash.h and not a type-alias for SHA256_CTX from OpenSSL. If the latter is the case, then sha2.c attempts to access several fields in RSha256Context which are not provided by OpenSSL.

Also, it is not possible to remove the included of ../crypto/hash/sha2.c from rvc_rvc.c as the file seems to use several functions which are specific to radare2's SHA2 implementation and not supported by OpenSSL (e.g. r_sha256_init).

Removing the --with-openssl fixes the build issue.

trufae commented 1 year ago

There's no need to build r2 with openssl. the benefit is unnoticeable. but i'll fix that asap and will be tested in the CI and fixed for 5.8.2, it's ok to package it without this option enabled for alpine? thanks for the heads up

trufae commented 1 year ago

I've tried to compile with openssl3 and it reports many other errors related to deprecated APIs, so it seems like now we have to support more than one api for "ssl".

What this flag makes is to enable:

The reason for using ssl's crypt/hash implementations is mainly for performance, but it is kind of problematic to maintain. So one option would be to add an option to only add the https thing, but keep the r2 implementations for crypto/hash.

How does it sound? Having so many different ssl implementations is imho an issue, so i would prefer to remove this feature but i understand distros prefer to ship this in this way.

nmeum commented 1 year ago

Removal of OpenSSL support is honestly also fine with me personally. I wouldn't mind building the Alpine package without --with-openssl. I just reported this issue here since prior radare2 releases compiled fine with --with-openssl.

trufae commented 1 year ago

Fixed as discussed in mastodon:

same goes for meson (use_openssl and use_ssl_crypto options)

trufae commented 1 year ago

I'm leaving the responsability to get the crytpo code to work with more ssl libraries to the distro maintainers as long as they seem to be the only interested in this and i prefer to spend my smol spare time in other things

xambroz commented 1 year ago

Going without openssl is fine with packagers as long as the openssl code is not embedded in radare2.

trufae commented 1 year ago

Its not