View Issue Details

IDProjectCategoryView StatusLast Update
0008787GNUnetnamestore servicepublic2024-05-05 13:58
Reporterulfvonbelow Assigned Toschanzen  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Platformx86-64OSGuix SystemOS Versiona1d711c92e
Product Version0.21.1 
Fixed in Version0.21.2 
Summary0008787: use-after-free in handle_edit_record_set
DescriptionThe editor_hint string that is passed in to GNUNET_NAMESTORE_RecordIterators
isn't guaranteed to stay live between when the iterator is called and when
GNUNET_NAMESTORE_PluginFunctions.edit_records returns. In fact, currently, it is guaranteed to have already been freed in the sqlite backend.
Steps To Reproduce1. ./configure --enable-sanitizer ; make ; make check
2. Observe use-after-free in gnunet-service-namestore.
Additional InformationThe already-freed editor hint string is copied up to the first null byte and sent to a client. I haven't looked into how deep the security implications of this go, so I'm setting this to private for now.

While observing this bug, the crash in gnunet-service-namestore caused by the sanitizer also caused a use-after-free to be observed in test_namestore_api_edit_records. This is because no error seems to have been reported despite nsqe already having been freed in force_reconnect in src/service/namestore/namestore_api.c. In fact, there seems to be no ability to specify an error callback with GNUNET_NAMESTORE_record_set_edit_begin, although the documentation next to the prototype in src/include/gnunet_namestore_service.h mentions both error_cb and error_cb_cls. Anyway, the endbadly timeout in the test eventually hits and it attempts to cancel nsqe, which has already been freed. I mention this here because it's not exactly a bug so much as a limitation.

After fixing this using the attached patch, it was discovered that editor_hint was actually never properly implemented in plugin_namestore_sqlite, so fixing this will actually cause test_namestore_api_edit_records to get the actual editor hint string and fail because of it. Will report that separately also.
TagsNo tags attached.
Attached Files
0001-service-namestore-fix-use-after-free-in-handle_edit_.patch (2,739 bytes)   
From 0c328d4b3a8c2c005f3798b1048b0862d1cd34bd Mon Sep 17 00:00:00 2001
From: ulfvonbelow <striness@tilde.club>
Date: Thu, 2 May 2024 16:05:00 -0500
Subject: [PATCH] service: namestore: fix use-after-free in
 handle_edit_record_set.

The editor_hint string that is passed in to GNUNET_NAMESTORE_RecordIterators
isn't guaranteed to stay live between when the iterator is called and when
GNUNET_NAMESTORE_PluginFunctions.edit_records returns. So lookup_it should
strdup that string, and anything that uses lookup_it should manage its
lifetime. Currently that's just handle_edit_record_set and
handle_record_lookup.
---
 src/service/namestore/gnunet-service-namestore.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/service/namestore/gnunet-service-namestore.c b/src/service/namestore/gnunet-service-namestore.c
index f29058d0e..448a9d2b3 100644
--- a/src/service/namestore/gnunet-service-namestore.c
+++ b/src/service/namestore/gnunet-service-namestore.c
@@ -1157,7 +1157,8 @@ lookup_it (void *cls,
   if (0 != strcmp (label, rlc->label))
     return;
   rlc->found = GNUNET_YES;
-  rlc->editor_hint = editor_hint;
+  if (NULL == rlc->editor_hint)
+    rlc->editor_hint = GNUNET_strdup (editor_hint);
   if (GNUNET_OK != GNUNET_GNSRECORD_normalize_record_set (rlc->label,
                                                           rd_nf,
                                                           rd_count_nf,
@@ -1332,6 +1333,7 @@ handle_edit_record_set (void *cls, const struct EditRecordSetMessage *er_msg)
     return;
   }
   name_len = strlen (conv_name) + 1;
+  rlc.editor_hint = NULL;
   rlc.label = conv_name;
   GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
               "Looking up without filter\n");
@@ -1370,6 +1372,7 @@ handle_edit_record_set (void *cls, const struct EditRecordSetMessage *er_msg)
   GNUNET_memcpy ((char*) &rer_msg[1] + old_editor_hint_len, rlc.res_rd,
                  rlc.rd_ser_len);
   GNUNET_MQ_send (nc->mq, env);
+  GNUNET_free (rlc.editor_hint);
   GNUNET_free (rlc.res_rd);
   GNUNET_free (conv_name);
 }
@@ -1566,6 +1569,7 @@ handle_record_lookup (void *cls, const struct LabelLookupMessage *ll_msg)
     return;
   }
   name_len = strlen (conv_name) + 1;
+  rlc.editor_hint = NULL;
   rlc.label = conv_name;
   GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
               "Looking up with filter %u\n", ntohs (ll_msg->filter));
@@ -1601,6 +1605,7 @@ handle_record_lookup (void *cls, const struct LabelLookupMessage *ll_msg)
   GNUNET_memcpy (res_name, conv_name, name_len);
   GNUNET_memcpy (&res_name[name_len], rlc.res_rd, rlc.rd_ser_len);
   GNUNET_MQ_send (nc->mq, env);
+  GNUNET_free (rlc.editor_hint);
   GNUNET_free (rlc.res_rd);
   GNUNET_free (conv_name);
 }
-- 
2.41.0

Activities

schanzen

2024-05-05 13:58

administrator   ~0022341

Applied in 55cf889cf6e337aad5a9f5215384775a47a94fed. Thanks.

Issue History

Date Modified Username Field Change
2024-05-03 03:23 ulfvonbelow New Issue
2024-05-03 03:23 ulfvonbelow File Added: 0001-service-namestore-fix-use-after-free-in-handle_edit_.patch
2024-05-05 13:58 schanzen Assigned To => schanzen
2024-05-05 13:58 schanzen Status new => resolved
2024-05-05 13:58 schanzen Resolution open => fixed
2024-05-05 13:58 schanzen Fixed in Version => 0.21.2
2024-05-05 13:58 schanzen Note Added: 0022341
2024-05-05 13:58 schanzen View Status private => public