help on TSIGs
Stefan (metze) Metzmacher
metze at samba.org
Sun Nov 16 01:24:52 MST 2014
Hi Amitay,
Your patch would remove the valgrind problem but seems to be the wrong
approach.
I think the attached patches would be the correct fix
as we need to allocate all members of struct dns_request_state
on the correct talloc parent.
Can someone review and push these pathces?
metze
> On Sun, Oct 12, 2014 at 1:17 PM, Matthieu Patou <mat at samba.org> wrote:
>
>> On 10/10/2014 08:20 AM, Simo wrote:
>>
>>> On Wed, 2014-10-08 at 19:00 -0700, Matthieu Patou wrote:
>>>
>>>> - if (state->state.sign) { - ret = dns_sign_tsig(state->dns, mem_ctx,
>>>> &state->state, + if (state->state->sign) { + ret =
>>>> dns_sign_tsig(state->dns, mem_ctx, state->state, &state->out_packet,
>>>> 0);
>>>>
>>> Looks to me a simpler fix would be to pass here 'state' instead of
>>> mem_ctx to dns_sign_tsig()
>>>
>> I think it wouldn't be sufficient, you will need to change the mem_ctx of
>> handle_tkey as well.
>> Also despite all the variables being called 'state' they have different
>> type, in the function dns_process_recv (the function that call
>> dns_sign_tsig) state is a struct dns_process_state and in
>> dns_server_process_query_send (calling handle_tkey, where the problem was
>> reported by address sanitizer) it's a struct dns_server_process_query_
>> state.
>> It might work but I'm not sure (I haven't checked the life period of
>> dns_server_process_query_state).
>>
>> Then I think it's a bad practice to have sub-objects allocated to an
>> unrelated context, because one day or another it will bite you because of
>> the different lifetime between the object and it's sub-objects.
>>
>> Last but not least, I don't think it should have an impact on the TSIG
>> stuff, and most probably I'll still have the errors message in nsupdate.
>>
>>
> I was able to reproduce this issue even with single NIC.
>
> There are two issues:
>
> 1. The use-heap-after-free error.
>
> A simpler patch is to just fix the memory context for req_state->key_name
> (attached).
>
> 2. tsig verify error
>
> Apparently this is a known issue (checked with Andrew Bartlett). The
> additional debug information from nsupdate shows that the error is coming
> from gssapi library.
>
> GSS verify error: GSSAPI error: Major = A token had an invalid Message
> Integrity Check (MIC), Minor = Success.
> tsig key '3061967696.sig-samba-i1.lindom.example.local' (<null>):
> signature failed to verify(1)
> ; TSIG error with server: tsig verify failure
>
>
> Amitay.
>
-------------- next part --------------
From a9c69020dd8c0d59c2ed3d9f515bb82266e50871 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 14 Oct 2014 09:30:43 +0200
Subject: [PATCH 1/2] s4:dns_server: add some const to
dns_server_process_update/dns_update_allowed arguments
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source4/dns_server/dns_server.h | 4 ++--
source4/dns_server/dns_update.c | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/source4/dns_server/dns_server.h b/source4/dns_server/dns_server.h
index 12ccc9b..19fd7a9 100644
--- a/source4/dns_server/dns_server.h
+++ b/source4/dns_server/dns_server.h
@@ -79,9 +79,9 @@ WERROR dns_server_process_query_recv(
struct dns_res_rec **additional, uint16_t *arcount);
WERROR dns_server_process_update(struct dns_server *dns,
- struct dns_request_state *state,
+ const struct dns_request_state *state,
TALLOC_CTX *mem_ctx,
- struct dns_name_packet *in,
+ const struct dns_name_packet *in,
struct dns_res_rec **prereqs, uint16_t *prereq_count,
struct dns_res_rec **updates, uint16_t *update_count,
struct dns_res_rec **additional, uint16_t *arcount);
diff --git a/source4/dns_server/dns_update.c b/source4/dns_server/dns_update.c
index 04e7d9a..4be91c5 100644
--- a/source4/dns_server/dns_update.c
+++ b/source4/dns_server/dns_update.c
@@ -732,7 +732,7 @@ failed:
}
static WERROR dns_update_allowed(struct dns_server *dns,
- struct dns_request_state *state,
+ const struct dns_request_state *state,
struct dns_server_tkey **tkey)
{
if (lpcfg_allow_dns_updates(dns->task->lp_ctx) == DNS_UPDATE_ON) {
@@ -761,9 +761,9 @@ static WERROR dns_update_allowed(struct dns_server *dns,
WERROR dns_server_process_update(struct dns_server *dns,
- struct dns_request_state *state,
+ const struct dns_request_state *state,
TALLOC_CTX *mem_ctx,
- struct dns_name_packet *in,
+ const struct dns_name_packet *in,
struct dns_res_rec **prereqs, uint16_t *prereq_count,
struct dns_res_rec **updates, uint16_t *update_count,
struct dns_res_rec **additional, uint16_t *arcount)
--
1.9.1
From 7d436170f4d1833c201d584661ad23f569ac49c1 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 14 Oct 2014 09:34:29 +0200
Subject: [PATCH 2/2] s4:dns_server: allocate substructures of struct
dns_request_state on the correct TALLOC_CTX
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source4/dns_server/dns_crypto.c | 4 ++--
source4/dns_server/dns_query.c | 2 +-
source4/dns_server/dns_server.c | 1 +
source4/dns_server/dns_server.h | 1 +
4 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/source4/dns_server/dns_crypto.c b/source4/dns_server/dns_crypto.c
index aba906d..3f199de 100644
--- a/source4/dns_server/dns_crypto.c
+++ b/source4/dns_server/dns_crypto.c
@@ -131,7 +131,7 @@ WERROR dns_verify_tsig(struct dns_server *dns,
/* We got a TSIG, so we need to sign our reply */
state->sign = true;
- state->tsig = talloc_zero(mem_ctx, struct dns_res_rec);
+ state->tsig = talloc_zero(state->mem_ctx, struct dns_res_rec);
if (state->tsig == NULL) {
return WERR_NOMEM;
}
@@ -226,7 +226,7 @@ WERROR dns_verify_tsig(struct dns_server *dns,
}
state->authenticated = true;
- state->key_name = talloc_strdup(mem_ctx, tkey->name);
+ state->key_name = talloc_strdup(state->mem_ctx, tkey->name);
if (state->key_name == NULL) {
return WERR_NOMEM;
}
diff --git a/source4/dns_server/dns_query.c b/source4/dns_server/dns_query.c
index b57cdb8..e30e369 100644
--- a/source4/dns_server/dns_query.c
+++ b/source4/dns_server/dns_query.c
@@ -529,7 +529,7 @@ static WERROR handle_tkey(struct dns_server *dns,
reply.data,
reply.length);
state->sign = true;
- state->key_name = talloc_strdup(mem_ctx, tkey->name);
+ state->key_name = talloc_strdup(state->mem_ctx, tkey->name);
if (state->key_name == NULL) {
return WERR_NOMEM;
}
diff --git a/source4/dns_server/dns_server.c b/source4/dns_server/dns_server.c
index 7ea70db..f1a4c4c 100644
--- a/source4/dns_server/dns_server.c
+++ b/source4/dns_server/dns_server.c
@@ -125,6 +125,7 @@ static struct tevent_req *dns_process_send(TALLOC_CTX *mem_ctx,
if (req == NULL) {
return NULL;
}
+ state->state.mem_ctx = state;
state->in = in;
state->dns = dns;
diff --git a/source4/dns_server/dns_server.h b/source4/dns_server/dns_server.h
index 19fd7a9..3423ee0 100644
--- a/source4/dns_server/dns_server.h
+++ b/source4/dns_server/dns_server.h
@@ -60,6 +60,7 @@ struct dns_server {
};
struct dns_request_state {
+ TALLOC_CTX *mem_ctx;
uint16_t flags;
bool authenticated;
bool sign;
--
1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20141116/1e2ecf6a/attachment.pgp>
More information about the samba-technical
mailing list