[PATCH] Make reading registry.tdb a bit more robust
Volker Lendecke
Volker.Lendecke at SerNet.DE
Thu Oct 4 15:55:04 UTC 2018
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
--
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
-------------- next part --------------
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