DCIT / perl-CryptX

https://metacpan.org/pod/CryptX
Other
35 stars 23 forks source link

Isolate libtomcrypt 'der_*' functions #79

Closed atoomic closed 2 years ago

atoomic commented 2 years ago

When compiling Perl or a binary with different versions of tomcrypt this is going to have unexpected behaviors as we cannot guarantee which flavor of the C function is used.

This changeset is isolating all the 'der_' C functions by adding a prefix 'cryptxder' so they are unique and would not conflict with another library linked to our binary.

Recipe replacement rules applied: replace ' der_' ' cryptxder' replace '(der_' '(cryptxder' replace ')der_' ')cryptxder' replace '!der_' '!cryptxder' replace '=der_' '=cryptxder' replace '= der_' '= cryptxder' replace 'cryptx_der_to_pem' 'der_to_pem' lib/*/..pm

Note: we should consider adding this to src/update-libtom.pl

atoomic commented 2 years ago

I just run into some issues with a compiled binary which was using libtomcrypt from multiple sources and linked against multiple versions if the 'der_' helpers

In my example these two libraries are providing 'der_*' helpers

der_length_integer is available from both libraries but only der_decode_integer is availabe from CryptX.so

At the end the binary is confused by which version to use

atoomic commented 2 years ago

also reported the issue to heimdal as https://github.com/heimdal/heimdal/issues/911

FGasper commented 2 years ago

Looks like this addresses the same issue as reported in #68.

karel-m commented 2 years ago

As I mentioned in #68 I do know how to fix this properly. The solution I would like will create CryptX.so in a way that it does not export "unwanted" symbols.

atoomic commented 2 years ago

we should consider something like this by default if possible to do not advertise the symbols

CFLAGS="-Wl,--exclude-libs,ALL -fvisibility=hidden" perl Makefile.PL
karel-m commented 2 years ago

Well, CFLAGS hack did not work for me, the resulting CryptX.so still exports a tons of functions.

But my experiments with LDDLFLAGS => "$Config{lddlflags} -Wl,--version-script=ldscript.version" seems to be promising.

karel-m commented 2 years ago

@atoomic could you please try this simple patch

diff --git a/Makefile.PL b/Makefile.PL
index dcc55858..101478e1 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -62,6 +62,9 @@ else {
     MYEXTLIB => "src/liballinone$Config{lib_ext}",
     clean    => { 'FILES' => join(' ', @myobjs, "src/liballinone$Config{lib_ext}") },
   );
+  if ($Config{ld} =~ /gcc|g\+\+/) {
+    push @EUMM_INC_LIB, (LDDLFLAGS => "$Config{lddlflags} -Wl,--exclude-libs,ALL");
+  }
 }

 my %eumm_args = (

Any suggestions how to improve if (..) condition are appreciated.

karel-m commented 2 years ago

BTW this kind of "protection" should be IMO a feature of perl toolchain.

atoomic commented 2 years ago

@karel-m sure I m going to test but I think the one we want is -fvisibility=hidden I was working on a similar patch

atoomic commented 2 years ago

my current patch looks like this, testing your now

diff --git a/Makefile.PL b/Makefile.PL
index dcc5585870..541c110121 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -31,6 +31,11 @@ else {
   $mycflags .= " $ENV{CFLAGS} "   if $ENV{CFLAGS};
   $mycflags .= " $ENV{CPPFLAGS} " if $ENV{CPPFLAGS};

+  # avoid der_* functions to conflict with libhx509.so
+  if ( $^O ne 'MSWin32' && $Config{ld} =~ /gcc|g\+\+/) {
+      $mycflags .= "-fvisibility=hidden";
+  }
+
   #### remove all lto hacks - https://github.com/DCIT/perl-CryptX/issues/70
   ## #FIX: gcc with -flto is a trouble maker see https://github.com/DCIT/perl-CryptX/issues/32
   ## #FIX: another issue with "-flto=auto" see https://github.com/DCIT/perl-CryptX/pull/66
atoomic commented 2 years ago

@karel-m I can confirm that your patch is working fine, thank you so much for your quick feedback maybe add a $^O ne 'MSWin32' ?

atoomic commented 2 years ago

We should not merge this change

atoomic commented 2 years ago

I reopened the PR with your suggested patch after confirming it was working fine

atoomic commented 2 years ago

Thanks again @karel-m for the patch

karel-m commented 2 years ago

now on CPAN as CryptX-0.075_001