[PATCH] harden libtdb against corrupt databases
Jeremy Allison
jra at samba.org
Wed Mar 21 23:38:58 UTC 2018
On Tue, Mar 20, 2018 at 05:31:08PM +0100, Volker Lendecke via samba-technical wrote:
> 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!
LGTM. Reviewed-by: Jeremy Allison <jra at samba.org>
I'll push once my current autobuild has finished...
Jeremy.
> --
> 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
> 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