So the problem is due to label being not null terminated but our code doing the assumption that it is.

Created attachment 8060 [details]
Proposed patch to fix the problem

We force buffer to be always initialized with 0, in order to avoid garbage at the end of a label.

Comment on attachment 8060 [details]
Proposed patch to fix the problem


>It avoid bugs when one of the buffer is supposed to contain a string
>that is not null terminated (ie. label->label) and that we don't force
>the last byte to 0.

I think the correct fix is to force the last byte to 0. Unless there's some sort of padding after those invalid strings, we'll leak into other data in any case.

(In reply to comment #3)
> Comment on attachment 8060 [details]
> Proposed patch to fix the problem
> 
> 
> >It avoid bugs when one of the buffer is supposed to contain a string
> >that is not null terminated (ie. label->label) and that we don't force
> >the last byte to 0.
> 
> I think the correct fix is to force the last byte to 0. Unless there's some
> sort of padding after those invalid strings, we'll leak into other data in any
> case.

that was my initial solution but metze convinced me that should be more prudent with this code that not everybody understand and initializing buffer to 0 would avoid copying garbage.
I think this makes sense, if really needed I can also force the last byte to 0 to clearly indicate that we know that the string should be finished with a '\0'

All I'm saying is that you say the problem is that we're not terminating strings correctly, but you're not fixing that problem.

It is fixing
+	if (!(label->label = talloc_zero_array(label, char, len+1))) {
 		buf->error = ERROR_DNS_NO_MEMORY;
 		goto error;
 	}
Here is the buffer for the label, we allocate len + 1 bytes, len is obtained from the response (not by doing a strlen on label). so we are sure that we will copy at most len bytes from label and so the last byte will 0.

* It is good to use the talloc_zero* calls in all cases anyways.
* For the label case, this in particular fixes the non-zero-terminated
  string case as Matthieu explained.
* The other cases changed in this patch, don't seem to contain strings,
  so I don't see a problem with this patch.

So this fixes what is described in comment #1:
It ensures that the label string is always zero-terminated.

Comment on attachment 8060 [details]
Proposed patch to fix the problem

ACK from me, based on the previous comment. What do we do with this bug now?

Well this is maybe one of the most important bug because it can impact a lot of users.
I'm a bit enclined to force the pickup for rc3.

Karolin, what's your point of view

Comment on attachment 8060 [details]
Proposed patch to fix the problem

This fixes the bug
and given the complexity of this code it's better to always use talloc*zero*
instead of just fixing single byte.

Metze, should we pick it for rc3?

Created attachment 8073 [details]
Patch for v3-6-test

Created attachment 8074 [details]
Patch for v3-5-test/v3-4-test

Karolin, please pick this to v4-0-test.

Pushed to autobuild-v4-0-test.
Re-assigning to Michael to review the other patches.

assigning to Karolin for picking to 3.X versions.

Pushed to v3-6-test and v3-5-test.
Closing out bug report.

Thanks!