[PATCH] Do not use un-initialised variables in audit_logging
Andrew Bartlett
abartlet at samba.org
Mon Jun 25 03:09:20 UTC 2018
G'Day Gary,
I was chasing an unrelated issue with valgrind and noticed use of an
uninitialised value here:
lib/audit_logging/audit_logging.c: audit_message_send()
struct server_id event_server
==9198== Conditional jump or move depends on uninitialised value(s)
==9198== at 0xD62CE58: imessaging_send (messaging_send.c:68)
==9198== by 0xBAC3D6F: audit_message_send (audit_logging.c:262)
==9198== by 0xBAC243D: log_json (auth_log.c:87)
==9198== by 0xBAC27B5: log_authentication_event_json
(auth_log.c:200)
==9198== by 0xBAC318B: log_authentication_event (auth_log.c:520)
==9198== by 0x1BC9AA30: hdb_samba4_auth_status (hdb-samba4.c:496)
==9198== by 0x1BA6E076: _kdc_as_rep (kerberos5.c:1390)
==9198== by 0x1BA7B131: kdc_as_req (process.c:70)
==9198== by 0x1BA7B397: krb5_kdc_process_krb5_request
(process.c:242)
==9198== by 0x1B85AE27: kdc_process (kdc-heimdal.c:84)
==9198== by 0x1B85440C: kdc_udp_call_loop (kdc-server.c:137)
==9198== by 0x69AAFAF: _tevent_req_notify_callback
(tevent_req.c:125)
==9198==
The attached patches fix up what looks like a bad migration from the
auth code to the common logging code. We don't want to ignore the
NO_SUCH_OBJECT errors.
CI: https://gitlab.com/catalyst-samba/samba/pipelines/24472905
Please review and push if CI indicates!
Thanks,
Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
-------------- next part --------------
From 880cf2037798e7d0645044fd5ef7cf93459695d7 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 25 Jun 2018 14:48:27 +1200
Subject: [PATCH 1/4] audit_logging: Clarify debug messages
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
lib/audit_logging/audit_logging.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/audit_logging/audit_logging.c b/lib/audit_logging/audit_logging.c
index 13ff3453dcc..2335fb14581 100644
--- a/lib/audit_logging/audit_logging.c
+++ b/lib/audit_logging/audit_logging.c
@@ -173,7 +173,7 @@ static NTSTATUS get_event_server(
if (!NT_STATUS_IS_OK(status)) {
DBG_NOTICE(
"Failed to find '%s' registered on the message bus to "
- "send audit events to: %s\n",
+ "send JSON audit events to: %s\n",
server_name,
nt_errstr(status));
TALLOC_FREE(frame);
@@ -199,7 +199,7 @@ static NTSTATUS get_event_server(
}
DBG_NOTICE(
"Failed to find '%s' registered on the message bus to "
- "send audit events to: %s\n",
+ "send JSON audit events to: %s\n",
server_name,
nt_errstr(status));
TALLOC_FREE(frame);
--
2.11.0
From 048e42243c335af46405eeafb2211ce3d78bc00b Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 25 Jun 2018 14:51:35 +1200
Subject: [PATCH 2/4] audit_logging: Remove incorrect check for
NT_STATUS_OBJECT_NAME_NOT_FOUND
NT_STATUS_OBJECT_NAME_NOT_FOUND is not a case we can ignore, it would mean that event_server
is not initialised.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
lib/audit_logging/audit_logging.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/audit_logging/audit_logging.c b/lib/audit_logging/audit_logging.c
index 2335fb14581..3a27eb6105f 100644
--- a/lib/audit_logging/audit_logging.c
+++ b/lib/audit_logging/audit_logging.c
@@ -248,8 +248,7 @@ void audit_message_send(
* messages may get lost
*/
status = get_event_server(msg_ctx, server_name, &event_server);
- if (!NT_STATUS_IS_OK(status) &&
- !NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
+ if (!NT_STATUS_IS_OK(status)) {
DBG_ERR("get_event_server for %s returned (%s)\n",
server_name,
nt_errstr(status));
@@ -270,8 +269,7 @@ void audit_message_send(
*/
if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
status = get_event_server(msg_ctx, server_name, &event_server);
- if (!NT_STATUS_IS_OK(status) &&
- !NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
+ if (!NT_STATUS_IS_OK(status)) {
DBG_ERR("get_event_server for %s returned (%s)\n",
server_name,
nt_errstr(status));
--
2.11.0
From e5884ae6a76b7d8776db6db09cd143b8d43de49f Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 25 Jun 2018 14:52:19 +1200
Subject: [PATCH 3/4] audit_logging: Initialise event_server
It is better if this is a known zero value to start, even if we check the errors
correctly.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
lib/audit_logging/audit_logging.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/audit_logging/audit_logging.c b/lib/audit_logging/audit_logging.c
index 3a27eb6105f..dda350cd144 100644
--- a/lib/audit_logging/audit_logging.c
+++ b/lib/audit_logging/audit_logging.c
@@ -230,7 +230,7 @@ void audit_message_send(
uint32_t message_type,
struct json_object *message)
{
- struct server_id event_server;
+ struct server_id event_server = {};
NTSTATUS status;
const char *message_string = NULL;
--
2.11.0
From 0e1d7bcd023afc09eb2acef79a8b5d0d6e3a91ac Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 25 Jun 2018 14:52:59 +1200
Subject: [PATCH 4/4] audit_logging: Remove duplciate error printing
These errors are already logged at DBG_NOTICE in get_event_server()
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
lib/audit_logging/audit_logging.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/lib/audit_logging/audit_logging.c b/lib/audit_logging/audit_logging.c
index dda350cd144..f94f2c2a839 100644
--- a/lib/audit_logging/audit_logging.c
+++ b/lib/audit_logging/audit_logging.c
@@ -249,9 +249,6 @@ void audit_message_send(
*/
status = get_event_server(msg_ctx, server_name, &event_server);
if (!NT_STATUS_IS_OK(status)) {
- DBG_ERR("get_event_server for %s returned (%s)\n",
- server_name,
- nt_errstr(status));
TALLOC_FREE(ctx);
return;
}
@@ -270,9 +267,6 @@ void audit_message_send(
if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
status = get_event_server(msg_ctx, server_name, &event_server);
if (!NT_STATUS_IS_OK(status)) {
- DBG_ERR("get_event_server for %s returned (%s)\n",
- server_name,
- nt_errstr(status));
TALLOC_FREE(ctx);
return;
}
--
2.11.0
More information about the samba-technical
mailing list