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

dird: fix tls reload crash #1249

Merged
merged 4 commits into from Oct 7, 2022
Merged

dird: fix tls reload crash #1249

merged 4 commits into from Oct 7, 2022

Conversation

pstorz
Copy link
Member

@pstorz pstorz commented Sep 6, 2022

The director reload logic was not aware of the fact that the tls-psk mechanism's callback also accesses the configuration resources.

The tls_openssl_private objects now also have a shared_ptr to the configuration resources to make sure that these are not freed between the tls-psk callback is setup and the callback is really called.

The following patch can be used to trigger the problem when built with sanitizers:

diff --git a/core/src/lib/tls_openssl_private.cc b/core/src/lib/tls_openssl_private.cc
index 3b47510a2..ece45fa6e 100644
--- a/core/src/lib/tls_openssl_private.cc
+++ b/core/src/lib/tls_openssl_private.cc
@@ -468,7 +468,7 @@ unsigned int TlsOpenSslPrivate::psk_server_cb(SSL* ssl,
   Dmsg1(100, "psk_server_cb. identitiy: %s.\n", lst.JoinReadable().c_str());
 
   std::string configured_psk;
-
+  Bmicrosleep(5, 0);
   ConfigurationParser* config
       = static_cast<ConfigurationParser*>(SSL_CTX_get_ex_data(
           openssl_ctx,

  • Reload the config every second e.g. with
    kill -HUP $(pidof bareos_dir-reload)
  • start bconsole

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General
  • PR name is meaningful
  • Purpose of the PR is understood
  • Separate commit for this PR in the CHANGELOG.md, PR number referenced is same
  • Commit descriptions are understandable and well formatted
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR
  • bareos-check-sources --since-merge does not report any problems
  • git status should not report modifications in the source tree after building and testing

@pstorz pstorz force-pushed the dev/pstorz/fix-tls-reload-crash branch 3 times, most recently from ca26496 to c209bd0 Compare September 7, 2022 09:27
@pstorz pstorz self-assigned this Sep 7, 2022
@pstorz pstorz force-pushed the dev/pstorz/fix-tls-reload-crash branch from c209bd0 to 2a89cec Compare September 8, 2022 08:17
@pstorz pstorz requested review from arogge and removed request for alaaeddineelamri September 12, 2022 08:24
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, I just have some stylistic comments.

core/src/lib/tls_openssl.cc Outdated Show resolved Hide resolved
core/src/lib/tls_openssl.cc Outdated Show resolved Hide resolved
core/src/lib/parse_conf.cc Outdated Show resolved Hide resolved
core/src/lib/tls_openssl_private.cc Outdated Show resolved Hide resolved
@pstorz pstorz requested a review from arogge September 12, 2022 11:12
@pstorz pstorz force-pushed the dev/pstorz/fix-tls-reload-crash branch 3 times, most recently from 816ddfc to 4cde69a Compare September 19, 2022 10:19
@pstorz pstorz mentioned this pull request Sep 19, 2022
13 tasks
@pstorz pstorz requested review from arogge and removed request for arogge September 19, 2022 10:33
@pstorz pstorz force-pushed the dev/pstorz/fix-tls-reload-crash branch 2 times, most recently from f51ca98 to b45cd76 Compare September 30, 2022 14:53
pstorz and others added 4 commits October 6, 2022 12:55
This avoids freeing the resource table while the tls psk callback still
needs to access it.
previously my_config was destroyed before the backups of the resource
table. As the dtor if the resource tables needs the configparser this
lead to a use-after-free.
the recently added functions BackupResourceTable(),
RestoreResourceTable() and GetResourceTable() turned out to be badly
named as they handle a shared_ptr<ConfigResourcesContainer> and there
already is a type named ResourceTable.
This commit renames the functions to BackupResourcesContainer(),
RestoreResourcesContainer() and GetResoucesContainer().
@arogge arogge force-pushed the dev/pstorz/fix-tls-reload-crash branch from b45cd76 to e6a180c Compare October 7, 2022 08:59
@arogge arogge merged commit cb4a04b into master Oct 7, 2022
@arogge arogge deleted the dev/pstorz/fix-tls-reload-crash branch October 7, 2022 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants