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

Forward integrity and confidentiality of logs #3121

Open
wants to merge 56 commits into
base: master
from
Open

Conversation

@nbsd
Copy link

nbsd commented Feb 7, 2020

Logs processed by syslog-ng are neither confidential nor integrity protected. The use of these logs for the purpose of security monitoring and forensic analysis in case of an incident is rather limited, as they may have been tampered with by an attacker trying to hide his trace.

The implementation providing forward integrity and confidentiality of logs with syslog-ng that was presented recently at the FOSDEM 2020 is contained in this pull request. It provides users of syslog-ng with forward integrity and confidentiality of their logs in case this is needed or required.

This pull request solves issue #3118.

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

2 similar comments
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

@lbudai

This comment has been minimized.

Copy link
Contributor

lbudai commented Feb 7, 2020

@kira-syslogng: ok to test

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

Build FAILURE

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

Build FAILURE

6 similar comments
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

Build FAILURE

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

Build FAILURE

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

Build FAILURE

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

Build FAILURE

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

Build FAILURE

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 7, 2020

Build FAILURE

@MrAnno

This comment has been minimized.

Copy link
Member

MrAnno commented Feb 7, 2020

Sorry for the over-complicated copyright check.
With your current license of choice, the policy file should look like this:

  LGPLv2.1+_SSL,non-balabit
 lib/timeutils/zonecache.[ch]$
+lib/slog\.[ch]$
+modules/cryptofuncs/slogimport/slogimport\.c$
+modules/cryptofuncs/slogkey/slogkey\.c$
+modules/cryptofuncs/slogverify/slogverify\.c$
  LGPLv2.1+_SSL
 autogen\.sh$
 sub-configure\.sh$
@@ -74,10 +78,7 @@ modules/timestamp/rewrite-.*$
 modules/add-contextual-data/add-contextual-data-glob-selector\.[ch]$
 modules/add-contextual-data/tests/test_glob_selector\.c$
 modules/affile/tests/test_file_writer\.c$
-lib/slog\.[ch]$
-modules/cryptofuncs/slogkey/slogimport\.c$
-modules/cryptofuncs/slogkey/slogkey\.c$
-modules/cryptofuncs/slogkey/slogverify\.c$

Note that I've changed directory names as well, not only the location of those lines.

MrAnno and others added 14 commits Feb 3, 2020
Headers should not define variables. Including them into different
compilation units results in multiple definitions of the same name.

Detected with the -fno-common gcc flag.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
The message parsing test library is not used in the proto library,
it was included only to load the syslogformat module.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
Instead of using a global variable, init_and_load_syslogformat_module()
now receives a parse_options argument, which will be initialized after
loading the syslogformat module.

Removing this global requires all tests to declare their own instance,
but even if it's redundant, it is cleaner. Those tests reference
parse_options in several places, but the declaration/definition was hidden
in msg-parse-lib.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
The -fcommon option hides (merges) multiple definition issues such as the
persist-tool global variable definition in the previous commit.
Such multi-definitions are not intentional in the syslog-ng codebase.

"The -fcommon places uninitialized global variables in a common block.
This allows the linker to resolve all tentative definitions of the same
variable in different compilation units to the same object."

In new GCC versions, -fno-common is the default.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
Introduction of the secure logging core module

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
Function tf_slog_prepare configures the secure logging template and
function tf_slog implements it

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
This utility imports logs using the secure logging template

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
This utility manages the keys use by the secure loggin template

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
This utility is used to verify the integrity of logs files that have been created using the secure logging template

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
Manual pages for the slogimport, slogkey and slogverify utilities

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
Extension of the Makefile for the manual pages

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
Building the secure logging template in CentOS 7, CentOS 8 and Fedora 31 needs a more fine-grained control over the build options

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
@nbsd nbsd force-pushed the nbsd:master branch from e8189cc to 73faa18 Feb 9, 2020
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 9, 2020

Build FAILURE

@lbudai

This comment has been minimized.

Copy link
Contributor

lbudai commented Feb 14, 2020

@nbsd: it's nice to see that your PR passed on CI

Could you move all the slog related sources to a separate module?
(It could speed up the merge process as it can be handled as a self-contained experimental module).

The secure logging functionality providing forward integrity and confidentiality for logs
is no longer part of the standard syslog-ng module cryptofuncs. It has been moved to its
own modules called secure-logging in order to allow for easier test and integration.

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
@nbsd

This comment has been minimized.

Copy link
Author

nbsd commented Feb 14, 2020

The secure logging functionality has been moved to a separate module called "secure-logging".

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 14, 2020

Build FAILURE

@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Feb 14, 2020

This pull request introduces 3 alerts when merging 4330105 into adfca59 - view on LGTM.com

new alerts:

  • 3 for Implicit function declaration
Airbus Commercial Aircraft
The test suite name must not contain a hyphen in both the source and the Makefiles.

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 14, 2020

Build FAILURE

@Kokan

This comment has been minimized.

Copy link
Contributor

Kokan commented Feb 17, 2020

@nbsd could you please also move the slog.h/slog.c file from lib to the new module ?

@nbsd

This comment has been minimized.

Copy link
Author

nbsd commented Feb 17, 2020

This would break the secure logging command line utilities for key management and log verification. I don't see where I would put the library functions contained in slog.[ch] such that both the module and the command line utilities will build without problems and are able to resolve the library functions at runtime.

@Kokan

This comment has been minimized.

Copy link
Contributor

Kokan commented Feb 18, 2020

@nbsd If you move every functionality (slog.[c]h and $(slog)) into the current secure-logging module (libsecure-logging.so) in that case:

  • your module has everything it needs
  • the command line tools could link to the module and obtain the needed functionality

Note1: still both of those also have to link to syslog-ng.
Note2: A set of patch just doing the above with cmake: https://gist.github.com/Kokan/a4ac2103d156d44a3ea7e866b08a4474

@nbsd

This comment has been minimized.

Copy link
Author

nbsd commented Feb 18, 2020

Thanks for providing the patches. However, trying to apply them results in the following errors:

user@host~> git apply --check 0001-secure-logging-fix-cmake-compile.patch
error: git diff header lacks filename information (line 40)
user@host~> git apply --check 0002-secure-logging-move-slog-under-modules.patch
error: corrupt patch at line 89

I have tried the patches against the current version of the forked repository.

@Kokan

This comment has been minimized.

Copy link
Contributor

Kokan commented Feb 18, 2020

Not sure if git could help you, but with patch they should apply: patch -p1 < [file] on top of your current branch.

@nbsd

This comment has been minimized.

Copy link
Author

nbsd commented Feb 18, 2020

After applying the patches and also changing the Makefile.am within the module, I get the following warning:

*** Warning: Linking the executable modules/secure-logging/slogverify/slogverify against the loadable module
*** libsecure-logging.so is not portable!

However, the syslog-ng package builds and installs correctly and the secure logging commandline tools do work.

Airbus Commercial Aircraft added 2 commits Feb 18, 2020
The core functionality for secuure logging is now part of the corresponding module,
such that the code does not depend on any other part of syslog-ng. The commandline
utilities now have to link against the module library in addition to the core
syslog-ng library.

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
…g-ng

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 18, 2020

Build FAILURE

Airbus Commercial Aircraft
Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 18, 2020

Build FAILURE

Airbus Commercial Aircraft
Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 18, 2020

Build FAILURE

Airbus Commercial Aircraft
Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 18, 2020

Build FAILURE

Declarations changed in order to avoid unused variables.

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 18, 2020

Build SUCCESS

@nbsd

This comment has been minimized.

Copy link
Author

nbsd commented Feb 18, 2020

The package including the secure logging functionality builds correctly with both gcc and clang after having created an extra module for it. However, make distcheck fails because of the slog.h not being found. This is not the case when building the package on various test machines. Building with cmake works without problems. Please explain the difference between the build environments that may cause this problem.


modules_secure_logging_libsecure_logging_la_SOURCES = \
modules/secure-logging/secure-logging.c \
modules/secure-logging/slog.c

This comment has been minimized.

Copy link
@Kokan

Kokan Feb 19, 2020

Contributor

You have to include the header files as well here, so the automake/autotools could track those files and include into the source tarball (this is the reason why the first job fails).

@Kokan

This comment has been minimized.

Copy link
Contributor

Kokan commented Feb 19, 2020

@nbsd
Regarding the *** Warning: Linking the executable modules/secure-logging/slogverify/slogverify against the loadable module, this is not my realm, hope somebody could give you proper guide on that. I only know that in case of such case (dqtool, pdbtool an intermediate library is provided, and this warning is not produced).

I only could read that this might cause issue on Mac OS (but the build seems fine on that).

The B=distcheck build aim is to check if the generated disk tarball is sound and a build can be made from it. First it creates a tarball, and builds syslog-ng from that.

Airbus Commercial Aircraft
The header file is included in the list of sources as it is required when creating a tarball.

Signed-off-by: Airbus Commercial Aircraft <secure-logging@airbus.com>
@nbsd

This comment has been minimized.

Copy link
Author

nbsd commented Feb 19, 2020

Thanks for the hint. The modules/secure-logging/Makefile.am was changed accordingly. Concerning the warnings when build the library, I will do some tests on my Mac and try to identifiy potential issues on the MacOS platform.

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

kira-syslogng commented Feb 19, 2020

Build SUCCESS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.