<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [ ] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [ ] 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 - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [ ] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail -->
Various fixes for issues discovered with the Coverity static analysis tooling.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/4030
-- Commit Summary --
* cdp: fixes for various issues found with coverity
-- File Changes --
M src/modules/cdp/authstatemachine.c (2) M src/modules/cdp/common.c (8) M src/modules/cdp/diameter_peer.c (6) M src/modules/cdp/receiver.c (4) M src/modules/cdp/session.c (12) M src/modules/cdp/session.h (13)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/4030.patch https://github.com/kamailio/kamailio/pull/4030.diff
@henningw commented on this pull request.
@@ -236,7 +236,7 @@ int diameter_peer_start(int blocking)
int seed; peer *p;
- seed = random(); + seed = kam_rand();
It looks like this seed is later used to seed the glibc random number generator for (sub-) processes? If yes, then you probably want to use cryptorand() for it. This is an internal kamailio function that returns a cryptographically secure random number. The kam_rand() is just a define for rand() from the glibc, which is quite weak security wise.
This applies also to the other places below.
Please also have a look if you are not seeding the main process again, as this is already done in main.c and core/pt.c
@vingarzan commented on this pull request.
@@ -236,7 +236,7 @@ int diameter_peer_start(int blocking)
int seed; peer *p;
- seed = random(); + seed = kam_rand();
Do we need though cryptographically secure seeding of the random though? I looked now in core/pt.c and there seem to be 3 things seeded there... I'd say that, if all 3 should be seeded after forking, then we should extract that code as a function, maybe even have a Kamailio-fork-function to do all of these. Anyway, this PR only tries to fight the immediate fires, not do such deeper changes.
It looks like the old code here is only seeding the child processes.
@henningw commented on this pull request.
@@ -236,7 +236,7 @@ int diameter_peer_start(int blocking)
int seed; peer *p;
- seed = random(); + seed = kam_rand();
It depends what the child processes actually use then in their code. The code in pt.c seeds all three as the different modules uses a lot of different functions. This is usually not needed in individual module new processes. My other point was that the change here does not change anything regarding the functionality, besides silence a warning. Of course its still better to use the kam_rand() as a policy, to allow easier auditing or later changes.
``` core/rand/kam_rand.h 31:#define kam_rand(x) rand(x) ```
@vingarzan commented on this pull request.
@@ -236,7 +236,7 @@ int diameter_peer_start(int blocking)
int seed; peer *p;
- seed = random(); + seed = kam_rand();
Right, that function is actually named `fork_process()`, exactly what I needed :rofl:
@lbalaceanu commented on this pull request.
@@ -830,7 +830,7 @@ int receive_loop(peer *original_peer)
#if OPENSSL_VERSION_NUMBER >= 0x10100000L if(enable_tls) { to_ssl(&sp2->tls_ctx, &sp2->tls_conn, - sp->tcp_socket, method); + sp2->tcp_socket, method);
You are right, this seems like a bug.
@lbalaceanu commented on this pull request.
@@ -843,7 +843,7 @@ int receive_loop(peer *original_peer)
#if OPENSSL_VERSION_NUMBER >= 0x10100000L if(enable_tls) { to_ssl(&sp2->tls_ctx, &sp2->tls_conn, - sp->tcp_socket, method); + sp2->tcp_socket, method);
This also looks like a typo.
@vingarzan commented on this pull request.
@@ -236,7 +236,7 @@ int diameter_peer_start(int blocking)
int seed; peer *p;
- seed = random(); + seed = kam_rand();
Seems like that function is already called, so I can probably just remove the seeding from here, since it was already seeded (by calling `fork_process()`).
@henningw commented on this pull request.
@@ -236,7 +236,7 @@ int diameter_peer_start(int blocking)
int seed; peer *p;
- seed = random(); + seed = kam_rand();
Great, if you are using fork_process from the core this is indeed not needed and it should be removed.
@vingarzan pushed 1 commit.
09faa4ce88cd1ecbf544017f2f0e986c2843be48 cdp: fixed issues discovered during static code analysis
@vingarzan commented on this pull request.
@@ -830,7 +830,7 @@ int receive_loop(peer *original_peer)
#if OPENSSL_VERSION_NUMBER >= 0x10100000L if(enable_tls) { to_ssl(&sp2->tls_ctx, &sp2->tls_conn, - sp->tcp_socket, method); + sp2->tcp_socket, method);
Thanks for confirming!
Does this PR still need to be reviewed or is ready to merge?
Does this PR still need to be reviewed or is ready to merge?
I have a small concern about session.c:485 if it should be `time_t`... but then that would go a bit deeper, yet I haven't seen issues with that. Hope to check and fix tomorrow.
@vingarzan pushed 1 commit.
1b9f124ba2514b822554ad90a2d359af884b4bb5 cdp: fixed issues discovered during static code analysis
Now it should be fine, so if all checks pass, it can be merged.
Merged #4030 into master.
-
Daniel-Constantin Mierla -
Dragos Vingarzan
-
Henning Westerholt
-
Lucian Balaceanu