Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake: link gost.so statically to its caller #384

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yanovich
Copy link

If any executable loads gost.so, the executable already either has
libcrypto.so loaded or is statically linked against libcrypto.a.
Anyway it already has all libcrypto (and libssl) symbols present.

Without this patch gost.so is linked against libcrypto,so. As
a result, a diamond dependency is introduced. If gost.so is then
loaded by an executable which is statically linked against libcrypto,
ld will insist on loading libcripto.so, despite the executable
already having all necessary symbols. When the executable is statically
linked, shared objects for libcrypto and libssl are usually not built,
ls won't find them, and the caller will crush.

The patch removes this unnecessary link dependency in gost.so,
allowing it to be used by executables which are statically linked
against libcrypto.

@yanovich
Copy link
Author

Here is the diff between original state and the fixed one:

$ cat .gitignore 
*.ts
*.o
*.o.d
*.a
*.bin
a.out
compiler_depend.*
bin/
Testing/
$ git diff
diff --git a/CMakeFiles/gost_engine.dir/build.make b/CMakeFiles/gost_engine.dir/build.make
index 83578e0..32ec190 100644
--- a/CMakeFiles/gost_engine.dir/build.make
+++ b/CMakeFiles/gost_engine.dir/build.make
@@ -94,7 +94,6 @@ bin/gost.so: CMakeFiles/gost_engine.dir/gost_eng.c.o
 bin/gost.so: CMakeFiles/gost_engine.dir/build.make
 bin/gost.so: libgost_core.a
 bin/gost.so: libgost_err.a
-bin/gost.so: /usr/lib/x86_64-linux-gnu/libcrypto.so
 bin/gost.so: CMakeFiles/gost_engine.dir/link.txt
        @$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --green --bold --progress-dir=/home/s/src/crypto/openssl/gost-engine/build/CMakeFiles --progress-num=$(CMAKE_PROGRESS_2) "Linking C shared module bin/gost.so"
        $(CMAKE_COMMAND) -E cmake_link_script CMakeFiles/gost_engine.dir/link.txt --verbose=$(VERBOSE)
diff --git a/CMakeFiles/gost_engine.dir/link.txt b/CMakeFiles/gost_engine.dir/link.txt
index 50c9b2b..b3b08f5 100644
--- a/CMakeFiles/gost_engine.dir/link.txt
+++ b/CMakeFiles/gost_engine.dir/link.txt
@@ -1 +1 @@
-/usr/bin/cc -fPIC -O2 -shared  -o bin/gost.so CMakeFiles/gost_engine.dir/gost_eng.c.o  libgost_core.a libgost_err.a /usr/lib/x86_64-linux-gnu/libcrypto.so 
+/usr/bin/cc -fPIC -O2 -shared  -o bin/gost.so CMakeFiles/gost_engine.dir/gost_eng.c.o  libgost_core.a libgost_err.a

@beldmit
Copy link
Contributor

beldmit commented Jan 10, 2022

I'm not sure that I want to link it this way by default.

@yanovich
Copy link
Author

There are possible use cases on linux. E.g., the official docker image of node.js is built by statically linking against libcrypto.a. gost.so just works there with this patch and normal openssl.conf. This patch will also solve segfaults whengost.so is loaded by a different version of libcrypto than it was built against (#363).

@yanovich
Copy link
Author

Please restart cancelled checks. They failed in before_script.sh by curl timeout.

@beldmit
Copy link
Contributor

beldmit commented Jan 11, 2022

I'd consider this option as a separate build variant.

@yanovich
Copy link
Author

Maybe I am wrong to call this type of linking static. It is inverse dynamic. GNU ld loads the engine then verifies that all symbol are resolved. Without my patch ld loads libcrypto.so which was present at link time before verification unless the exact version of it is already loaded. You don't want that. This causes subtle bugs like segfaults on version mismatch.

gost.so is not a valid shared library, it is a plugin for libcrypto (unlike 'libgost.so which is). When you invoke a function with side effects from gost.so, you expect that the application will change state. You link plugin as a shared library now, when gost.so invokes a function with side effects, it manipulates a separate global state, unless it is linked with the same shared library as the executable.

@vt-alt
Copy link
Member

vt-alt commented Jan 12, 2022

I'd consider this option as a separate build variant.

Agree. I discussed this slightly with @ldv-alt and he suggests that this should be build time option — as ALT certainly will not use this feature.

@yanovich
Copy link
Author

Agree. I discussed this slightly with @ldv-alt and he suggests that this should be build time option — as ALT certainly will not use this feature.

I don't understand how linking against libcrypto makes the system any safer. If the caller uses a different version of libcrypto than the engine, it will segfault. And segfaults are bad for security.

The plugins are quite common. Kernel modules are plugins in the same sense as gost.so. The difference is that linux is specifically states that its ABI is a private moving target, so we rebuild all kernel modules when we update a single line of kernel code. But this is not the case with libcrypto. Its ABI is public and versioned. If openssl breaks libcrypto ABI, tons of packages will break.

Finally, all of this is the least concern possible for a distribution like ALT, which completely controls all the files it ships.

Dynamic linking is a mess for sure. That is why everyone tries to link statically: go (google has this policy in general), node.js, snap, flatpack. But there is no way to statically link gost.so. This makes the issue complicated. openssl itself links in-tree engines against libcrypto, which makes then difficult to use in statically linked executables.

My patch offers part of a solution, if libcrypto ABI can be frozen so that no interface change are allowed (only addition or removal is possible), then the solution for linux will be complete. I am not sure whether it is possible to solve this issue for other platforms at all.

@vt-alt
Copy link
Member

vt-alt commented Jan 13, 2022

I don't understand how linking against libcrypto makes the system any safer. If the caller uses a different version of libcrypto than the engine, it will segfault. And segfaults are bad for security.

@ldv-alt thinks this is better, because this will create strict ABI dependence for packages. Dynamic linker when loading gost.so will load libcrypto and check presence of all needed symbols. (This is different use case than we have for engine crash). Also, ALT have set-versions feature which automatically set dependence between packages with libraries and their users, which will not work if we disable -lcrypto.

@vt-alt
Copy link
Member

vt-alt commented Jan 13, 2022

If the caller uses a different version of libcrypto than the engine, it will segfault. And segfaults are bad for security.

Btw, this only happen to me when I play with manually built openssl OR engine and never when I use packages. And @ldv-alt's concern is packages.

If any executable loads `gost.so`, the executable already either has
`libcrypto.so` loaded or is statically linked against `libcrypto.a`.
Anyway it already has all libcrypto (and libssl) symbols present.

Without this patch `gost.so` is linked against `libcrypto,so`. As
a result, a diamond dependency is introduced. If `gost.so` is then
loaded by an executable which is statically linked against libcrypto,
`ld` will insist on loading `libcripto.so`, despite the executable
already having all necessary symbols. When the executable is statically
linked, shared objects for libcrypto and libssl are usually not built,
`ls` won't find them, and the caller will crush.

The patch removes this unnecessary link dependency in `gost.so`,
allowing it to be used by executables which are statically linked
against libcrypto.
gost_gost2015.c Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants