[PATCH] Make reading registry.tdb a bit more robust
Jeremy Allison
jra at samba.org
Mon Oct 8 18:06:11 UTC 2018
On Thu, Oct 04, 2018 at 05:55:04PM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
>
> A customer sent me an accidentially corrupted registry.tdb under which
> smbd crashed. Add some safety measures for this crash. As with commit
> df2a036377ad68a999cbcc this is not seen as a security issue: If you
> can write the registry.tdb, you're root anyway.
>
> Review appreciated!
>
> Thanks, Volker
>
> https://gitlab.com/samba-team/devel/samba/pipelines/31985971
RB+, LGTM and pushed, thanks !
Jeremy.
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:kontakt at sernet.de
> From e334a1409344a6721ab37f0ac7468b024e5a96b0 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Thu, 4 Oct 2018 10:57:47 +0200
> Subject: [PATCH 1/8] tdb_unpack: Convert to size_t for internal calculations
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/util_tdb.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/source3/lib/util_tdb.c b/source3/lib/util_tdb.c
> index 80e66e0a108..9f39fcb4df3 100644
> --- a/source3/lib/util_tdb.c
> +++ b/source3/lib/util_tdb.c
> @@ -145,20 +145,20 @@ size_t tdb_pack(uint8_t *buf, int bufsize, const char *fmt, ...)
> integers and strings.
> ****************************************************************************/
>
> -int tdb_unpack(const uint8_t *buf, int bufsize, const char *fmt, ...)
> +int tdb_unpack(const uint8_t *buf, int in_bufsize, const char *fmt, ...)
> {
> va_list ap;
> uint8_t *bt;
> uint16_t *w;
> uint32_t *d;
> - int len;
> + size_t bufsize = in_bufsize;
> + size_t len;
> int *i;
> void **p;
> char *s, **b, **ps;
> char c;
> const uint8_t *buf0 = buf;
> const char *fmt0 = fmt;
> - int bufsize0 = bufsize;
>
> va_start(ap, fmt);
>
> @@ -249,7 +249,7 @@ int tdb_unpack(const uint8_t *buf, int bufsize, const char *fmt, ...)
> va_end(ap);
>
> DEBUG(18,("tdb_unpack(%s, %d) -> %d\n",
> - fmt0, bufsize0, (int)PTR_DIFF(buf, buf0)));
> + fmt0, in_bufsize, (int)PTR_DIFF(buf, buf0)));
>
> return PTR_DIFF(buf, buf0);
>
> --
> 2.17.1
>
>
> From babf7f1e296ec92aa727374c30f531cbef9fd7b1 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Thu, 4 Oct 2018 11:05:46 +0200
> Subject: [PATCH 2/8] tdb_unpack: Correct "len" arg for "B" format
>
> All but one of the users of the "B" format specifier passed in a pointer
> to uint32_t instead of what tdb_unpack expected, an "int". Because this
> is a purely internal API, change the tdb_unpack function and adjust that
> one caller.
>
> To reviewers: Please check carefully, thanks :-)
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/util_tdb.c | 4 ++--
> source3/printing/printing.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/source3/lib/util_tdb.c b/source3/lib/util_tdb.c
> index 9f39fcb4df3..035daf8b7b7 100644
> --- a/source3/lib/util_tdb.c
> +++ b/source3/lib/util_tdb.c
> @@ -153,7 +153,7 @@ int tdb_unpack(const uint8_t *buf, int in_bufsize, const char *fmt, ...)
> uint32_t *d;
> size_t bufsize = in_bufsize;
> size_t len;
> - int *i;
> + uint32_t *i;
> void **p;
> char *s, **b, **ps;
> char c;
> @@ -216,7 +216,7 @@ int tdb_unpack(const uint8_t *buf, int in_bufsize, const char *fmt, ...)
> memcpy(s, buf, len);
> break;
> case 'B': /* fixed-length string */
> - i = va_arg(ap, int *);
> + i = va_arg(ap, uint32_t *);
> b = va_arg(ap, char **);
> len = 4;
> if (bufsize < len)
> diff --git a/source3/printing/printing.c b/source3/printing/printing.c
> index 105166ddf7d..e6caaa1222f 100644
> --- a/source3/printing/printing.c
> +++ b/source3/printing/printing.c
> @@ -352,7 +352,7 @@ static int unpack_devicemode(TALLOC_CTX *mem_ctx,
> struct spoolss_DeviceMode *dm;
> enum ndr_err_code ndr_err;
> char *data = NULL;
> - int data_len = 0;
> + uint32_t data_len = 0;
> DATA_BLOB blob;
> int len = 0;
>
> --
> 2.17.1
>
>
> From 526340df692040443c7bfcbbb19a5437d37a2d76 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Thu, 4 Oct 2018 11:07:21 +0200
> Subject: [PATCH 3/8] tdb_unpack: Protect against overflow
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/util_tdb.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/source3/lib/util_tdb.c b/source3/lib/util_tdb.c
> index 035daf8b7b7..9730a030c10 100644
> --- a/source3/lib/util_tdb.c
> +++ b/source3/lib/util_tdb.c
> @@ -227,6 +227,9 @@ int tdb_unpack(const uint8_t *buf, int in_bufsize, const char *fmt, ...)
> break;
> }
> len += *i;
> + if (len < *i) {
> + goto no_space;
> + }
> if (bufsize < len)
> goto no_space;
> *b = (char *)SMB_MALLOC(*i);
> --
> 2.17.1
>
>
> From 09522600c32be041709a75bcc7739dc713695306 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 2 Oct 2018 12:00:30 +0200
> Subject: [PATCH 4/8] registry: Fix a typo
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/registry/reg_objects.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/registry/reg_objects.c b/source3/registry/reg_objects.c
> index edafc920b11..f61822b5919 100644
> --- a/source3/registry/reg_objects.c
> +++ b/source3/registry/reg_objects.c
> @@ -484,7 +484,7 @@ int regval_ctr_addvalue(struct regval_ctr *ctr, const char *name, uint32_t type,
> return 0;
> }
>
> - /* allocate a new value and store the pointer in the arrya */
> + /* allocate a new value and store the pointer in the array */
>
> ctr->values[ctr->num_values] = regval_compose(ctr, name, type, data_p,
> size);
> --
> 2.17.1
>
>
> From a70ef20713d9fb8bbb66c0d6b571e1336cb08359 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 2 Oct 2018 13:16:04 +0200
> Subject: [PATCH 5/8] registry: Add error checks to regdb_fetch_keys_internal
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/registry/reg_backend_db.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/source3/registry/reg_backend_db.c b/source3/registry/reg_backend_db.c
> index aa97d60abec..a0db5eab9ce 100644
> --- a/source3/registry/reg_backend_db.c
> +++ b/source3/registry/reg_backend_db.c
> @@ -1784,7 +1784,23 @@ static WERROR regdb_fetch_keys_internal(struct db_context *db, const char *key,
> }
>
> for (i=0; i<num_items; i++) {
> - len += tdb_unpack(buf+len, buflen-len, "f", subkeyname);
> + int this_len;
> +
> + this_len = tdb_unpack(buf+len, buflen-len, "f", subkeyname);
> + if (this_len == -1) {
> + DBG_WARNING("Invalid registry data, "
> + "tdb_unpack failed\n");
> + werr = WERR_INTERNAL_DB_CORRUPTION;
> + goto done;
> + }
> + len += this_len;
> + if (len < this_len) {
> + DBG_WARNING("Invalid registry data, "
> + "integer overflow\n");
> + werr = WERR_INTERNAL_DB_CORRUPTION;
> + goto done;
> + }
> +
> werr = regsubkey_ctr_addkey(ctr, subkeyname);
> if (!W_ERROR_IS_OK(werr)) {
> DEBUG(5, ("regdb_fetch_keys: regsubkey_ctr_addkey "
> --
> 2.17.1
>
>
> From 4fc422a1068d14025cffeee4ca8b62cb729776fb Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 2 Oct 2018 13:16:23 +0200
> Subject: [PATCH 6/8] registry: Add error checks to regdb_unpack_values
>
> This makes "regdb_unpack_values" take a size_t as buflen. The only
> caller calls it with TDB_DATA.dsize, which *is* size_t. Convert the
> internal "len" variable to the unsigned size_t as well and add overflow
> checks. This depends on tdb_unpack to either return -1 or a positive
> value less than or equal to the passed-in "size_t" buflen;
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/registry/reg_backend_db.c | 36 ++++++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/source3/registry/reg_backend_db.c b/source3/registry/reg_backend_db.c
> index a0db5eab9ce..32189101e31 100644
> --- a/source3/registry/reg_backend_db.c
> +++ b/source3/registry/reg_backend_db.c
> @@ -1833,9 +1833,12 @@ static int regdb_fetch_keys(const char *key, struct regsubkey_ctr *ctr)
> Unpack a list of registry values frem the TDB
> ***************************************************************************/
>
> -static int regdb_unpack_values(struct regval_ctr *values, uint8_t *buf, int buflen)
> +static int regdb_unpack_values(struct regval_ctr *values,
> + uint8_t *buf,
> + size_t buflen)
> {
> - int len = 0;
> + int this_len;
> + size_t len = 0;
> uint32_t type;
> fstring valuename;
> uint32_t size;
> @@ -1845,7 +1848,13 @@ static int regdb_unpack_values(struct regval_ctr *values, uint8_t *buf, int bufl
>
> /* loop and unpack the rest of the registry values */
>
> - len += tdb_unpack(buf+len, buflen-len, "d", &num_values);
> + this_len = tdb_unpack(buf, buflen, "d", &num_values);
> + if (this_len == -1) {
> + DBG_WARNING("Invalid registry data, "
> + "tdb_unpack failed\n");
> + return -1;
> + }
> + len = this_len;
>
> for ( i=0; i<num_values; i++ ) {
> /* unpack the next regval */
> @@ -1854,11 +1863,22 @@ static int regdb_unpack_values(struct regval_ctr *values, uint8_t *buf, int bufl
> size = 0;
> data_p = NULL;
> valuename[0] = '\0';
> - len += tdb_unpack(buf+len, buflen-len, "fdB",
> - valuename,
> - &type,
> - &size,
> - &data_p);
> + this_len = tdb_unpack(buf+len, buflen-len, "fdB",
> + valuename,
> + &type,
> + &size,
> + &data_p);
> + if (this_len == -1) {
> + DBG_WARNING("Invalid registry data, "
> + "tdb_unpack failed\n");
> + return -1;
> + }
> + len += this_len;
> + if (len < (size_t)this_len) {
> + DBG_WARNING("Invalid registry data, "
> + "integer overflow\n");
> + return -1;
> + }
>
> regval_ctr_addvalue(values, valuename, type,
> (uint8_t *)data_p, size);
> --
> 2.17.1
>
>
> From 5ea944571880d9f4eada4e67d523192ae0653fe1 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 2 Oct 2018 12:10:01 +0200
> Subject: [PATCH 7/8] registry: Print failure of regdb_unpack_values
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/registry/reg_backend_db.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/source3/registry/reg_backend_db.c b/source3/registry/reg_backend_db.c
> index 32189101e31..d87a3283baa 100644
> --- a/source3/registry/reg_backend_db.c
> +++ b/source3/registry/reg_backend_db.c
> @@ -1981,7 +1981,11 @@ static int regdb_fetch_values_internal(struct db_context *db, const char* key,
> goto done;
> }
>
> - regdb_unpack_values(values, value.dptr, value.dsize);
> + ret = regdb_unpack_values(values, value.dptr, value.dsize);
> + if (ret == -1) {
> + DBG_WARNING("regdb_unpack_values failed\n");
> + }
> +
> ret = regval_ctr_numvals(values);
>
> done:
> --
> 2.17.1
>
>
> From f6ea36369ecfe481ef148329e30c53a03e31b30f Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Thu, 4 Oct 2018 11:59:43 +0200
> Subject: [PATCH 8/8] registry: Don't use an uninitialized value
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/registry/reg_backend_db.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/source3/registry/reg_backend_db.c b/source3/registry/reg_backend_db.c
> index d87a3283baa..7749b812c24 100644
> --- a/source3/registry/reg_backend_db.c
> +++ b/source3/registry/reg_backend_db.c
> @@ -772,9 +772,9 @@ WERROR regdb_init(void)
> status = dbwrap_fetch_int32_bystring(regdb, REGDB_VERSION_KEYNAME,
> &vers_id);
> if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(10, ("regdb_init: registry version uninitialized "
> - "(got %d), initializing to version %d\n",
> - vers_id, REGDB_VERSION_V1));
> + DBG_DEBUG("Reading registry version failed: %s, "
> + "initializing to version %d\n",
> + nt_errstr(status), REGDB_VERSION_V1);
>
> /*
> * There was a regdb format version prior to version 1
> --
> 2.17.1
>
More information about the samba-technical
mailing list