rpms/sssd
3
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Resolves: upstream#3386 - KCM: Payload buffer is too small

Browse Source 
Related to: rhbz#1494843 - KCM Does not work

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
This commit is contained in:
Fabiano Fidêncio 2018-03-30 14:43:19 +02:00
parent 73735e9522
commit 1c7376afc5
6 changed files with 414 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
0009-KCM-Remove-mem_ctx-from-kcm_new_req.patch Normal file
View File

@ -0,0 +1,50 @@
From 48cff40315cfbfcfae3582935efda961757ceec6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio@redhat.com>
Date: Tue, 13 Mar 2018 21:11:16 +0100
Subject: [PATCH 09/15] KCM: Remove mem_ctx from kcm_new_req()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Let's remove the mem_ctx argument as we really want cctx to be the
memory context here, so that if the client disconnects the request goes
away.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
Reviewed-by: Jakub Hrozek <jhrozek@redhat.com>
---
src/responder/kcm/kcmsrv_cmd.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/responder/kcm/kcmsrv_cmd.c b/src/responder/kcm/kcmsrv_cmd.c
index 0b933f0b4..d4ebb79bf 100644
--- a/src/responder/kcm/kcmsrv_cmd.c
+++ b/src/responder/kcm/kcmsrv_cmd.c
@@ -423,8 +423,10 @@ static errno_t kcm_recv_data(int fd, struct kcm_reqbuf *reqbuf)
return EOK;
}
-static struct kcm_req_ctx *kcm_new_req(TALLOC_CTX *mem_ctx,
- struct cli_ctx *cctx,
+/* Mind that kcm_new_req() does not take a mem_ctx argument on purpose as we
+ * really want the cctx to be the memory context here so that if the client
+ * disconnects, the request goes away. */
+static struct kcm_req_ctx *kcm_new_req(struct cli_ctx *cctx,
struct kcm_ctx *kctx)
{
struct kcm_req_ctx *req;
@@ -467,8 +469,8 @@ static void kcm_recv(struct cli_ctx *cctx)
kctx = talloc_get_type(cctx->rctx->pvt_ctx, struct kcm_ctx);
req = talloc_get_type(cctx->state_ctx, struct kcm_req_ctx);
if (req == NULL) {
- /* A new request comes in, setup data structures */
- req = kcm_new_req(cctx, cctx, kctx);
+ /* A new request comes in, setup data structures. */
+ req = kcm_new_req(cctx, kctx);
if (req == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE,
"Cannot set up client connection\n");
--
2.14.3

61
0010-KCM-Introduce-kcm_input_get_payload_len.patch Normal file
View File

@ -0,0 +1,61 @@
From 7fa69ab8152392b11490950ff8aeeef7e0ad14de Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio@redhat.com>
Date: Tue, 13 Mar 2018 23:13:35 +0100
Subject: [PATCH 10/15] KCM: Introduce kcm_input_get_payload_len()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
As this piece of code will be useful for us in the future patches of
this series, let's move it to a new function.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
Reviewed-by: Jakub Hrozek <jhrozek@redhat.com>
---
src/responder/kcm/kcmsrv_cmd.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/responder/kcm/kcmsrv_cmd.c b/src/responder/kcm/kcmsrv_cmd.c
index d4ebb79bf..3ecba9df2 100644
--- a/src/responder/kcm/kcmsrv_cmd.c
+++ b/src/responder/kcm/kcmsrv_cmd.c
@@ -129,23 +129,27 @@ struct kcm_reqbuf {
struct kcm_iovec v_msg;
};
+static uint32_t kcm_input_get_payload_len(struct kcm_iovec *v)
+{
+ size_t lc = 0;
+ uint32_t len_be = 0;
+
+ /* The first 4 bytes before the payload is message length */
+ SAFEALIGN_COPY_UINT32_CHECK(&len_be, v->kiov_base, v->kiov_len, &lc);
+
+ return be32toh(len_be);
+}
+
static errno_t kcm_input_parse(struct kcm_reqbuf *reqbuf,
struct kcm_op_io *op_io)
{
- size_t lc = 0;
size_t mc = 0;
uint16_t opcode_be = 0;
- uint32_t len_be = 0;
uint32_t msglen;
uint8_t proto_maj = 0;
uint8_t proto_min = 0;
- /* The first 4 bytes before the payload is message length */
- SAFEALIGN_COPY_UINT32_CHECK(&len_be,
- reqbuf->v_len.kiov_base,
- reqbuf->v_len.kiov_len,
- &lc);
- msglen = be32toh(len_be);
+ msglen = kcm_input_get_payload_len(&reqbuf->v_len);
DEBUG(SSSDBG_TRACE_LIBS,
"Received message with length %"PRIu32"\n", msglen);
--
2.14.3

243
0011-KCM-Do-not-use-2048-as-fixed-size-for-the-payload.patch Normal file
View File

@ -0,0 +1,243 @@
From 9f078d2e9ec7e1803b6c7e2f8a51e0e185723e76 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio@redhat.com>
Date: Wed, 14 Mar 2018 00:57:39 +0100
Subject: [PATCH 11/15] KCM: Do not use 2048 as fixed size for the payload
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The KCM code has the limit set as 2048 only inside #ifdef __APPLE__,
while it should be normally set as 10 * 1024 * 1024, as seen in:
https://github.com/krb5/krb5/blob/master/src/lib/krb5/ccache/cc_kcm.c#L53
Last but not least, doesn't make much sense to use a fixed value as the
first 4 bytes received are the payload size ... so let's just allocate
the needed size instead of having a fixed value.
Resolves:
https://pagure.io/SSSD/sssd/issue/3671
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
Reviewed-by: Jakub Hrozek <jhrozek@redhat.com>
---
src/responder/kcm/kcmsrv_cmd.c | 103 +++++++++++++++++++++++++----------------
1 file changed, 62 insertions(+), 41 deletions(-)
diff --git a/src/responder/kcm/kcmsrv_cmd.c b/src/responder/kcm/kcmsrv_cmd.c
index 3ecba9df2..728979da9 100644
--- a/src/responder/kcm/kcmsrv_cmd.c
+++ b/src/responder/kcm/kcmsrv_cmd.c
@@ -38,7 +38,7 @@
/* The maximum length of a request or reply as defined by the RPC
* protocol. This is the same constant size as MIT KRB5 uses
*/
-#define KCM_PACKET_MAX_SIZE 2048
+#define KCM_PACKET_MAX_SIZE 10*1024*1024
/* KCM operation, its raw input and raw output and result */
struct kcm_op_io {
@@ -125,7 +125,6 @@ struct kcm_reqbuf {
struct kcm_iovec v_len;
/* Includes the major, minor versions etc */
- uint8_t msgbuf[KCM_PACKET_MAX_SIZE];
struct kcm_iovec v_msg;
};
@@ -238,7 +237,6 @@ struct kcm_repbuf {
uint8_t rcbuf[KCM_RETCODE_SIZE];
struct kcm_iovec v_rc;
- uint8_t msgbuf[KCM_PACKET_MAX_SIZE];
struct kcm_iovec v_msg;
};
@@ -259,11 +257,13 @@ static errno_t kcm_failbuf_construct(errno_t ret,
/* retcode is 0 if the operation at least ran, non-zero if there
* was some kind of internal KCM error, like input couldn't be parsed
*/
-static errno_t kcm_output_construct(struct kcm_op_io *op_io,
+static errno_t kcm_output_construct(TALLOC_CTX *mem_ctx,
+ struct kcm_op_io *op_io,
struct kcm_repbuf *repbuf)
{
- size_t c;
+ uint8_t *rep;
size_t replen;
+ size_t c;
replen = sss_iobuf_get_len(op_io->reply);
if (replen > KCM_PACKET_MAX_SIZE) {
@@ -281,14 +281,22 @@ static errno_t kcm_output_construct(struct kcm_op_io *op_io,
SAFEALIGN_SETMEM_UINT32(repbuf->rcbuf, 0, &c);
if (replen > 0) {
+ rep = talloc_zero_array(mem_ctx, uint8_t, replen);
+ if (rep == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Failed to allocate memory for the message\n");
+ return ENOMEM;
+ }
+
c = 0;
- SAFEALIGN_MEMCPY_CHECK(repbuf->msgbuf,
+ SAFEALIGN_MEMCPY_CHECK(rep,
sss_iobuf_get_data(op_io->reply),
replen,
- repbuf->v_msg.kiov_len,
+ replen,
&c);
- /* Length of the buffer to send to KCM client */
+ /* Set the buffer and its length to send to KCM client */
+ repbuf->v_msg.kiov_base = rep;
repbuf->v_msg.kiov_len = replen;
}
@@ -321,24 +329,6 @@ static void kcm_reply_error(struct cli_ctx *cctx,
TEVENT_FD_WRITEABLE(cctx->cfde);
}
-static void kcm_send_reply(struct cli_ctx *cctx,
- struct kcm_op_io *op_io,
- struct kcm_repbuf *repbuf)
-{
- errno_t ret;
-
- DEBUG(SSSDBG_TRACE_INTERNAL, "Sending a reply\n");
- ret = kcm_output_construct(op_io, repbuf);
- if (ret != EOK) {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Cannot construct the reply buffer, terminating client\n");
- kcm_reply_error(cctx, ret, repbuf);
- return;
- }
-
- TEVENT_FD_WRITEABLE(cctx->cfde);
-}
-
/**
* Request-reply dispatcher
*/
@@ -356,6 +346,26 @@ struct kcm_req_ctx {
struct kcm_op_io op_io;
};
+static void kcm_send_reply(struct kcm_req_ctx *req_ctx)
+{
+ struct cli_ctx *cctx;
+ errno_t ret;
+
+ DEBUG(SSSDBG_TRACE_INTERNAL, "Sending a reply\n");
+
+ cctx = req_ctx->cctx;
+
+ ret = kcm_output_construct(cctx, &req_ctx->op_io, &req_ctx->repbuf);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Cannot construct the reply buffer, terminating client\n");
+ kcm_reply_error(cctx, ret, &req_ctx->repbuf);
+ return;
+ }
+
+ TEVENT_FD_WRITEABLE(cctx->cfde);
+}
+
static void kcm_cmd_request_done(struct tevent_req *req);
static errno_t kcm_cmd_dispatch(struct kcm_ctx *kctx,
@@ -385,11 +395,9 @@ static errno_t kcm_cmd_dispatch(struct kcm_ctx *kctx,
static void kcm_cmd_request_done(struct tevent_req *req)
{
struct kcm_req_ctx *req_ctx;
- struct cli_ctx *cctx;
errno_t ret;
req_ctx = tevent_req_callback_data(req, struct kcm_req_ctx);
- cctx = req_ctx->cctx;
ret = kcm_cmd_recv(req_ctx, req,
&req_ctx->op_io.reply);
@@ -397,15 +405,19 @@ static void kcm_cmd_request_done(struct tevent_req *req)
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,
"KCM operation failed [%d]: %s\n", ret, sss_strerror(ret));
- kcm_reply_error(cctx, ret, &req_ctx->repbuf);
+ kcm_reply_error(req_ctx->cctx, ret, &req_ctx->repbuf);
return;
}
- kcm_send_reply(cctx, &req_ctx->op_io, &req_ctx->repbuf);
+ kcm_send_reply(req_ctx);
}
-static errno_t kcm_recv_data(int fd, struct kcm_reqbuf *reqbuf)
+static errno_t kcm_recv_data(TALLOC_CTX *mem_ctx,
+ int fd,
+ struct kcm_reqbuf *reqbuf)
{
+ uint8_t *msg;
+ uint32_t msglen;
errno_t ret;
ret = kcm_read_iovec(fd, &reqbuf->v_len);
@@ -416,6 +428,24 @@ static errno_t kcm_recv_data(int fd, struct kcm_reqbuf *reqbuf)
return ret;
}
+ msglen = kcm_input_get_payload_len(&reqbuf->v_len);
+ if (msglen > KCM_PACKET_MAX_SIZE) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Request exceeds the KCM protocol limit, aborting\n");
+ return E2BIG;
+ }
+
+ msg = talloc_zero_array(mem_ctx, uint8_t, msglen);
+ if (msg == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Failed to allocate memory for the message\n");
+ return ENOMEM;
+ }
+
+ /* Set the buffer and its expected len to receive the data */
+ reqbuf->v_msg.kiov_base = msg;
+ reqbuf->v_msg.kiov_len = msglen;
+
ret = kcm_read_iovec(fd, &reqbuf->v_msg);
if (ret != EOK) {
/* Not all errors are fatal, hence we don't print DEBUG messages
@@ -443,21 +473,12 @@ static struct kcm_req_ctx *kcm_new_req(struct cli_ctx *cctx,
req->reqbuf.v_len.kiov_base = req->reqbuf.lenbuf;
req->reqbuf.v_len.kiov_len = KCM_MSG_LEN_SIZE;
- req->reqbuf.v_msg.kiov_base = req->reqbuf.msgbuf;
- req->reqbuf.v_msg.kiov_len = KCM_PACKET_MAX_SIZE;
-
req->repbuf.v_len.kiov_base = req->repbuf.lenbuf;
req->repbuf.v_len.kiov_len = KCM_MSG_LEN_SIZE;
req->repbuf.v_rc.kiov_base = req->repbuf.rcbuf;
req->repbuf.v_rc.kiov_len = KCM_RETCODE_SIZE;
- req->repbuf.v_msg.kiov_base = req->repbuf.msgbuf;
- /* Length of the msg iobuf will be adjusted later, so far use the full
- * length so that constructing the reply can use that capacity
- */
- req->repbuf.v_msg.kiov_len = KCM_PACKET_MAX_SIZE;
-
req->cctx = cctx;
req->kctx = kctx;
@@ -485,7 +506,7 @@ static void kcm_recv(struct cli_ctx *cctx)
cctx->state_ctx = req;
}
- ret = kcm_recv_data(cctx->cfd, &req->reqbuf);
+ ret = kcm_recv_data(req, cctx->cfd, &req->reqbuf);
switch (ret) {
case ENODATA:
DEBUG(SSSDBG_TRACE_ALL, "Client closed connection.\n");
--
2.14.3

55
0012-KCM-Adjust-REPLY_MAX-to-the-one-used-in-krb5.patch Normal file
View File

@ -0,0 +1,55 @@
From d910ef0667a902b4ac0551f3e8d11121bb02214c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio@redhat.com>
Date: Wed, 14 Mar 2018 09:21:45 +0100
Subject: [PATCH 12/15] KCM: Adjust REPLY_MAX to the one used in krb5
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
krb5 has its MAX_REPLY_SIZE set as 10*1024*1024, as seen in:
https://github.com/krb5/krb5/blob/master/src/lib/krb5/ccache/cc_kcm.c#L53
Related:
https://pagure.io/SSSD/sssd/issue/3386
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
Reviewed-by: Jakub Hrozek <jhrozek@redhat.com>
---
src/responder/kcm/kcmsrv_ops.c | 5 ++++-
src/util/tev_curl.c | 3 ++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/responder/kcm/kcmsrv_ops.c b/src/responder/kcm/kcmsrv_ops.c
index 7a78e9d6b..1e229adc4 100644
--- a/src/responder/kcm/kcmsrv_ops.c
+++ b/src/responder/kcm/kcmsrv_ops.c
@@ -31,7 +31,10 @@
#include "responder/kcm/kcmsrv_ops.h"
#include "responder/kcm/kcmsrv_ccache.h"
-#define KCM_REPLY_MAX 16384
+/* This limit comes from:
+ * https://github.com/krb5/krb5/blob/master/src/lib/krb5/ccache/cc_kcm.c#L53
+ */
+#define KCM_REPLY_MAX 10*1024*1024
struct kcm_op_ctx {
struct kcm_resp_ctx *kcm_data;
diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c
index 4c2f1ec9f..f8bede6c5 100644
--- a/src/util/tev_curl.c
+++ b/src/util/tev_curl.c
@@ -35,7 +35,8 @@
#include "util/tev_curl.h"
#define TCURL_IOBUF_CHUNK 1024
-#define TCURL_IOBUF_MAX 16384
+/* This limit in the same one as KCM_REPLY_MAX */
+#define TCURL_IOBUF_MAX 10*1024*1024
static bool global_is_curl_initialized;
--
2.14.3

41
0504-KCM-temporary-increase-hardcoded-buffers.patch
View File

@ -1,41 +0,0 @@
From 3f2845f98ad28e57cf6a2a3ce33ff01d417c4a45 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn@fedoraproject.org>
Date: Tue, 21 Nov 2017 17:48:16 +0100
Subject: [PATCH] KCM: temporary increase hardcoded buffers
Temporary workaround:
https://pagure.io/SSSD/sssd/issue/3386
---
src/responder/kcm/kcmsrv_ops.c | 2 +-
src/util/tev_curl.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/responder/kcm/kcmsrv_ops.c b/src/responder/kcm/kcmsrv_ops.c
index 7a78e9d6b36b4aa3d31ad467216244f733f4a57b..5af567c0d19d347e28cdeada22d15807fb8bc0d5 100644
--- a/src/responder/kcm/kcmsrv_ops.c
+++ b/src/responder/kcm/kcmsrv_ops.c
@@ -31,7 +31,7 @@
#include "responder/kcm/kcmsrv_ops.h"
#include "responder/kcm/kcmsrv_ccache.h"
-#define KCM_REPLY_MAX 16384
+#define KCM_REPLY_MAX 131072
struct kcm_op_ctx {
struct kcm_resp_ctx *kcm_data;
diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c
index 4c2f1ec9ff0127ccfd72010460ed75dad43e9ce3..a51003f4118d4dc0dcb697469b861d277cd1b917 100644
--- a/src/util/tev_curl.c
+++ b/src/util/tev_curl.c
@@ -35,7 +35,7 @@
#include "util/tev_curl.h"
#define TCURL_IOBUF_CHUNK 1024
-#define TCURL_IOBUF_MAX 16384
+#define TCURL_IOBUF_MAX 131072
static bool global_is_curl_initialized;
--
2.15.0

6
sssd.spec
View File

@ -50,10 +50,13 @@ Patch0005: 0005-TESTS-Move-get_call_output-to-util.py.patch
Patch0006: 0006-TESTS-Make-get_call_output-more-flexible-about-the-s.patch
Patch0007: 0007-TESTS-Add-a-basic-test-of-sssctl-domain-list.patch
Patch0008: 0008-KCM-Use-json_loadb-when-dealing-with-sss_iobuf-data.patch
Patch0009: 0009-KCM-Remove-mem_ctx-from-kcm_new_req.patch
Patch0010: 0010-KCM-Introduce-kcm_input_get_payload_len.patch
Patch0011: 0011-KCM-Do-not-use-2048-as-fixed-size-for-the-payload.patch
Patch0012: 0012-KCM-Adjust-REPLY_MAX-to-the-one-used-in-krb5.patch
Patch0502: 0502-SYSTEMD-Use-capabilities.patch
Patch0503: 0503-Disable-stopping-idle-socket-activated-responders.patch
Patch0504: 0504-KCM-temporary-increase-hardcoded-buffers.patch
### Dependencies ###
@ -1257,6 +1260,7 @@ fi
- Resolves: upstream#3658 - Application domain is not interpreted correctly
- Resolves: upstream#3687 - KCM: Don't pass a non null terminated string to
json_loads()
- Resolves: upstream#3386 - KCM: Payload buffer is too small
* Fri Mar 9 2018 Fabiano Fidêncio <fidencio@fedoraproject.org> - 1.16.1-1
- New upstream release 1.16.1