#### Pre-Submission Checklist - [x] Commit message has the format required by CONTRIBUTING guide - [ ] Commits are split per component (core, individual modules, libs, utils, ...) - [ ] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: - [ ] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description Wireshark project allows embedding session encryption keys into PCAP files. So it allows parsing encrypted packets as it was unencrypted. [More info](https://blog.didierstevens.com/2021/01/11/decrypting-tls-streams-with-wiresh...) I prepared a change that allows export session encryption keys. Please review PR. If ok, then I add commits with DocBook info.
Kyys may be emeberd using command ``` editcap --inject-secrets tls,/var/lib/kamailio/session_keylog encrypted.pcap with_keys.pcapng ```
As prototype used https://github.com/openssl/openssl/blob/master/apps/lib/s_cb.c#L1480-L1525 You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2785
-- Commit Summary --
* tls: added new session_keylog_enable and session_keylog_filename configuration params * tls: first interation of session key logger * tls: added logs output
-- File Changes --
M src/modules/tls/tls_cfg.c (6) M src/modules/tls/tls_cfg.h (2) M src/modules/tls/tls_init.c (79) M src/modules/tls/tls_init.h (2) M src/modules/tls/tls_mod.c (2) M src/modules/tls/tls_rpc.c (2) M src/modules/tls/tls_server.c (1)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2785.patch https://github.com/kamailio/kamailio/pull/2785.diff
Before feature merged also wanted: 1. add ACL support for specified IP addresses (use case - server located behind HAproxy with enabled HTTPS health checks, i do not want to save session keys for these requests). 2. add ACL support to specific IP addresses (use case - I want session case only for request from the white list).
Maybe need some event route support to use ACL module.
Looks as there is only for inbound connections relayed session keys. Think need also to make the some for outbound connections.
Documentation is missing :-)
yes, I will add when we complete "c" part of PR.
@sergey-safarov pushed 1 commit.
368c13945d6d1d78541a72b29d260e50cd2fdeb4 tls: enabled session key loging for outbound connections
To get working session keys embedding and description required to record TLS connection from start (TCP SYN/ACK). If TLS connection is long-term and the start of the connection not recorded, then traffic cannot be decrypted.
From my point view, need to try utilize `SSL_key_update()` with `SSL_KEY_UPDATE_REQUESTED`. [More info](https://www.openssl.org/docs/man1.1.1/man3/SSL_renegotiate.html).
In my opinion there are issues of concurrency in writing to the file. It seems to be open in the main process, during the initialisation and then write to it during runtime from kamailio processes. Writing from multiple processes to the same file is going to have unpredictable content.
The solution would be to either have a lock around and do every time: lock, open file, write, close unlock; or create a consumer process that write to the file and the others produce data that is sent to it. Another option would be that each process writes to dedicated file (e.g., filename-pid).
I also noticed that the file name is reallocated in pkg using a local variable in a function and not freed.
I have checked openssl sources, loogs as `BIO_printf` and `BIO_write` function to not use lock mechanism.
The solution would be to either have a lock around and do every time: lock, open file, write, close, unlock;
Why we cannot open file once and use logic inside childs ``` lock, write, unlock ```
I also noticed that the file name is reallocated in pkg using a local variable in a function and not freed.
is memory allocated here and not freed? ```diff @@ -636,6 +710,7 @@ int tls_h_mod_pre_init_f(void) #endif SSL_load_error_strings(); tls_mod_preinitialized=1; + prepare_keylog_file(cfg_get(tls, tls_cfg, session_keylog_filename)); return 0; }
```
I have checked openssl sources, look as `BIO_printf` and `BIO_write` function to not use lock mechanism.
The solution would be to either have a lock around and do every time: lock, open file, write, close, unlock;
Why we cannot open the file once and use logic inside childrens
lock, write, unlock
You can read more on the net about opening a file before fork() and what happens after forking, etc ... In short, if you do not want to complicate the life a lot, it is not a good idea at all. Each process will have a clone of the file descriptor, with managing own states after fork ...
I also noticed that the file name is reallocated in pkg using a local variable in a function and not freed.
is memory allocated here and not freed?
@@ -636,6 +710,7 @@ int tls_h_mod_pre_init_f(void) #endif SSL_load_error_strings(); tls_mod_preinitialized=1; + prepare_keylog_file(cfg_get(tls, tls_cfg, session_keylog_filename)); return 0; }
I meant the `keylog_file` variable which is local to the function and then lost:
``` +int prepare_keylog_file(str session_keylog_filename) +{ + char *keylog_file = NULL; ```
I couldn't spot where is freed.
@sergey-safarov pushed 1 commit.
b6ac6e2b7297ee7830d96290014b36c0dadb7651 tls: keylog_file variable freed after use
To free memory applied patch ```diff --- a/src/modules/tls/tls_init.c +++ b/src/modules/tls/tls_init.c @@ -627,8 +627,9 @@ int prepare_keylog_file(str session_keylog_filename) * the tool is run multiple times. */ bio_keylog = BIO_new_file(keylog_file, "a"); + pkg_free(keylog_file); if (bio_keylog == NULL) { - LOG(tls_log, "Error writing keylog file: %s\n", keylog_file); + LOG(tls_log, "Error writing keylog file: %.*s\n", session_keylog_filename.len, session_keylog_filename.s); return 1; }
``` As session keys may be refreshed during TLS session, then the file open and close operation needs use on each `keylog_callback`. Will it be trick on a server with a lot of connected TLS clients?
As I was not able to implement the correct TLS key write implementation. I closing this PR.
Closed #2785.
No need to close it, I think it is pretty useful, unless you want to do something different. My comments were not to reject the comment, but present what needs to be made to work properly for writing concurrently in a file.
For example, you can write to syslog in the first version. As extra, you can use own facility and then divert to a special syslog file, so it is not combined with all the other logs. I think acc module has the same option for a log facility.
Later, you can eventually look at sipdump module, which collects data from kamailio processes and writes to disk from a dedicated process.
-
Daniel-Constantin Mierla -
Olle E. Johansson
-
sergey-safarov