Hi
Inspired by a talk at FOSDEM this weekend [1], I tried building
Varnish with -fsanitize=address enabled.
This exposed a few buffer overflow situations, mainly in varnishtest.
Patches w/ fixes attached.
This was done with ASan's leak checker disabled, since varnishtest has
intentional memory leaks and I haven't yet investigated if specific
allocations can be annotated to make ASan disregard individual leaks.
In addition, there is a use-after-free situation in current master
related to ESI (not present in 4.1.1). I'm about to file a bug for
that one.
In order to build varnish w/ -fsanitize=address, you can do
./autogen.des --without-jemalloc CFLAGS="-fno-omit-frame-pointer
-fsanitize=address"
export ASAN_OPTIONS=detect_leaks=0
followed by the regular make/make check/etc.
This requires clang >= 3.1 or gcc >= 4.8. Disabling jemalloc is
required as ASan only works with the system allocator.
Note the third patch attached may be a false positive, but I think it
could be an issue on platforms where memcmp does 64 bit word
comparisons at a time.
Regards,
Dag
[1]:
https://fosdem.org/2016/schedule/event/csafecode/attachments/slides/1131/export/events/attachments/csafecode/slides/1131/fosdem_gentoo_asan.pdf
--
Dag Haavi Finstad
Software Developer | Varnish Software
Mobile: +47 476 64 134
We Make Websites Fly!
From 189d0f2bd5ebf183614b4888be31627b48617d58 Mon Sep 17 00:00:00 2001
From: Dag Haavi Finstad <[email protected]>
Date: Mon, 1 Feb 2016 18:25:03 +0100
Subject: [PATCH 1/3] Fix a buffer overflow in vtc.c:macro_get
Spotted by AddressSanitizer.
---
bin/varnishtest/vtc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/bin/varnishtest/vtc.c b/bin/varnishtest/vtc.c
index 33b56e1..92a2591 100644
--- a/bin/varnishtest/vtc.c
+++ b/bin/varnishtest/vtc.c
@@ -159,9 +159,10 @@ macro_get(const char *b, const char *e)
}
AZ(pthread_mutex_lock(¯o_mtx));
- VTAILQ_FOREACH(m, ¯o_list, list)
- if (!memcmp(b, m->name, l) && m->name[l] == '\0')
+ VTAILQ_FOREACH(m, ¯o_list, list) {
+ if (strlen(m->name) == l && !memcmp(b, m->name, l))
break;
+ }
if (m != NULL)
retval = strdup(m->val);
AZ(pthread_mutex_unlock(¯o_mtx));
--
2.7.0.rc3
From f52bc6a6c6ac9c5a6b299827a1951a3bae3685cc Mon Sep 17 00:00:00 2001
From: Dag Haavi Finstad <[email protected]>
Date: Mon, 1 Feb 2016 19:29:14 +0100
Subject: [PATCH 2/3] Fix a buffer overflow in vtc_http.c:cmd_var_resolve
Spotted by AddressSanitizer.
---
bin/varnishtest/vtc_http.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bin/varnishtest/vtc_http.c b/bin/varnishtest/vtc_http.c
index 0440542..6df2cae 100644
--- a/bin/varnishtest/vtc_http.c
+++ b/bin/varnishtest/vtc_http.c
@@ -223,10 +223,10 @@ cmd_var_resolve(struct http *hp, char *spec)
return(hp->bodylen);
if (!strcmp(spec, "resp.body"))
return(hp->body != NULL ? hp->body : spec);
- if (!memcmp(spec, "req.http.", 9)) {
+ if (strlen(spec) > 9 && !memcmp(spec, "req.http.", 9)) {
hh = hp->req;
hdr = spec + 9;
- } else if (!memcmp(spec, "resp.http.", 10)) {
+ } else if (strlen(spec) > 10 && !memcmp(spec, "resp.http.", 10)) {
hh = hp->resp;
hdr = spec + 10;
} else
--
2.7.0.rc3
From dfab2fc1512c8ed7dd3989fd9f0fa87400e13532 Mon Sep 17 00:00:00 2001
From: Dag Haavi Finstad <[email protected]>
Date: Mon, 1 Feb 2016 19:32:20 +0100
Subject: [PATCH 3/3] Test lengths of both ban strings, to avoid a potential
buffer overflow.
For memcmp() implementation that do 64 bit word comparisons at the time,
this could lead to a buffer overflow read.
Spotted by AddressSanitizer
---
bin/varnishd/cache/cache_ban.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/bin/varnishd/cache/cache_ban.c b/bin/varnishd/cache/cache_ban.c
index 2b71d94..cc7141c 100644
--- a/bin/varnishd/cache/cache_ban.c
+++ b/bin/varnishd/cache/cache_ban.c
@@ -139,15 +139,15 @@ ban_len(const uint8_t *banspec)
int
ban_equal(const uint8_t *bs1, const uint8_t *bs2)
{
- unsigned u;
+ unsigned u, v;
/*
* Compare two ban-strings.
- * The memcmp() is safe because the first field we compare is the
- * length and that is part of the fixed header structure.
*/
u = vbe32dec(bs1 + BANS_LENGTH);
- return (!memcmp(bs1 + BANS_LENGTH, bs2 + BANS_LENGTH, u - BANS_LENGTH));
+ v = vbe32dec(bs2 + BANS_LENGTH);
+ return (u == v &&
+ !memcmp(bs1 + BANS_LENGTH, bs2 + BANS_LENGTH, u - BANS_LENGTH));
}
void
--
2.7.0.rc3
_______________________________________________
varnish-dev mailing list
[email protected]
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
