r13786@catbus: nickm | 2007-07-16 13:46:28 -0400

 Backport a minimal fix for bug 455: strndup a NUL-terminated copy of each router descriptor before trying to parse it.  If this slows us down a lot, we will need to reconsider, but it seems far safer than the more sophisticated stuff we are trying to do to fix 455 on trunk.


svn:r10846

3 files changed, 16 insertions, 2 deletions

diff --git a/ChangeLog b/ChangeLog
index c77e2ea96f..55708d4cae 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -10,6 +10,9 @@ Changes in version 0.1.2.15 - 2007-07-??
     - Fix eventdns.c behavior on Solaris: It is critical to include
       orconfig.h _before_ sys/types.h, so that we can get the expected
       definition of _FILE_OFFSET_BITS.
+    - When the cached-routers file is an even multiple of the page size,
+      don't run off the end and crash.  (Fixes bug 455; based on idea
+      from croup.)
 
   o Major bugfixes (security):
     - Fix a possible buffer overrun when using BSD natd support.
diff --git a/doc/TODO.012 b/doc/TODO.012
index 978e5f4b4c..7a8b0b4c41 100644
--- a/doc/TODO.012
+++ b/doc/TODO.012
@@ -16,7 +16,7 @@ Backport items for 0.1.2:
   o r10730: Don't choose guards after any never-connected-to guard.
   o r10760: fix possible buffer overrun in old BSD natd code
   o r10790: Don't include reasons in destroy cells from the origin.
-  - Some fix for bug 455.
+  o Some fix for bug 455.
   o r10830 if nick thinks it's a real fix
   - r10835 if nick thinks it's a real fix
 
diff --git a/src/or/routerparse.c b/src/or/routerparse.c
index 9ef6249211..ff6bff53b1 100644
--- a/src/or/routerparse.c
+++ b/src/or/routerparse.c
@@ -649,6 +649,8 @@ router_parse_list_from_string(const char **s, const char *eos,
 {
   routerinfo_t *router;
   const char *end, *cp, *start;
+  char *buf;
+  size_t buf_len;
 
   tor_assert(s);
   tor_assert(*s);
@@ -690,14 +692,23 @@ router_parse_list_from_string(const char **s, const char *eos,
     /* cp now points to the first \n before the last non-blank line in this
      * descriptor */
 
+    if (eos - cp < 25) /* not long enough to hold an "end signature" */
+      break;
+
     if (strcmpstart(cp, "\n-----END SIGNATURE-----\n")) {
       log_info(LD_DIR, "Ignoring truncated router descriptor.");
       *s = end;
       continue;
     }
 
-    router = router_parse_entry_from_string(*s, end,
+    /* router_parse_entry_from_string isn't necessarily safe if the string
+     * is non-NUL-terminated.  This fix is a workaround for the stable
+     * series only;  */
+    buf_len = end-*s;
+    buf = tor_strndup(*s, buf_len); /* nul-terminates the copy. */
+    router = router_parse_entry_from_string(buf, buf+buf_len,
                                             saved_location != SAVED_IN_CACHE);
+    tor_free(buf);
 
     if (!router) {
       log_warn(LD_DIR, "Error reading router; skipping");