From 37b2a73cd7499a78fc4adb1ffd86b53d829a385b Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Nov 2019 12:14:29 -0800 Subject: [PATCH 1/6] s3: smbd: Allow smbd_smb2_process_negprot() to return NTSTATUS as it can fail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14205 Signed-off-by: Jeremy Allison --- source3/smbd/globals.h | 2 +- source3/smbd/smb2_server.c | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index c791eb0fa6f..a5e02ebc96a 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -234,7 +234,7 @@ NTSTATUS smbd_add_connection(struct smbXsrv_client *client, int sock_fd, void reply_smb2002(struct smb_request *req, uint16_t choice); void reply_smb20ff(struct smb_request *req, uint16_t choice); -void smbd_smb2_process_negprot(struct smbXsrv_connection *xconn, +NTSTATUS smbd_smb2_process_negprot(struct smbXsrv_connection *xconn, uint64_t expected_seq_low, const uint8_t *inpdu, size_t size); diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index 7044ecb991a..9cf8841d827 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -3629,7 +3629,7 @@ static NTSTATUS smbd_smb2_request_next_incoming(struct smbXsrv_connection *xconn return NT_STATUS_OK; } -void smbd_smb2_process_negprot(struct smbXsrv_connection *xconn, +NTSTATUS smbd_smb2_process_negprot(struct smbXsrv_connection *xconn, uint64_t expected_seq_low, const uint8_t *inpdu, size_t size) { @@ -3643,25 +3643,25 @@ void smbd_smb2_process_negprot(struct smbXsrv_connection *xconn, status = smbd_initialize_smb2(xconn, expected_seq_low); if (!NT_STATUS_IS_OK(status)) { smbd_server_connection_terminate(xconn, nt_errstr(status)); - return; + return status; } status = smbd_smb2_request_create(xconn, inpdu, size, &req); if (!NT_STATUS_IS_OK(status)) { smbd_server_connection_terminate(xconn, nt_errstr(status)); - return; + return status; } status = smbd_smb2_request_validate(req); if (!NT_STATUS_IS_OK(status)) { smbd_server_connection_terminate(xconn, nt_errstr(status)); - return; + return status; } status = smbd_smb2_request_setup_out(req); if (!NT_STATUS_IS_OK(status)) { smbd_server_connection_terminate(xconn, nt_errstr(status)); - return; + return status; } #ifdef WITH_PROFILE @@ -3676,16 +3676,17 @@ void smbd_smb2_process_negprot(struct smbXsrv_connection *xconn, status = smbd_smb2_request_dispatch(req); if (!NT_STATUS_IS_OK(status)) { smbd_server_connection_terminate(xconn, nt_errstr(status)); - return; + return status; } status = smbd_smb2_request_next_incoming(xconn); if (!NT_STATUS_IS_OK(status)) { smbd_server_connection_terminate(xconn, nt_errstr(status)); - return; + return status; } sconn->num_requests++; + return NT_STATUS_OK; } static int socket_error_from_errno(int ret, -- 2.24.0.432.g9d3f5f5b63-goog From 05400c24099697959763ff584b5d54e7a66a2a15 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Nov 2019 12:17:29 -0800 Subject: [PATCH 2/6] s3: smbd: Ensure we exit on smbd_smb2_process_negprot() fail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14205 Signed-off-by: Jeremy Allison --- source3/smbd/process.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/source3/smbd/process.c b/source3/smbd/process.c index 0e0d45d2af1..fc43519a614 100644 --- a/source3/smbd/process.c +++ b/source3/smbd/process.c @@ -1968,7 +1968,14 @@ static void process_smb(struct smbXsrv_connection *xconn, if (smbd_is_smb2_header(inbuf, nread)) { const uint8_t *inpdu = inbuf + NBT_HDR_SIZE; size_t pdulen = nread - NBT_HDR_SIZE; - smbd_smb2_process_negprot(xconn, 0, inpdu, pdulen); + NTSTATUS status = smbd_smb2_process_negprot( + xconn, + 0, + inpdu, + pdulen); + if (!NT_STATUS_IS_OK(status)) { + exit_server_cleanly("SMB2 negprot fail"); + } return; } if (nread >= smb_size && valid_smb_header(inbuf) -- 2.24.0.432.g9d3f5f5b63-goog From 265399109ac0f0a46e8f7da41b6cbd59e5467d48 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Nov 2019 12:21:06 -0800 Subject: [PATCH 3/6] s3: smbd: Change reply_smb20xx() to return NTSTATUS. Not yet used. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14205 Signed-off-by: Jeremy Allison --- source3/smbd/smb2_negprot.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source3/smbd/smb2_negprot.c b/source3/smbd/smb2_negprot.c index 6e7201b1cd8..b07fc972cbb 100644 --- a/source3/smbd/smb2_negprot.c +++ b/source3/smbd/smb2_negprot.c @@ -36,7 +36,7 @@ extern fstring remote_proto; * this is the entry point if SMB2 is selected via * the SMB negprot and the given dialect. */ -static void reply_smb20xx(struct smb_request *req, uint16_t dialect) +static NTSTATUS reply_smb20xx(struct smb_request *req, uint16_t dialect) { uint8_t *smb2_inpdu; uint8_t *smb2_hdr; @@ -48,7 +48,7 @@ static void reply_smb20xx(struct smb_request *req, uint16_t dialect) if (smb2_inpdu == NULL) { DEBUG(0, ("Could not push spnego blob\n")); reply_nterror(req, NT_STATUS_NO_MEMORY); - return; + return NT_STATUS_NO_MEMORY; } smb2_hdr = smb2_inpdu; smb2_body = smb2_hdr + SMB2_HDR_BODY; @@ -64,8 +64,7 @@ static void reply_smb20xx(struct smb_request *req, uint16_t dialect) req->outbuf = NULL; - smbd_smb2_process_negprot(req->xconn, 0, smb2_inpdu, len); - return; + return smbd_smb2_process_negprot(req->xconn, 0, smb2_inpdu, len); } /* -- 2.24.0.432.g9d3f5f5b63-goog From b9be8393a9ee134d3f08770b92a712e677664a9e Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Nov 2019 12:43:25 -0800 Subject: [PATCH 4/6] s3: smbd: Change (*proto_reply_fn()) to return an NTSTATUS. That way the caller can know if the negprot really succeeded or not. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14205 Signed-off-by: Jeremy Allison --- source3/smbd/globals.h | 4 ++-- source3/smbd/negprot.c | 29 +++++++++++++++-------------- source3/smbd/smb2_negprot.c | 8 ++++---- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index a5e02ebc96a..e6641837668 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -232,8 +232,8 @@ bool smbd_smb2_is_compound(const struct smbd_smb2_request *req); NTSTATUS smbd_add_connection(struct smbXsrv_client *client, int sock_fd, struct smbXsrv_connection **_xconn); -void reply_smb2002(struct smb_request *req, uint16_t choice); -void reply_smb20ff(struct smb_request *req, uint16_t choice); +NTSTATUS reply_smb2002(struct smb_request *req, uint16_t choice); +NTSTATUS reply_smb20ff(struct smb_request *req, uint16_t choice); NTSTATUS smbd_smb2_process_negprot(struct smbXsrv_connection *xconn, uint64_t expected_seq_low, const uint8_t *inpdu, size_t size); diff --git a/source3/smbd/negprot.c b/source3/smbd/negprot.c index 2d5edf1282c..3b2555e3d21 100644 --- a/source3/smbd/negprot.c +++ b/source3/smbd/negprot.c @@ -66,7 +66,7 @@ static void get_challenge(struct smbXsrv_connection *xconn, uint8_t buff[8]) Reply for the lanman 1.0 protocol. ****************************************************************************/ -static void reply_lanman1(struct smb_request *req, uint16_t choice) +static NTSTATUS reply_lanman1(struct smb_request *req, uint16_t choice) { int secword=0; time_t t = time(NULL); @@ -100,7 +100,7 @@ static void reply_lanman1(struct smb_request *req, uint16_t choice) status = smbXsrv_connection_init_tables(xconn, PROTOCOL_LANMAN1); if (!NT_STATUS_IS_OK(status)) { reply_nterror(req, status); - return; + return status; } /* Reply, SMBlockread, SMBwritelock supported. */ @@ -115,14 +115,14 @@ static void reply_lanman1(struct smb_request *req, uint16_t choice) srv_put_dos_date((char *)req->outbuf,smb_vwv8,t); - return; + return NT_STATUS_OK; } /**************************************************************************** Reply for the lanman 2.0 protocol. ****************************************************************************/ -static void reply_lanman2(struct smb_request *req, uint16_t choice) +static NTSTATUS reply_lanman2(struct smb_request *req, uint16_t choice) { int secword=0; time_t t = time(NULL); @@ -158,7 +158,7 @@ static void reply_lanman2(struct smb_request *req, uint16_t choice) status = smbXsrv_connection_init_tables(xconn, PROTOCOL_LANMAN2); if (!NT_STATUS_IS_OK(status)) { reply_nterror(req, status); - return; + return status; } /* Reply, SMBlockread, SMBwritelock supported. */ @@ -169,6 +169,7 @@ static void reply_lanman2(struct smb_request *req, uint16_t choice) SSVAL(req->outbuf,smb_vwv5,raw); /* readbraw and/or writebraw */ SSVAL(req->outbuf,smb_vwv10, set_server_zone_offset(t)/60); srv_put_dos_date((char *)req->outbuf,smb_vwv8,t); + return NT_STATUS_OK; } /**************************************************************************** @@ -266,7 +267,7 @@ DATA_BLOB negprot_spnego(TALLOC_CTX *ctx, struct smbXsrv_connection *xconn) Reply for the nt protocol. ****************************************************************************/ -static void reply_nt1(struct smb_request *req, uint16_t choice) +static NTSTATUS reply_nt1(struct smb_request *req, uint16_t choice) { /* dual names + lock_and_read + nt SMBs + remote API calls */ int capabilities = CAP_NT_FIND|CAP_LOCK_AND_READ| @@ -359,7 +360,7 @@ static void reply_nt1(struct smb_request *req, uint16_t choice) status = smbXsrv_connection_init_tables(xconn, PROTOCOL_NT1); if (!NT_STATUS_IS_OK(status)) { reply_nterror(req, status); - return; + return status; } SSVAL(req->outbuf,smb_vwv1+1, lp_max_mux()); /* maxmpx */ @@ -385,7 +386,7 @@ static void reply_nt1(struct smb_request *req, uint16_t choice) if (ret == -1) { DEBUG(0, ("Could not push challenge\n")); reply_nterror(req, NT_STATUS_NO_MEMORY); - return; + return NT_STATUS_NO_MEMORY; } SCVAL(req->outbuf, smb_vwv16+1, ret); } @@ -395,7 +396,7 @@ static void reply_nt1(struct smb_request *req, uint16_t choice) if (ret == -1) { DEBUG(0, ("Could not push workgroup string\n")); reply_nterror(req, NT_STATUS_NO_MEMORY); - return; + return NT_STATUS_NO_MEMORY; } ret = message_push_string(&req->outbuf, lp_netbios_name(), STR_UNICODE|STR_TERMINATE @@ -403,7 +404,7 @@ static void reply_nt1(struct smb_request *req, uint16_t choice) if (ret == -1) { DEBUG(0, ("Could not push netbios name string\n")); reply_nterror(req, NT_STATUS_NO_MEMORY); - return; + return NT_STATUS_NO_MEMORY; } DEBUG(3,("not using SPNEGO\n")); } else { @@ -411,14 +412,14 @@ static void reply_nt1(struct smb_request *req, uint16_t choice) if (spnego_blob.data == NULL) { reply_nterror(req, NT_STATUS_NO_MEMORY); - return; + return NT_STATUS_NO_MEMORY; } ret = message_push_blob(&req->outbuf, spnego_blob); if (ret == -1) { DEBUG(0, ("Could not push spnego blob\n")); reply_nterror(req, NT_STATUS_NO_MEMORY); - return; + return NT_STATUS_NO_MEMORY; } data_blob_free(&spnego_blob); @@ -426,7 +427,7 @@ static void reply_nt1(struct smb_request *req, uint16_t choice) DEBUG(3,("using SPNEGO\n")); } - return; + return NT_STATUS_OK; } /* these are the protocol lists used for auto architecture detection: @@ -540,7 +541,7 @@ protocol [SMB 2.???] static const struct { const char *proto_name; const char *short_name; - void (*proto_reply_fn)(struct smb_request *req, uint16_t choice); + NTSTATUS (*proto_reply_fn)(struct smb_request *req, uint16_t choice); int protocol_level; } supported_protocols[] = { {"SMB 2.???", "SMB2_FF", reply_smb20ff, PROTOCOL_SMB2_10}, diff --git a/source3/smbd/smb2_negprot.c b/source3/smbd/smb2_negprot.c index b07fc972cbb..2c1d4185b28 100644 --- a/source3/smbd/smb2_negprot.c +++ b/source3/smbd/smb2_negprot.c @@ -71,20 +71,20 @@ static NTSTATUS reply_smb20xx(struct smb_request *req, uint16_t dialect) * this is the entry point if SMB2 is selected via * the SMB negprot and the "SMB 2.002" dialect. */ -void reply_smb2002(struct smb_request *req, uint16_t choice) +NTSTATUS reply_smb2002(struct smb_request *req, uint16_t choice) { - reply_smb20xx(req, SMB2_DIALECT_REVISION_202); + return reply_smb20xx(req, SMB2_DIALECT_REVISION_202); } /* * this is the entry point if SMB2 is selected via * the SMB negprot and the "SMB 2.???" dialect. */ -void reply_smb20ff(struct smb_request *req, uint16_t choice) +NTSTATUS reply_smb20ff(struct smb_request *req, uint16_t choice) { struct smbXsrv_connection *xconn = req->xconn; xconn->smb2.allow_2ff = true; - reply_smb20xx(req, SMB2_DIALECT_REVISION_2FF); + return reply_smb20xx(req, SMB2_DIALECT_REVISION_2FF); } enum protocol_types smbd_smb2_protocol_dialect_match(const uint8_t *indyn, -- 2.24.0.432.g9d3f5f5b63-goog From f4a3443ab5e9f0a90931b7c67a8d5fc2956d4204 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Nov 2019 12:46:16 -0800 Subject: [PATCH 5/6] s3: smbd: Ensure we exit if supported_protocols[protocol].proto_reply_fn() fails. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14205 Signed-off-by: Jeremy Allison --- source3/smbd/negprot.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/source3/smbd/negprot.c b/source3/smbd/negprot.c index 3b2555e3d21..8317dc49086 100644 --- a/source3/smbd/negprot.c +++ b/source3/smbd/negprot.c @@ -580,6 +580,7 @@ void reply_negprot(struct smb_request *req) bool signing_required = true; int max_proto; int min_proto; + NTSTATUS status; START_PROFILE(SMBnegprot); @@ -768,7 +769,11 @@ void reply_negprot(struct smb_request *req) fstrcpy(remote_proto,supported_protocols[protocol].short_name); reload_services(sconn, conn_snum_used, true); - supported_protocols[protocol].proto_reply_fn(req, choice); + status = supported_protocols[protocol].proto_reply_fn(req, choice); + if (!NT_STATUS_IS_OK(status)) { + exit_server_cleanly("negprot function failed\n"); + } + DEBUG(3,("Selected protocol %s\n",supported_protocols[protocol].proto_name)); DBG_INFO("negprot index=%zu\n", choice); -- 2.24.0.432.g9d3f5f5b63-goog From 1d2f9adaca98f71a81050a723dde3798959a4fc1 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Nov 2019 12:53:09 -0800 Subject: [PATCH 6/6] s3: smbd: Only set xconn->smb1.negprot.done = true after supported_protocols[protocol].proto_reply_fn() succeeds. Otherwise we can end up with negprot.done set, but without smbXsrv_connection_init_tables() being called. This can cause a client self-crash. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14205 Signed-off-by: Jeremy Allison --- source3/smbd/negprot.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source3/smbd/negprot.c b/source3/smbd/negprot.c index 8317dc49086..e77c8f52261 100644 --- a/source3/smbd/negprot.c +++ b/source3/smbd/negprot.c @@ -588,7 +588,6 @@ void reply_negprot(struct smb_request *req) END_PROFILE(SMBnegprot); exit_server_cleanly("multiple negprot's are not permitted"); } - xconn->smb1.negprot.done = true; if (req->buflen == 0) { DEBUG(0, ("negprot got no protocols\n")); @@ -778,6 +777,8 @@ void reply_negprot(struct smb_request *req) DBG_INFO("negprot index=%zu\n", choice); + xconn->smb1.negprot.done = true; + /* We always have xconn->smb1.signing_state also for >= SMB2_02 */ signing_required = smb_signing_is_mandatory(xconn->smb1.signing_state); if (signing_required && (chosen_level < PROTOCOL_NT1)) { -- 2.24.0.432.g9d3f5f5b63-goog