Bug 15422 (CVE-2023-3961) - CVE-2023-3961 [SECURITY] Unsanitized client pipe name passed to local_np_connect()
Summary: CVE-2023-3961 [SECURITY] Unsanitized client pipe name passed to local_np_conn...
Status: RESOLVED FIXED
Alias: CVE-2023-3961
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DCE-RPCs and pipes (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 15483
  Show dependency tree / graph
 
Reported: 2023-07-17 22:45 UTC by Jeremy Allison
Modified: 2023-10-20 09:14 UTC (History)
6 users (show)

See Also:

Attachments
git-am fix for master. (9.49 KB, patch)
2023-07-26 00:57 UTC, Jeremy Allison 		no flags Details
git-am fix for master. (9.45 KB, patch)
2023-07-26 18:02 UTC, Jeremy Allison 		slow: review+
jra: ci-passed+
Details
Updated patch for master. (9.51 KB, patch)
2023-08-15 00:13 UTC, Jeremy Allison 		slow: review+
Details
First (rough) draft of an advisory. (2.63 KB, text/plain)
2023-08-16 16:12 UTC, Jeremy Allison 		slow: review+
Details
patch in master backported to Samba 4.19 v1 (9.51 KB, patch)
2023-09-13 02:11 UTC, Andrew Bartlett 		slow: review+
jra: review+
abartlet: ci-passed+
Details
patch in master backported to Samba 4.18 v1 (9.51 KB, patch)
2023-09-13 02:12 UTC, Andrew Bartlett 		jra: review+
abartlet: ci-passed+
Details
patch in master backported to Samba 4.17 v1 (9.39 KB, patch)
2023-09-13 02:13 UTC, Andrew Bartlett 		slow: review+
jra: review+
abartlet: ci-passed+
Details
patch for master backported to Samba 4.16 v1 (9.36 KB, patch)
2023-09-13 02:13 UTC, Andrew Bartlett 		slow: review+
jra: review+
abartlet: ci-passed+
Details
Updated patch for master (v1.1) (9.41 KB, patch)
2023-09-25 23:01 UTC, Jeremy Allison 		slow: review+
jra: ci-passed+
Details
git-am fix for 4.18.next - rebased after 4.18.7 release (9.49 KB, patch)
2023-09-27 17:03 UTC, Jeremy Allison 		slow: review+
jra: ci-passed+
Details
Updated patch for master (v1.2) (9.44 KB, patch)
2023-10-04 21:56 UTC, Jeremy Allison 		slow: review+
jra: ci-passed+
Details
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2023-07-17 22:45:24 UTC 
I'm not sure if this is a security bug, but I'm marking it as one and closed out of an abundance of caution.

------------------------------------------------------
I started checking the pathname processing for SMB2 out of an abundance of
paranoia.

The passed in path is pulled out of the SMB2
packet in smbd_smb2_request_process_create()
using convert_string_talloc(), so the raw
client-sent name is now in 'in_name_string'.

This is passed to smbd_smb2_create_send()
where it's received as the parameter 'in_name'.

If the DFS flag is not set it then gets
talloc'ed into:

state->fname = talloc_strdup(state, in_name);

and if this is an IPC$ share this code path
is taken:

	if (IS_IPC(smb1req->conn)) {
                const char *pipe_name = in_name;
                ...
                status = open_np_file(smb1req, pipe_name, &state->result);

Note - pipe_name here is the raw, unprocessed
client name (possibly containing ../../../ characters).

open_np_file() passes pipe_name to np_open().
np_open() passes the given name to local_np_connect().

local_np_connect() lower cases the passed in name,
and then calls:

        state->socketpath = talloc_asprintf(
                state, "%s/np/%s", socket_dir, lower_case_pipename);
        if (tevent_req_nomem(state->socketpath, req)) {
                return tevent_req_post(req, ev);
        }

If lower_case_pipename contains / and .. components
I think it can escape from the socket_dir/np path.
Comment 1 Jeremy Allison 2023-07-17 22:45:49 UTC 
Note, inside the SMB1 code reply_open_pipe_and_X()
there is this code:

       /* If the name doesn't start \PIPE\ then this is directed */
        /* at a mailslot or something we really, really don't understand, */
        /* not just something we really don't understand. */

#define PIPE            "PIPE\\"
#define PIPELEN         strlen(PIPE)

        fname = pipe_name;
        while (fname[0] == '\\') {
                fname++;
        }
        if (!strnequal(fname, PIPE, PIPELEN)) {
                reply_nterror(req, NT_STATUS_OBJECT_PATH_SYNTAX_BAD);
                return;
        }
        fname += PIPELEN;
        while (fname[0] == '\\') {
                fname++;
        }

        DEBUG(4,("Opening pipe %s => %s.\n", pipe_name, fname));
        status = open_np_file(req, fname, &fsp);

This is also unsanitized, and I think has the same problem.
Comment 2 Jeremy Allison 2023-07-17 22:46:29 UTC 
Here is a prototype fix. Compiles, but we need to add
a test to pass a pipename with multi-path components
into SMB1 and SMB2.

diff --git a/source3/smbd/smb2_pipes.c b/source3/smbd/smb2_pipes.c
index 8f8786752db..2a3afcbfc16 100644
--- a/source3/smbd/smb2_pipes.c
+++ b/source3/smbd/smb2_pipes.c
@@ -43,6 +43,12 @@ NTSTATUS open_np_file(struct smb_request *smb_req, const char *name,
        struct auth_session_info *session_info = conn->session_info;
        NTSTATUS status;
+	if (ISDOT(name) || ISDOTDOT(name) ||
+                       (strchr(name, '/') != NULL) ||
+                       (strchr(name, '\\') != NULL)) {
+               return NT_STATUS_OBJECT_PATH_SYNTAX_BAD;
+	}
+
        status = file_new(smb_req, conn, &fsp);
        if (!NT_STATUS_IS_OK(status)) {
                DEBUG(0, ("file_new failed: %s\n", nt_errstr(status)));
Comment 3 Andrew Bartlett 2023-07-18 09:29:48 UTC 
Why not just restrict pipe names to A-Z and a-z?
Comment 4 Jeremy Allison 2023-07-18 17:24:34 UTC 
(In reply to Andrew Bartlett from comment #3)

Hmmm. I don't think any pipe names are internationalized. Still, the fix given prevents the actual problem elements (unix path components) so I think I'll just use it as a 'minimal' change for now.
Comment 5 Jeremy Allison 2023-07-25 22:13:33 UTC 
We will need a CVE for this. The socket connect is done as root.
Comment 6 Jeremy Allison 2023-07-26 00:57:38 UTC 
Created attachment 18001 [details]
git-am fix for master.

Ralph, can you look this over ? We'll also need a CVE number and I'll write an advisory too.
Comment 7 Ralph Böhme 2023-07-26 14:22:46 UTC 
Comment on attachment 18001 [details]
git-am fix for master.

Code looks good, thanks! :)

Style could be slightly improved to match README.Coding if that would be possible?

+	if (ISDOTDOT(lower_case_pipename) ||
+	    (strchr(lower_case_pipename, '/') != NULL))
+	{
+		DBG_DEBUG("attempt to connect to invalid pipe pathname %s\n",
+			  lower_case_pipename);

Thanks!
Comment 8 Jeremy Allison 2023-07-26 16:07:30 UTC 
(In reply to Ralph Böhme from comment #7)

No problem, will do !
Comment 9 Jeremy Allison 2023-07-26 18:02:48 UTC 
Created attachment 18003 [details]
git-am fix for master.

Updated as requested for coding standards. Asked for a CVE from RH product security.
Comment 10 Ralph Böhme 2023-07-26 18:06:04 UTC 
Comment on attachment 18003 [details]
git-am fix for master.

Thanks, much better. Though there's still missing spaces around the "!=" comparison operator... Maybe next time. :)))

Btw, remember to run private CI on sn-devel...
Comment 11 Jeremy Allison 2023-07-26 18:16:32 UTC 
(In reply to Ralph Böhme from comment #10)

"remember to run private CI on sn-devel..."

Is there a step-by-step guide for how to do this ? If not, I'll write one once I've done it :-).
Comment 12 Ralph Böhme 2023-07-26 18:25:21 UTC 
(In reply to Jeremy Allison from comment #11)
Cf doc/autobuild.txt in the team repo. Look for "private autobuild". Hth! :)
Comment 13 Jeremy Allison 2023-07-26 21:46:55 UTC 
CVE-2023-3961
Comment 14 Andreas Schneider 2023-08-02 06:10:18 UTC 
+	/*
+	 * Ensure we cannot process a path that exits
+	 * the socket_dir.
+	 */

I don't get that comment. Maybe go a bit more into details?
Comment 15 Jeremy Allison 2023-08-02 17:52:34 UTC 
(In reply to Andreas Schneider from comment #14)

OK, I'll expand the comment. The code checks that the path isn't ".." (which is an obvious open of the parent directory) or that the path doesn't contain a '/' character, which is a prerequisite for a path that escapes from the constructed socket pathname.

Remember, the socket path we open is:

state->socketpath = talloc_asprintf(
 		state, "%s/np/%s", socket_dir, lower_case_pipename);

where socket_dir comes is an absolute pathname that from the config (so is safe), but lower_case_pipename is supplied by the connecting client.

So if the client sends a pathname of (for example):

"../../../../../../../tmp/X11/.some-x11-socket"

then we connect (as root) to whatever socket we can get to on the filesystem.

I'm not sure what we can do with that connection, but we shouldn't be able to make it in the first place.
Comment 16 Jeremy Allison 2023-08-15 00:13:16 UTC 
Created attachment 18050 [details]
Updated patch for master.
Comment 17 Jeremy Allison 2023-08-15 16:57:16 UTC 
Comment on attachment 18003 [details]
git-am fix for master.

Passed a private autobuild. Now to work on the advisory.
Comment 18 Jeremy Allison 2023-08-15 23:52:32 UTC 
I get this for the CVE score. If someone who knows what they're doing can confirm I'd appreciate it :-).

CVSS v3.1 Vector
AV:N/AC:L/PR:N/UI:N/S:C/C:L/I:L/A:N/E:P/RL:O/RC:C/CR:L/IR:X/AR:L/MAV:N/MAC:L/MPR:N/MUI:N/MS:C/MC:L/MI:X/MA:N
Overall CVSS Score:
5.9

CVSS Base Score:
7.2
Impact Subscore:
2.7
Exploitability Subscore:
3.9
CVSS Temporal Score:
6.5
CVSS Environmental Score:
5.9
Modified Impact Subscore:
2.1
Overall CVSS Score:
5.9
Comment 19 Jeremy Allison 2023-08-16 16:12:35 UTC 
Created attachment 18062 [details]
First (rough) draft of an advisory.
Comment 20 Andrew Bartlett 2023-09-13 02:11:40 UTC 
Created attachment 18099 [details]
patch in master backported to Samba 4.19 v1
Comment 21 Andrew Bartlett 2023-09-13 02:12:08 UTC 
Created attachment 18100 [details]
patch in master backported to Samba 4.18 v1
Comment 22 Andrew Bartlett 2023-09-13 02:13:06 UTC 
Created attachment 18101 [details]
patch in master backported to Samba 4.17 v1
Comment 23 Andrew Bartlett 2023-09-13 02:13:39 UTC 
Created attachment 18102 [details]
patch for master backported to Samba 4.16 v1
Comment 24 Jeremy Allison 2023-09-13 17:27:44 UTC 
I just want to say thanks for the back-ports Andrew, this is much appreciated.
Comment 25 Jeremy Allison 2023-09-25 23:01:40 UTC 
Created attachment 18124 [details]
Updated patch for master (v1.1)

No change in code from previous version, I just needed to update the back-port to rebase on current master changes in smbtorture code.
Comment 26 Andrew Bartlett 2023-09-26 02:05:49 UTC 
Comment on attachment 18124 [details]
Updated patch for master (v1.1)

So this is the same patch just with different context, so the backports are not impacted?
Comment 27 Jeremy Allison 2023-09-26 04:41:42 UTC 
(In reply to Andrew Bartlett from comment #26)

Yes, exactly the same patch - just rebased to apply to current master (master had some more tests added to smbtorture which meant the originally reviewed patch didn't apply cleanly).
Comment 28 Jeremy Allison 2023-09-26 04:42:53 UTC 
When we get a release date I'll check that the back-ports apply cleanly to their relevent branches - some of the fixes in master with tests associated got back-ported to 4.19.next and 4.18.next too (from the SNIA SDC fixes).
Comment 29 Andrew Bartlett 2023-09-26 05:06:33 UTC 
(In reply to Jeremy Allison from comment #28)
Release date is 10 October.
Comment 30 Andrew Bartlett 2023-09-26 05:08:38 UTC 
(In reply to Andrew Bartlett from comment #29)
Assuming no more patches before "Wednesday, September 27 2023 - Planned release date for Samba 4.18.7." then checking this now should be enough.
Comment 31 Jeremy Allison 2023-09-27 17:03:33 UTC 
Created attachment 18128 [details]
git-am fix for 4.18.next - rebased after 4.18.7 release

Rebased after smbtorture changes to 4.18.7 release. No code changes.
Comment 32 Andrew Bartlett 2023-09-29 03:24:57 UTC 
Assigning to Jule for security release.
Comment 33 Jule Anger 2023-10-02 07:21:54 UTC 
Opening security bugs to vendors. Release date is currently proposed to be October 10.
Comment 34 Jeremy Allison 2023-10-04 21:56:38 UTC 
Created attachment 18151 [details]
Updated patch for master (v1.2)

Updated to add the 3 new parameters to smbXcli_negprot() recently added. All 3 parameters are NULL (the default).

No other changes.
Comment 35 Jule Anger 2023-10-10 14:41:03 UTC 
Removing vendor CC (so that any public comments don't need to be broadcast so widely) and opening these bugs to the public.
If you wish to continue to be informed about any changes here please CC individually.
Comment 36 Samba QA Contact 2023-10-10 14:46:18 UTC 
This bug was referenced in samba v4-18-stable:

1688b6d3dd4d6490e10c57d7431cb300f8286e73
fbb9cf8d11840828cbb2ef2249f10fbf766d65ea
682a9a808b4578fef2d984e47d227bc181603cbf
Comment 37 Samba QA Contact 2023-10-10 14:47:11 UTC 
This bug was referenced in samba v4-17-stable:

23199e115457e8054c905eedee95ebee114809e0
e6f096c4c8fb3e19fff954c2ddd9c329fb86b06a
f958415a69fdd6324810e16a050e25b821ba204b
Comment 38 Samba QA Contact 2023-10-10 14:48:41 UTC 
This bug was referenced in samba v4-19-stable:

5dab2cfde7e45b9e366325ed1f75b65fc6e30d90
45d584532f865a39432b9a3f179fdeb5cbef2f76
f17abf9c4a7ce75893a46f90dde5871501b78e86
Comment 39 Samba QA Contact 2023-10-10 15:01:13 UTC 
This bug was referenced in samba v4-17-stable:

e5a1c1cfb0a73a37001afee530ae09bf5c58b515
125ce23115b92045a1584f5654669180bea83067
4b3e5c2f036f868e38ad5da7faba05db32f624f4
Comment 40 Samba QA Contact 2023-10-10 15:07:04 UTC 
This bug was referenced in samba v4-19-stable:

67c6778534d8fc1f6ce20cfb67d682b6f16ce1b9
44d59c380afbd227243d1dcf65b17cb445357c0f
456a758f10c8163122d1746d40a03df6f3f7b391
Comment 41 Samba QA Contact 2023-10-10 15:08:03 UTC 
This bug was referenced in samba v4-18-stable:

84b5d3640f7103dcc8984df7be679967bc06fd44
d1a26b4f46b4f27d01c90a71b9bbd065c2d0efd1
3e64edae781fe8cdec33b68b00f5daa51bb74b5d
Comment 42 Samba QA Contact 2023-10-10 15:18:06 UTC 
This bug was referenced in samba v4-17-test:

23199e115457e8054c905eedee95ebee114809e0
e6f096c4c8fb3e19fff954c2ddd9c329fb86b06a
f958415a69fdd6324810e16a050e25b821ba204b
e5a1c1cfb0a73a37001afee530ae09bf5c58b515
125ce23115b92045a1584f5654669180bea83067
4b3e5c2f036f868e38ad5da7faba05db32f624f4
Comment 43 Samba QA Contact 2023-10-10 15:29:09 UTC 
This bug was referenced in samba v4-18-test:

1688b6d3dd4d6490e10c57d7431cb300f8286e73
fbb9cf8d11840828cbb2ef2249f10fbf766d65ea
682a9a808b4578fef2d984e47d227bc181603cbf
84b5d3640f7103dcc8984df7be679967bc06fd44
d1a26b4f46b4f27d01c90a71b9bbd065c2d0efd1
3e64edae781fe8cdec33b68b00f5daa51bb74b5d
Comment 44 Samba QA Contact 2023-10-10 15:48:12 UTC 
This bug was referenced in samba master:

ae476e1c28b797fe221172ed1066bf8efa476d8d
c39f90a12496fea74a11cbd8b34ad4074d2529db
5ed25efb0731de2062cd1d9e109dcf9e3eb5c356
Comment 45 Samba QA Contact 2023-10-10 15:59:27 UTC 
This bug was referenced in samba v4-19-test:

5dab2cfde7e45b9e366325ed1f75b65fc6e30d90
45d584532f865a39432b9a3f179fdeb5cbef2f76
f17abf9c4a7ce75893a46f90dde5871501b78e86
67c6778534d8fc1f6ce20cfb67d682b6f16ce1b9
44d59c380afbd227243d1dcf65b17cb445357c0f
456a758f10c8163122d1746d40a03df6f3f7b391
Comment 46 Jule Anger 2023-10-20 09:14:01 UTC 
Pushed to all branches.
Closing out bug report.
Thanks!