Took a quick look at this trying to be helpful...

Looks like in nbt_name_socket_recv() we call socket_pending(nbtsock->sock, &dsize), which will return a dsize of zero.

We don't check for a zero dsize so allocate a zero-length DATA_BLOB, and then call into socket_recvfrom(..., blob.length,...) which for NBT should end up in ipv4_recvfrom().

ipv4_recvfrom() has:

        *nread = 0;

        gotlen = recvfrom(sock->fd, buf, wantlen, 0, 
                          src->sockaddr, &from_len);
        if (gotlen == 0) {
                talloc_free(src);
                return NT_STATUS_END_OF_FILE;
        }

and sock->fd I believe is non-blocking, so when wantlen == 0, I would assume it returns gotlen of zero, and return EOF. However, calling recvfrom() with a length of zero I don't think clears the pending read status on the socket, and so we go round again (and again...).

A quick check would be to ensure if wantlen == 0 we do an explicit 1 byte read to clear the pending fd condition.

That should be done inside nbt_name_socket_recv(), not ipv4_recvfrom() however, because if dsize == 0 here then at this point we *know* we're trying to read UDP messages, and a zero length UDP means someone is messing with us.

ipv4_recvfrom() also handles TCP messages I think so getting a zero byte read request only means the data might not have arrived yet.

Created attachment 16070 [details]
Proposed patch for master V1

Oh sorry Gary, you were already ahead of me ! Glad we came to the same conclusion as to the correct fix though :-).

Just one comment on your fix - shouldn't the length to your new call to socket_recvfrom() be 1 instead of dsize ? We already know dsize == 0 above and that looks to me your code adds the same call we would already be doing after the data_blob_talloc() of a zero length blob. Asking for 1 byte doesn't hurt as we already know this call will return zero and the socket is non-blocking so we won't wait for more data.

Am I missing something ?

I don't think so, that was me just playing safe. Well I tried a zero byte read and it worked.

But looking at the other places in the code were socket_pending is called I wonder if the code should just be.

blob = data_blob_talloc(tmp_ctx, NULL, dsize);                                                                              
if (blob.data == NULL && dsize != 0) {                                                                                                    
»       talloc_free(tmp_ctx);                                                                                               
»       return;
}

which would be consistent to the other uses.

Created attachment 16071 [details]
Alternative patch for master

Alternate patch in line with my previous comments.

Because we expect that the FD will note be closed due to the non-processing of the packet, the CVSS score is:

CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H (7.5)

(In reply to Gary Lockyer from comment #6)
I prefer this approach.

(In reply to Gary Lockyer from comment #5)

Ah, now I see why your patch works !

The call to:

blob = data_blob_talloc(tmp_ctx, NULL, dsize);

will return a blob with blob.data==NULL, blob.length==0
so it gets caught by the blob allocation check in
the next line


        if (blob.data == NULL) {
                talloc_free(tmp_ctx);
                return;
        }

so never gets to call the following socket_recvfrom()
with the zero length read.

Wow. That depends on the semantics of data_blob_talloc()
returning a null blob when dsize==0, which I didn't realize
(that's different to malloc(), which returns a valid pointer
to zero-length memory when you ask for a length of zero. I
know this as it used to be my Google interview warm-up
question :-).

I was assuming the socket_recvfrom() was being called already with a zero-length read len but this didn't clear the condition. Turns out the zero-length read does clear it.

Well you learn something new every day :-).

Comment on attachment 16071 [details]
Alternative patch for master

If we go with this alternate patch, can you please add a comment around the check on the return from data_blob_talloc() return explaining the strange semantics in the error checking here.

Something like:

/*
 * Given a zero length, data_blob_talloc() returns the NULL blob {NULL,0}.
 *
 * We only want to error return here on a real out of memory condition (i.e.
 * dsize != 0, so the UDP packet has data, but the return of the allocation
 * failed, so blob.data==NULL).
 *
 * Given an actual zero length UDP packet having blob.data == NULL isn't an
 * out of memory error condition, that's the defined semantics of data_blob_talloc()
 * when asked for zero bytes.
 *
 * We still need to continue to do the zero-length socket_recvfrom() read
 * in order to clear the "read pending" condition on the socket.
 */

Does that help ?

That makes sense.

Outside this bug I'll also add a similar comment to the other places that use
a similar pattern.

source4/libcli/dgram/dgramsocket.c:44
source4/auth/kerberos/krb5_init_context.c:94

They both have the dsize == 0 check already

(In reply to Andrew Bartlett from comment #7)
This is incorrect, we don't leak an FD, we just stop responding to new name queries. 

Still, the severity is the same.

Created attachment 16072 [details]
advisory without CVE number

I've written this draft on the assumption that we will slip this into the impending release.  If not we will just need to increase the release versions.

Created attachment 16073 [details]
Advisory v2

Still with speculative release versions.

Created attachment 16075 [details]
Patch for master V2

Created attachment 16076 [details]
Patch for V4.12

Created attachment 16077 [details]
Patch for V4.11

Created attachment 16078 [details]
Patch for V4.10

Created attachment 16079 [details]
patch for master to fix this issue (with tests!) (v3)

This patch adds tests.   This one requires the patches for CVE-2020-10745 to be applied first.

The reason we have kept v2 up is that there is a chance this security issue will be in the upcoming security release and so the v2 patches are independent, already reviewed and under CI.

Created attachment 16080 [details]
patch for 4.12 to fix this issue (with tests, on CVE-2020-10745!) (v4)

Created attachment 16081 [details]
Patch for V4.5

Created attachment 16082 [details]
patch for master to fix this issue (with tests, on CVE-2020-10745!) (v4)

Created attachment 16083 [details]
patch for 4.11 to fix this issue (with tests, on CVE-2020-10745!) (v4)

Created attachment 16084 [details]
patch for 4.10 to fix this issue (with tests, on CVE-2020-10745!) (v4)

Created attachment 16085 [details]
advisory v3 with credits

Created attachment 16086 [details]
patch for 4.10 to fix this issue (with tests, on CVE-2020-10745!) (v4.1) - fix python 3.4 issue

Created attachment 16087 [details]
advisory v4

Planned release date: Thursday, July 2nd 2020

Opening up to vendors.

Created attachment 16092 [details]
Patch for V4.5 (with tests)

Samba 4.12.4, 4.11.11 and 4.10.17 have been shipped to address this defect.

Pushed to autobuild-master.

Merged into v4-{12,11,10}-test.

Pushed to master.
Closing out bug report.

Thanks!

Opening to the public and removing the samba-vendor alias from CC.  

Vendors: CC individually if you wish to follow along.

(In reply to Gary Lockyer from comment #12)
Gary, now is the time to do that extra comment work.

Actually removing vendors and opening up now.