Created attachment 7389 [details]
draft patches for v3-4-test

Created attachment 7390 [details]
draft patches for v3-5-test

Created attachment 7391 [details]
draft patches for v3-6-test

Created attachment 7392 [details]
draft patches for master

Created attachment 7393 [details]
draft patches for master with gen_ndr

Ouch. Been looking into back-porting this into 3.0.x (there are pidl generated files inside source/librpc/gen_ndr/) but there are no idl files or pidl source code in that tree.

Hmmmm. Does anyone remember how these files were generated for 3.0.x and from which IDL source ?

Jeremy.

(In reply to comment #6)
> Ouch. Been looking into back-porting this into 3.0.x (there are pidl generated
> files inside source/librpc/gen_ndr/) but there are no idl files or pidl source
> code in that tree.
> 
> Hmmmm. Does anyone remember how these files were generated for 3.0.x and from
> which IDL source ?
> 
> Jeremy.

Maybe just copied from the samba4 branch.

I'll upload patch for v3-3-test and v3-2-test soon...

Created attachment 7398 [details]
draft patches for v3-3-test

Created attachment 7399 [details]
draft patches for v3-2-test

Created attachment 7400 [details]
Patch for v3-0-test

Before we forget: we need to check the handmarshalling routines (librpc/ndr/ndr_X.c) also. These are typically created from autogenerated code and then modified afterwards, so there is a good chance we need to fix them as well.

Ok, I took a quick look inside librpc/ndr/ndr_X.c in master, and the only thing I saw using ndr_check_array() was in ndr_spoolss_buf.c.

Can someone else confirm that's all we have to worry about ?

Jeremy.

Ok, I think we have a problem in librpc/ndr/ndr_spoolss_buf.c where we allocate r->file_info based on ndr_get_array_size(ndr, &r->file_info) and then call ndr_check_array_size(ndr, (void*)&r->file_info, r->file_count));

Can someone else confirm this please ?

Jeremy.

From metze:

This from master looks correct:

#if 0
                        NDR_CHECK(ndr_pull_array_size(ndr, &r->file_info));
#else
                        NDR_CHECK(ndr_token_store(ndr,
&ndr->array_size_list, &r->file_info, r->file_count));
#endif
                        NDR_PULL_ALLOC_N(ndr, r->file_info,
ndr_get_array_size(ndr, &r->file_info));
                        _mem_save_file_info_1 = NDR_PULL_GET_MEM_CTX(ndr);
                        NDR_PULL_SET_MEM_CTX(ndr, r->file_info, 0);
                        for (cntr_file_info_1 = 0; cntr_file_info_1 <
r->file_count; cntr_file_info_1++) {

NDR_CHECK(ndr_pull_spoolss_DriverFileInfo(ndr, NDR_SCALARS,
&r->file_info[cntr_file_info_1]));
                        }

Because
NDR_CHECK(ndr_token_store(ndr, &ndr->array_size_list, &r->file_info,
r->file_count));
sets the value of ndr_get_array_size() to r->file_count.

metze

Ok, seems like the patchset is the final set of changes.

Off-list several people pointed out that we need to have more review of the changes though. Preferably RH or SuSE security teams should take a closer look at the changes and give their OK.

This means we most probably cannot hold the CRD of March, 29th.

Shall we move the CRD by one week to lets say April, 5th ?

I agree with the one week move. It will allow testing of the new reports also to ensure the patchset fixes everything.

Jeremy.

What about April, 3rd or 10th to avoid shipping (and making the issue public) the evening before the Easter weekend?

Karolin

Better the 10th as it gives a bit more time for testing.

Ok, let's make it the 10th !
Jeremy.

Created attachment 7418 [details]
First set of reproducers from ZDI

Created attachment 7419 [details]
Second set of reproducers from ZDI

The security researchers/ team members we like to ask if you're able to see further issues based on the two reproducers added?

If that's the case please speak up now or forever hold your peace.

Created attachment 7420 [details]
draft of the advisory

Comment on attachment 7420 [details]
draft of the advisory

The attached draft was written by Jeremy Allison.  Acknowledged by several Samba team members.

Is anyone willing to set the review flags?

Thanks!

Karolin

Comment on attachment 7420 [details]
draft of the advisory

this is not the latest version of the advisory

Created attachment 7421 [details]
draft of the advisory

I'll try to upload real patches this evening and ask for review flags then.

maybe tomorrow...

I'm concerned that this patch isn't correct (sorry).

I've looked at this chunk:

@@ -387,16 +411,18 @@ _PUBLIC_ enum ndr_err_code ndr_push_wbint_Principals(struct ndr_push *ndr, int n
 
 _PUBLIC_ enum ndr_err_code ndr_pull_wbint_Principals(struct ndr_pull *ndr, int ndr_flags, struct wbint_Principals *r)
 {
+	uint32_t size_principals_0 = 0;
 	uint32_t cntr_principals_0;
 	TALLOC_CTX *_mem_save_principals_0;
 	if (ndr_flags & NDR_SCALARS) {
 		NDR_CHECK(ndr_pull_array_size(ndr, &r->principals));
 		NDR_CHECK(ndr_pull_align(ndr, 5));
 		NDR_CHECK(ndr_pull_int32(ndr, NDR_SCALARS, &r->num_principals));
-		NDR_PULL_ALLOC_N(ndr, r->principals, ndr_get_array_size(ndr, &r->principals));
+		size_principals_0 = ndr_get_array_size(ndr, &r->principals);
+		NDR_PULL_ALLOC_N(ndr, r->principals, size_principals_0);
 		_mem_save_principals_0 = NDR_PULL_GET_MEM_CTX(ndr);
 		NDR_PULL_SET_MEM_CTX(ndr, r->principals, 0);
-		for (cntr_principals_0 = 0; cntr_principals_0 < r->num_principals; cntr_principals_0++) {
+		for (cntr_principals_0 = 0; cntr_principals_0 < size_principals_0; cntr_principals_0++) {
 			NDR_CHECK(ndr_pull_wbint_Principal(ndr, NDR_SCALARS, &r->principals[cntr_principals_0]));
 		}
 		NDR_PULL_SET_MEM_CTX(ndr, _mem_save_principals_0, 0);
@@ -406,9 +432,10 @@ _PUBLIC_ enum ndr_err_code ndr_pull_wbint_Principals(struct ndr_pull *ndr, int n
 		NDR_CHECK(ndr_pull_trailer_align(ndr, 5));
 	}
 	if (ndr_flags & NDR_BUFFERS) {
+		size_principals_0 = ndr_get_array_size(ndr, &r->principals);
 		_mem_save_principals_0 = NDR_PULL_GET_MEM_CTX(ndr);
 		NDR_PULL_SET_MEM_CTX(ndr, r->principals, 0);
-		for (cntr_principals_0 = 0; cntr_principals_0 < r->num_principals; cntr_principals_0++) {
+		for (cntr_principals_0 = 0; cntr_principals_0 < size_principals_0; cntr_principals_0++) {
 			NDR_CHECK(ndr_pull_wbint_Principal(ndr, NDR_BUFFERS, &r->principals[cntr_principals_0]));
 		}
 		NDR_PULL_SET_MEM_CTX(ndr, _mem_save_principals_0, 0);

For the memory overwrite in the PIDL code, the generated output is correct.  But there is still no tie (that I can see) between r->num_principals and the size of the array.  This means that we simply move the illigal memory access up the stack, to the application layer which is relying on r->num_principals matching up to the array size.

It seems to me that we must strictly tie the NDR size with application layer variable somehow, presumably via the size_is() expression.  (But that expression can be very complex...)

Am I missing something?

(In reply to comment #30)
> I'm concerned that this patch isn't correct (sorry).
> 
> I've looked at this chunk:
> 
> @@ -387,16 +411,18 @@ _PUBLIC_ enum ndr_err_code
> ndr_push_wbint_Principals(struct ndr_push *ndr, int n
> 
>  _PUBLIC_ enum ndr_err_code ndr_pull_wbint_Principals(struct ndr_pull *ndr, int
> ndr_flags, struct wbint_Principals *r)
>  {
> +    uint32_t size_principals_0 = 0;
>      uint32_t cntr_principals_0;
>      TALLOC_CTX *_mem_save_principals_0;
>      if (ndr_flags & NDR_SCALARS) {
>          NDR_CHECK(ndr_pull_array_size(ndr, &r->principals));
>          NDR_CHECK(ndr_pull_align(ndr, 5));
>          NDR_CHECK(ndr_pull_int32(ndr, NDR_SCALARS, &r->num_principals));
> -        NDR_PULL_ALLOC_N(ndr, r->principals, ndr_get_array_size(ndr,
> &r->principals));
> +        size_principals_0 = ndr_get_array_size(ndr, &r->principals);
> +        NDR_PULL_ALLOC_N(ndr, r->principals, size_principals_0);
>          _mem_save_principals_0 = NDR_PULL_GET_MEM_CTX(ndr);
>          NDR_PULL_SET_MEM_CTX(ndr, r->principals, 0);
> -        for (cntr_principals_0 = 0; cntr_principals_0 < r->num_principals;
> cntr_principals_0++) {
> +        for (cntr_principals_0 = 0; cntr_principals_0 < size_principals_0;
> cntr_principals_0++) {
>              NDR_CHECK(ndr_pull_wbint_Principal(ndr, NDR_SCALARS,
> &r->principals[cntr_principals_0]));
>          }
>          NDR_PULL_SET_MEM_CTX(ndr, _mem_save_principals_0, 0);
> @@ -406,9 +432,10 @@ _PUBLIC_ enum ndr_err_code
> ndr_pull_wbint_Principals(struct ndr_pull *ndr, int n
>          NDR_CHECK(ndr_pull_trailer_align(ndr, 5));
>      }
>      if (ndr_flags & NDR_BUFFERS) {
> +        size_principals_0 = ndr_get_array_size(ndr, &r->principals);
>          _mem_save_principals_0 = NDR_PULL_GET_MEM_CTX(ndr);
>          NDR_PULL_SET_MEM_CTX(ndr, r->principals, 0);
> -        for (cntr_principals_0 = 0; cntr_principals_0 < r->num_principals;
> cntr_principals_0++) {
> +        for (cntr_principals_0 = 0; cntr_principals_0 < size_principals_0;
> cntr_principals_0++) {
>              NDR_CHECK(ndr_pull_wbint_Principal(ndr, NDR_BUFFERS,
> &r->principals[cntr_principals_0]));
>          }
>          NDR_PULL_SET_MEM_CTX(ndr, _mem_save_principals_0, 0);
> 
> For the memory overwrite in the PIDL code, the generated output is correct. 
> But there is still no tie (that I can see) between r->num_principals and the
> size of the array.  This means that we simply move the illigal memory access up
> the stack, to the application layer which is relying on r->num_principals
> matching up to the array size.
> 
> It seems to me that we must strictly tie the NDR size with application layer
> variable somehow, presumably via the size_is() expression.  (But that
> expression can be very complex...)
> 
> Am I missing something?

While I haven't really found the exact diff that you are referring to, I think just looking at the patch alone is not sufficient. Further down in the code there used to be calls to ndr_check_array_size that should address exactly your concerns. They should still be there.

Yep, this looks correct once I dived into it more. 

But I'll continue to give a suspicious eye to the output, in case I can find something.  In doing these checks I've become more confident in the patches, which is a good thing. 

Thanks,

Created attachment 7426 [details]
Patches for master with gen_ndr

Only the commit one commit message was changed.

Created attachment 7427 [details]
Patches for master (and samba-4.0.0aplha18)

Only the commit one commit message was changed.

Created attachment 7428 [details]
Patches for v3-6-test

Only the commit one commit message was changed.

Created attachment 7429 [details]
Patches for v3-5-test

Only the commit one commit message was changed.

Created attachment 7430 [details]
Patches for v3-4-test

Only the commit one commit message was changed.

Created attachment 7431 [details]
Patches for v3-3-test

Only the commit one commit message was changed.

Created attachment 7432 [details]
Patches for v3-2-test

Only the commit one commit message was changed.

Created attachment 7433 [details]
python based reproducers for 3.0.x samba (draft)

ZDI did not provide reproducers for the issue in samba 3.0.x (for wkssvc dcerpc), here is a start how such a reproducer might look like.

Created attachment 7436 [details]
Assert array-lengths for NULL pointers to be 0

I'm sorry if I'm still missing something, but I would like someone to look over this particular part for me again.  I agree with Volker that the ndr_check_array_size() call is just outside the diff, but I think we still have an issue here, but in a different element. 

Consider lsa_BinaryString:

	typedef [public] struct {
		uint16 length;
		uint16 size;
		[size_is(size/2),length_is(length/2)] uint16 *array;
	} lsa_BinaryString;

Unlike my previous example, the array here is a pointer.  The critical check for the application layer is:
		if (r->array) {
			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->array, r->size / 2));
		}

That is, only if the client supplied the pointer, which it may not:

		NDR_CHECK(ndr_pull_generic_ptr(ndr, &_ptr_array));
		if (_ptr_array) {
			NDR_PULL_ALLOC(ndr, r->array);
		} else {
			r->array = NULL;
		}

However, the application layer assumes that if length is non-zero, the array is non-NULL. 

This happens in source3/rpc_server/samr/srv_samr_nt.c:4589 (set_user_info_21)

The attached patch for master asserts that for NULL pointers, that the size_is() and length_is() values are 0, if they are not themselves NULL pointers. 

(gen_ndr to follow, autobuild testing under-way, comments and review required)

Andrew Bartlett


_PUBLIC_ enum ndr_err_code ndr_pull_lsa_BinaryString(struct ndr_pull *ndr, int ndr_flags, struct lsa_BinaryString *r)
{
	uint32_t _ptr_array;
	uint32_t size_array_1 = 0;
	uint32_t length_array_1 = 0;
	uint32_t cntr_array_1;
	TALLOC_CTX *_mem_save_array_0;
	TALLOC_CTX *_mem_save_array_1;
	NDR_PULL_CHECK_FLAGS(ndr, ndr_flags);
	if (ndr_flags & NDR_SCALARS) {
		NDR_CHECK(ndr_pull_align(ndr, 5));
		NDR_CHECK(ndr_pull_uint16(ndr, NDR_SCALARS, &r->length));
		NDR_CHECK(ndr_pull_uint16(ndr, NDR_SCALARS, &r->size));
		NDR_CHECK(ndr_pull_generic_ptr(ndr, &_ptr_array));
		if (_ptr_array) {
			NDR_PULL_ALLOC(ndr, r->array);
		} else {
			r->array = NULL;
		}
		NDR_CHECK(ndr_pull_trailer_align(ndr, 5));
	}
	if (ndr_flags & NDR_BUFFERS) {
		if (r->array) {
			_mem_save_array_0 = NDR_PULL_GET_MEM_CTX(ndr);
			NDR_PULL_SET_MEM_CTX(ndr, r->array, 0);
			NDR_CHECK(ndr_pull_array_size(ndr, &r->array));
			NDR_CHECK(ndr_pull_array_length(ndr, &r->array));
			size_array_1 = ndr_get_array_size(ndr, &r->array);
			length_array_1 = ndr_get_array_length(ndr, &r->array);
			if (length_array_1 > size_array_1) {
				return ndr_pull_error(ndr, NDR_ERR_ARRAY_SIZE, "Bad array size %u should exceed array length %u", size_array_1, length_array_1);
			}
			NDR_PULL_ALLOC_N(ndr, r->array, size_array_1);
			_mem_save_array_1 = NDR_PULL_GET_MEM_CTX(ndr);
			NDR_PULL_SET_MEM_CTX(ndr, r->array, 0);
			for (cntr_array_1 = 0; cntr_array_1 < length_array_1; cntr_array_1++) {
				NDR_CHECK(ndr_pull_uint16(ndr, NDR_SCALARS, &r->array[cntr_array_1]));
			}
			NDR_PULL_SET_MEM_CTX(ndr, _mem_save_array_1, 0);
			NDR_PULL_SET_MEM_CTX(ndr, _mem_save_array_0, 0);
		}
		if (r->array) {
			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->array, r->size / 2));
		}
		if (r->array) {
			NDR_CHECK(ndr_check_array_length(ndr, (void*)&r->array, r->length / 2));
		}
	}
	return NDR_ERR_SUCCESS;
}

Created attachment 7437 [details]
gen_ndr for master after my patch

Sorry for the noise.  

While I still think the issue I found is genuine, it is distinct, and patching the IDL layer is difficult as winreg (at least) expects this behaviour.  We may have to find and fix all the callers.

I'll try and find someone on the team to explore this in more detail with over Easter, and perhaps file a new bug.

Still waiting for the review flags. I will go ahead anyway if there are no objections.

While the formal review flag has been missing, please go ahead. All comments say this has been positively tested.

Ok, I'll put my neck on the line and positively review :-). I have tested these patches (and caused them to be tested at several OEM's) and they've passed.

Jeremy.

(In reply to comment #46)
> Ok, I'll put my neck on the line and positively review :-). I have tested these
> patches (and caused them to be tested at several OEM's) and they've passed.
> 
> Jeremy.

Thanks, Jeremy! :-)

What time does this go public today?

(In reply to comment #48)
> What time does this go public today?

~1h

(In reply to comment #34)
> Created attachment 7427 [details]
> Patches for master
> 
> Only the commit one commit message was changed.

Pushed to autobuild.

(In reply to comment #33)
> Created attachment 7426 [details]
> Patches for master with gen_ndr
> 
> Only the commit one commit message was changed.

Metze, are these patches needed in addition to the "Patches for master"?

Patches for v3-6, v3-5 and v3-4 have been pushed to the release branches and Samba 3.6.4, 3.5.14 and 3.4.16 have been issued as security releases.
Closing out bug report.

Thanks!

(In reply to comment #51)
> (In reply to comment #33)
> > Created attachment 7426 [details] [details]
> > Patches for master with gen_ndr
> > 
> > Only the commit one commit message was changed.
> 
> Metze, are these patches needed in addition to the "Patches for master"?

No, they're just for review the resulting changes

Comment on attachment 7427 [details]
Patches for master (and samba-4.0.0aplha18)

This also applies to samba-4.0.0alpha18

The content of attachment 7433 [details] has been deleted by
    Lars Müller <lars@samba.org>
who provided the following reason:

Remove 3.0 reproducer.

The token used to delete this attachment was generated at 2012-04-11 14:15:55 UTC.

The content of attachment 7418 [details] has been deleted by
    Lars Müller <lars@samba.org>
who provided the following reason:

Remove reproducer.

The token used to delete this attachment was generated at 2012-04-11 16:51:17 UTC.

The content of attachment 7419 [details] has been deleted by
    Lars Müller <lars@samba.org>
who provided the following reason:

Remove reproducer.

The token used to delete this attachment was generated at 2012-04-11 16:52:00 UTC.