[PATCH] harden libtdb against corrupt databases
Volker Lendecke
Volker.Lendecke at SerNet.DE
Tue Mar 20 16:31:08 UTC 2018
Hi!
Attached find a patchset that hardens libtdb against corrupt
databases. This was triggered by crashreports found by Lionel Debroux
<lionel_debroux at yahoo.fr> running fuzzers against tdb.
We (Samba Team) agreed that they are mere crash bugs and not security
problems, as we trust our databases in many ways anyway.
Review appreciated!
Thanks, Volker
--
Besuchen Sie die verinice.XP 2018 in Berlin,
Anwenderkonferenz für Informationssicherheit
vom 21.-23.03.2018 im Sofitel Kurfürstendamm
Info & Anmeldung hier: http://veriniceXP.org
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 71a74e2c09ffc9f79ec3642bf3e52ccf30583f14 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 4 Mar 2018 10:07:09 +0100
Subject: [PATCH 1/8] tdbdump: Avoid an int cast
---
lib/tdb/tools/tdbdump.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/tdb/tools/tdbdump.c b/lib/tdb/tools/tdbdump.c
index 8cf3146c4b6..2c68970660b 100644
--- a/lib/tdb/tools/tdbdump.c
+++ b/lib/tdb/tools/tdbdump.c
@@ -41,10 +41,10 @@ static void print_data(TDB_DATA d)
static int traverse_fn(TDB_CONTEXT *tdb, TDB_DATA key, TDB_DATA dbuf, void *state)
{
printf("{\n");
- printf("key(%d) = \"", (int)key.dsize);
+ printf("key(%zu) = \"", key.dsize);
print_data(key);
printf("\"\n");
- printf("data(%d) = \"", (int)dbuf.dsize);
+ printf("data(%zu) = \"", dbuf.dsize);
print_data(dbuf);
printf("\"\n");
printf("}\n");
--
2.11.0
From 6f7ca41a64d79c75083eddf54fabb1135f26e4c4 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 4 Mar 2018 10:07:29 +0100
Subject: [PATCH 2/8] tdb: Harden tdb_rec_read
---
lib/tdb/common/io.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/lib/tdb/common/io.c b/lib/tdb/common/io.c
index 15ba5b7d497..94b316331c1 100644
--- a/lib/tdb/common/io.c
+++ b/lib/tdb/common/io.c
@@ -733,6 +733,9 @@ int tdb_parse_data(struct tdb_context *tdb, TDB_DATA key,
/* read/write a record */
int tdb_rec_read(struct tdb_context *tdb, tdb_off_t offset, struct tdb_record *rec)
{
+ int ret;
+ tdb_len_t overall_len;
+
if (tdb->methods->tdb_read(tdb, offset, rec, sizeof(*rec),DOCONV()) == -1)
return -1;
if (TDB_BAD_MAGIC(rec)) {
@@ -741,6 +744,31 @@ int tdb_rec_read(struct tdb_context *tdb, tdb_off_t offset, struct tdb_record *r
TDB_LOG((tdb, TDB_DEBUG_FATAL,"tdb_rec_read bad magic 0x%x at offset=%u\n", rec->magic, offset));
return -1;
}
+
+ overall_len = rec->key_len + rec->data_len;
+ if (overall_len < rec->data_len) {
+ /* overflow */
+ return -1;
+ }
+
+ if (overall_len > rec->rec_len) {
+ /* invalid record */
+ return -1;
+ }
+
+ ret = tdb->methods->tdb_oob(tdb, offset, rec->key_len, 1);
+ if (ret == -1) {
+ return -1;
+ }
+ ret = tdb->methods->tdb_oob(tdb, offset, rec->data_len, 1);
+ if (ret == -1) {
+ return -1;
+ }
+ ret = tdb->methods->tdb_oob(tdb, offset, rec->rec_len, 1);
+ if (ret == -1) {
+ return -1;
+ }
+
return tdb->methods->tdb_oob(tdb, rec->next, sizeof(*rec), 0);
}
--
2.11.0
From 6628307f9c9fc2a4060b7680d14c9265d743ca2c Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 4 Mar 2018 10:21:09 +0100
Subject: [PATCH 3/8] tdb: Handle TDB_NEXT_LOCK_ERR in tdb_traverse_internal
---
lib/tdb/common/traverse.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/lib/tdb/common/traverse.c b/lib/tdb/common/traverse.c
index 9b833795959..7a1d567cc01 100644
--- a/lib/tdb/common/traverse.c
+++ b/lib/tdb/common/traverse.c
@@ -166,9 +166,16 @@ static int tdb_traverse_internal(struct tdb_context *tdb,
/* tdb_next_lock places locks on the record returned, and its chain */
while ((off = tdb_next_lock(tdb, tl, &rec)) != 0) {
- tdb_len_t full_len = rec.key_len + rec.data_len;
+ tdb_len_t full_len;
int nread;
+ if (off == TDB_NEXT_LOCK_ERR) {
+ ret = -1;
+ goto out;
+ }
+
+ full_len = rec.key_len + rec.data_len;
+
if (full_len > recbuf_len) {
recbuf_len = full_len;
@@ -195,10 +202,6 @@ static int tdb_traverse_internal(struct tdb_context *tdb,
}
}
- if (off == TDB_NEXT_LOCK_ERR) {
- ret = -1;
- goto out;
- }
count++;
/* now read the full record */
nread = tdb->methods->tdb_read(tdb, tl->off + sizeof(rec),
--
2.11.0
From 1a64b418b774fa78b946739ee5fbb52b23ed668e Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 4 Mar 2018 10:46:09 +0100
Subject: [PATCH 4/8] Harden tdb_check_used_record against overflow
---
lib/tdb/common/check.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/lib/tdb/common/check.c b/lib/tdb/common/check.c
index e632af51536..3a5c8b8ba94 100644
--- a/lib/tdb/common/check.c
+++ b/lib/tdb/common/check.c
@@ -242,12 +242,27 @@ static bool tdb_check_used_record(struct tdb_context *tdb,
void *private_data)
{
TDB_DATA key, data;
+ tdb_len_t len;
if (!tdb_check_record(tdb, off, rec))
return false;
/* key + data + tailer must fit in record */
- if (rec->key_len + rec->data_len + sizeof(tdb_off_t) > rec->rec_len) {
+ len = rec->key_len;
+ len += rec->data_len;
+ if (len < rec->data_len) {
+ /* overflow */
+ TDB_LOG((tdb, TDB_DEBUG_ERROR, "Record lengths overflow\n"));
+ return false;
+ }
+ len += sizeof(tdb_off_t);
+ if (len < sizeof(tdb_off_t)) {
+ /* overflow */
+ TDB_LOG((tdb, TDB_DEBUG_ERROR, "Record lengths overflow\n"));
+ return false;
+ }
+
+ if (len > rec->rec_len) {
TDB_LOG((tdb, TDB_DEBUG_ERROR,
"Record offset %u too short for contents\n", off));
return false;
--
2.11.0
From e9d0f5149c054fdca8456aa61b50a8b78172fa92 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 4 Mar 2018 11:09:10 +0100
Subject: [PATCH 5/8] tdb: Make sure the hash size fits
---
lib/tdb/common/open.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/lib/tdb/common/open.c b/lib/tdb/common/open.c
index cfb476d2385..8baa7e40de0 100644
--- a/lib/tdb/common/open.c
+++ b/lib/tdb/common/open.c
@@ -687,6 +687,21 @@ _PUBLIC_ struct tdb_context *tdb_open_ex(const char *name, int hash_size, int td
}
}
+ if (tdb->hash_size > UINT32_MAX/4) {
+ TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
+ "hash size %"PRIu32" too large\n", tdb->hash_size));
+ errno = EINVAL;
+ goto fail;
+ }
+
+ ret = tdb->methods->tdb_oob(tdb, FREELIST_TOP, 4*tdb->hash_size, 1);
+ if (ret == -1) {
+ TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
+ "hash size %"PRIu32" does not fit\n", tdb->hash_size));
+ errno = EINVAL;
+ goto fail;
+ }
+
if (locked) {
if (tdb_nest_unlock(tdb, ACTIVE_LOCK, F_WRLCK, false) == -1) {
TDB_LOG((tdb, TDB_DEBUG_ERROR, "tdb_open_ex: "
--
2.11.0
From 99ae955ce3f43ed0ec9b381a2e3ad68b830c7403 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 4 Mar 2018 11:26:37 +0100
Subject: [PATCH 6/8] tdb: Harden allocating the tdb recovery area
---
lib/tdb/common/transaction.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/lib/tdb/common/transaction.c b/lib/tdb/common/transaction.c
index 7d281fc75ff..6a5338c5faf 100644
--- a/lib/tdb/common/transaction.c
+++ b/lib/tdb/common/transaction.c
@@ -689,6 +689,8 @@ int tdb_recovery_area(struct tdb_context *tdb,
tdb_off_t *recovery_offset,
struct tdb_record *rec)
{
+ int ret;
+
if (tdb_ofs_read(tdb, TDB_RECOVERY_HEAD, recovery_offset) == -1) {
return -1;
}
@@ -709,6 +711,13 @@ int tdb_recovery_area(struct tdb_context *tdb,
*recovery_offset = 0;
rec->rec_len = 0;
}
+
+ ret = methods->tdb_oob(tdb, *recovery_offset, rec->rec_len, 1);
+ if (ret == -1) {
+ *recovery_offset = 0;
+ rec->rec_len = 0;
+ }
+
return 0;
}
--
2.11.0
From e65a09d571f69a8fa91d6b0f0c49d260e79d7ec8 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 4 Mar 2018 11:48:47 +0100
Subject: [PATCH 7/8] tdb: Align a few integer types
Signed-off-by: Volker Lendecke <vl at samba.org>
---
lib/tdb/common/transaction.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/tdb/common/transaction.c b/lib/tdb/common/transaction.c
index 6a5338c5faf..390e51dfa2a 100644
--- a/lib/tdb/common/transaction.c
+++ b/lib/tdb/common/transaction.c
@@ -592,7 +592,8 @@ static int transaction_sync(struct tdb_context *tdb, tdb_off_t offset, tdb_len_t
static int _tdb_transaction_cancel(struct tdb_context *tdb)
{
- int i, ret = 0;
+ uint32_t i;
+ int ret = 0;
if (tdb->transaction == NULL) {
TDB_LOG((tdb, TDB_DEBUG_ERROR, "tdb_transaction_cancel: no transaction\n"));
@@ -654,7 +655,7 @@ _PUBLIC_ int tdb_transaction_cancel(struct tdb_context *tdb)
static bool tdb_recovery_size(struct tdb_context *tdb, tdb_len_t *result)
{
tdb_len_t recovery_size = 0;
- int i;
+ uint32_t i;
recovery_size = sizeof(uint32_t);
for (i=0;i<tdb->transaction->num_blocks;i++) {
@@ -843,7 +844,7 @@ static int transaction_setup_recovery(struct tdb_context *tdb,
tdb_off_t recovery_offset, recovery_max_size;
tdb_off_t old_map_size = tdb->transaction->old_map_size;
uint32_t magic, tailer;
- int i;
+ uint32_t i;
/*
check that the recovery area has enough space
@@ -1105,7 +1106,7 @@ static bool repack_worthwhile(struct tdb_context *tdb)
_PUBLIC_ int tdb_transaction_commit(struct tdb_context *tdb)
{
const struct tdb_methods *methods;
- int i;
+ uint32_t i;
bool need_repack = false;
if (tdb->transaction == NULL) {
--
2.11.0
From de4ec0e24bf01a48a2582d1d2e3ebde2a4ecf8b1 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 4 Mar 2018 11:51:13 +0100
Subject: [PATCH 8/8] tdb: Fix a "increases alignment" warning
Many of those warnings are difficult to fix, but this one was easy :-)
Signed-off-by: Volker Lendecke <vl at samba.org>
---
lib/tdb/common/transaction.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/lib/tdb/common/transaction.c b/lib/tdb/common/transaction.c
index 390e51dfa2a..73d02b684a3 100644
--- a/lib/tdb/common/transaction.c
+++ b/lib/tdb/common/transaction.c
@@ -854,13 +854,12 @@ static int transaction_setup_recovery(struct tdb_context *tdb,
return -1;
}
- data = (unsigned char *)malloc(recovery_size + sizeof(*rec));
- if (data == NULL) {
+ rec = malloc(recovery_size + sizeof(*rec));
+ if (rec == NULL) {
tdb->ecode = TDB_ERR_OOM;
return -1;
}
- rec = (struct tdb_record *)data;
memset(rec, 0, sizeof(*rec));
rec->magic = TDB_RECOVERY_INVALID_MAGIC;
@@ -869,6 +868,8 @@ static int transaction_setup_recovery(struct tdb_context *tdb,
rec->key_len = old_map_size;
CONVERT(*rec);
+ data = (unsigned char *)rec;
+
/* build the recovery data into a single blob to allow us to do a single
large write, which should be more efficient */
p = data + sizeof(*rec);
--
2.11.0
More information about the samba-technical
mailing list