Bug 14417 (CVE-2020-14303) - CVE-2020-14303 [SECURITY] Endless loop from empty UDP packet sent to AD DC nbt_server
Summary: CVE-2020-14303 [SECURITY] Endless loop from empty UDP packet sent to AD DC nb...
Status: RESOLVED FIXED
Alias: CVE-2020-14303
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.12.3
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 14412
  Show dependency treegraph
 
Reported: 2020-06-23 22:16 UTC by Andrew Bartlett
Modified: 2021-08-31 04:59 UTC (History)
6 users (show)

See Also:


Attachments
Proposed patch for master V1 (1.35 KB, patch)
2020-06-24 01:18 UTC, Gary Lockyer
no flags Details
Alternative patch for master (905 bytes, patch)
2020-06-24 02:32 UTC, Gary Lockyer
no flags Details
advisory without CVE number (1.91 KB, text/plain)
2020-06-24 04:58 UTC, Andrew Bartlett
no flags Details
Advisory v2 (1.92 KB, text/plain)
2020-06-24 08:05 UTC, Andrew Bartlett
gary: review+
Details
Patch for master V2 (1.55 KB, patch)
2020-06-24 22:31 UTC, Gary Lockyer
abartlet: review+
gary: ci-passed+
Details
Patch for V4.12 (1.55 KB, patch)
2020-06-24 22:32 UTC, Gary Lockyer
abartlet: review+
gary: ci-passed+
Details
Patch for V4.11 (1.55 KB, patch)
2020-06-24 22:34 UTC, Gary Lockyer
abartlet: review+
gary: ci-passed+
Details
Patch for V4.10 (1.55 KB, patch)
2020-06-24 22:35 UTC, Gary Lockyer
abartlet: review+
abartlet: ci-passed+
Details
patch for master to fix this issue (with tests!) (v3) (3.71 KB, patch)
2020-06-25 00:19 UTC, Andrew Bartlett
abartlet: review-
Details
patch for 4.12 to fix this issue (with tests, on CVE-2020-10745!) (v4) (4.11 KB, patch)
2020-06-25 00:31 UTC, Andrew Bartlett
abartlet: review? (gary)
gary: review+
abartlet: ci-passed+
Details
Patch for V4.5 (1.55 KB, patch)
2020-06-25 00:32 UTC, Gary Lockyer
abartlet: review+
Details
patch for master to fix this issue (with tests, on CVE-2020-10745!) (v4) (4.11 KB, patch)
2020-06-25 00:44 UTC, Andrew Bartlett
abartlet: review? (gary)
gary: review+
abartlet: ci-passed+
Details
patch for 4.11 to fix this issue (with tests, on CVE-2020-10745!) (v4) (4.11 KB, patch)
2020-06-25 01:26 UTC, Andrew Bartlett
abartlet: review? (gary)
gary: review+
abartlet: ci-passed+
Details
patch for 4.10 to fix this issue (with tests, on CVE-2020-10745!) (v4) (4.11 KB, patch)
2020-06-25 01:38 UTC, Andrew Bartlett
gary: review+
Details
advisory v3 with credits (2.08 KB, text/plain)
2020-06-25 01:42 UTC, Andrew Bartlett
no flags Details
patch for 4.10 to fix this issue (with tests, on CVE-2020-10745!) (v4.1) - fix python 3.4 issue (4.22 KB, patch)
2020-06-25 03:08 UTC, Andrew Bartlett
metze: review+
dbagnall: review+
abartlet: ci-passed+
Details
advisory v4 (2.09 KB, text/plain)
2020-06-25 04:20 UTC, Andrew Bartlett
dbagnall: review+
Details
Patch for V4.5 (with tests) (4.33 KB, patch)
2020-06-26 03:16 UTC, Gary Lockyer
abartlet: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2020-06-23 22:16:11 UTC
Sending an empty UDP packet to port 137 on a Samba AD DC results in a spin:

19:32:33.040469 getpid()                = 20042
    19:32:33.040599 epoll_wait(7, [{EPOLLIN, {u32=1855594528, u64=94538380746784}}], 1, 6964367) = 1
    19:32:33.040761 ioctl(26, FIONREAD, [0]) = 0
    19:32:33.040912 getpid()                = 20042
    19:32:33.041074 epoll_wait(7, [{EPOLLIN, {u32=1855594528, u64=94538380746784}}], 1, 6964367) = 1
    19:32:33.041307 ioctl(26, FIONREAD, [0]) = 0
    19:32:33.041457 getpid()                = 20042
    19:32:33.041524 epoll_wait(7, [{EPOLLIN, {u32=1855594528, u64=94538380746784}}], 1, 6964366) = 1
    19:32:33.041704 ioctl(26, FIONREAD, [0]) = 0
    19:32:33.042014 getpid()                = 20042
    19:32:33.042223 epoll_wait(7, [{EPOLLIN, {u32=1855594528, u64=94538380746784}}], 1, 6964366) = 1
    19:32:33.042370 ioctl(26, FIONREAD, [0]) = 0
    19:32:33.042654 getpid()                = 20042
    19:32:33.042808 epoll_wait(7, [{EPOLLIN, {u32=1855594528, u64=94538380746784}}], 1, 6964365) = 1
    19:32:33.043001 ioctl(26, FIONREAD, [0]) = 0
    19:32:33.043198 getpid()                = 20042
    19:32:33.043328 epoll_wait(7, [{EPOLLIN, {u32=1855594528, u64=94538380746784}}], 1, 6964365) = 1
    19:32:33.043536 ioctl(26, FIONREAD, [0]) = 0
    19:32:33.043735 getpid()                = 20042
Comment 1 Jeremy Allison 2020-06-24 01:18:25 UTC
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.
Comment 2 Gary Lockyer 2020-06-24 01:18:25 UTC
Created attachment 16070 [details]
Proposed patch for master V1
Comment 3 Jeremy Allison 2020-06-24 01:19:46 UTC
Oh sorry Gary, you were already ahead of me ! Glad we came to the same conclusion as to the correct fix though :-).
Comment 4 Jeremy Allison 2020-06-24 01:26:32 UTC
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 ?
Comment 5 Gary Lockyer 2020-06-24 01:38:25 UTC
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.
Comment 6 Gary Lockyer 2020-06-24 02:32:53 UTC
Created attachment 16071 [details]
Alternative patch for master

Alternate patch in line with my previous comments.
Comment 7 Andrew Bartlett 2020-06-24 02:37:17 UTC
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)
Comment 8 Andrew Bartlett 2020-06-24 03:06:44 UTC
(In reply to Gary Lockyer from comment #6)
I prefer this approach.
Comment 9 Jeremy Allison 2020-06-24 03:24:12 UTC
(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 :-).
Comment 10 Jeremy Allison 2020-06-24 03:26:21 UTC
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 11 Jeremy Allison 2020-06-24 03:33:44 UTC
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 ?
Comment 12 Gary Lockyer 2020-06-24 03:40:13 UTC
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
Comment 13 Andrew Bartlett 2020-06-24 03:44:26 UTC
(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.
Comment 14 Andrew Bartlett 2020-06-24 04:58:54 UTC
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.
Comment 15 Andrew Bartlett 2020-06-24 08:05:39 UTC
Created attachment 16073 [details]
Advisory v2

Still with speculative release versions.
Comment 16 Gary Lockyer 2020-06-24 22:31:35 UTC
Created attachment 16075 [details]
Patch for master V2
Comment 17 Gary Lockyer 2020-06-24 22:32:45 UTC
Created attachment 16076 [details]
Patch for V4.12
Comment 18 Gary Lockyer 2020-06-24 22:34:03 UTC
Created attachment 16077 [details]
Patch for V4.11
Comment 19 Gary Lockyer 2020-06-24 22:35:33 UTC
Created attachment 16078 [details]
Patch for V4.10
Comment 20 Andrew Bartlett 2020-06-25 00:19:22 UTC
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.
Comment 21 Andrew Bartlett 2020-06-25 00:31:52 UTC
Created attachment 16080 [details]
patch for 4.12 to fix this issue (with tests, on CVE-2020-10745!) (v4)
Comment 22 Gary Lockyer 2020-06-25 00:32:04 UTC
Created attachment 16081 [details]
Patch for V4.5
Comment 23 Andrew Bartlett 2020-06-25 00:44:03 UTC
Created attachment 16082 [details]
patch for master to fix this issue (with tests, on CVE-2020-10745!) (v4)
Comment 24 Andrew Bartlett 2020-06-25 01:26:08 UTC
Created attachment 16083 [details]
patch for 4.11 to fix this issue (with tests, on CVE-2020-10745!) (v4)
Comment 25 Andrew Bartlett 2020-06-25 01:38:34 UTC
Created attachment 16084 [details]
patch for 4.10 to fix this issue (with tests, on CVE-2020-10745!) (v4)
Comment 26 Andrew Bartlett 2020-06-25 01:42:48 UTC
Created attachment 16085 [details]
advisory v3 with credits
Comment 27 Andrew Bartlett 2020-06-25 03:08:05 UTC
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
Comment 28 Andrew Bartlett 2020-06-25 04:20:25 UTC
Created attachment 16087 [details]
advisory v4
Comment 29 Karolin Seeger 2020-06-25 06:37:08 UTC
Planned release date: Thursday, July 2nd 2020
Comment 30 Andrew Bartlett 2020-06-25 07:47:35 UTC
Opening up to vendors.
Comment 31 Gary Lockyer 2020-06-26 03:16:45 UTC
Created attachment 16092 [details]
Patch for V4.5 (with tests)
Comment 32 Karolin Seeger 2020-07-02 08:53:34 UTC
Samba 4.12.4, 4.11.11 and 4.10.17 have been shipped to address this defect.
Comment 33 Karolin Seeger 2020-07-02 08:57:21 UTC
Pushed to autobuild-master.
Comment 34 Karolin Seeger 2020-07-02 09:08:37 UTC
Merged into v4-{12,11,10}-test.
Comment 35 Karolin Seeger 2020-07-03 09:21:58 UTC
Pushed to master.
Closing out bug report.

Thanks!
Comment 36 Andrew Bartlett 2020-07-21 19:44:32 UTC
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.
Comment 37 Andrew Bartlett 2021-08-31 04:59:53 UTC
Actually removing vendors and opening up now.