New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESI parser panic on malformed src URL #2197

Closed
daghf opened this Issue Jan 24, 2017 · 5 comments

Comments

Projects
None yet
5 participants
@daghf
Member

daghf commented Jan 24, 2017

Current Behavior

For an esi:include where the src attribute is specified with an absolute-URL with the URL component missing (e.g. <esi:include src="http://foobar" />) we hit a panic.

***  v1    0.4 debug|Assert error in vep_do_include(), cache/cache_esi_parse.c line 482:
***  v1    0.4 debug|  Condition((p) != 0) not true.
***  v1    0.4 debug|thread = (cache-worker)
***  v1    0.4 debug|version = varnish-trunk revision fbe04890b
***  v1    0.4 debug|ident = Linux,4.8.0-2-amd64,x86_64,-jnone,-smalloc,-smalloc,-hcritbit,epoll
***  v1    0.4 debug|now = 1366.807007 (mono), 1485262000.473857 (real)
***  v1    0.4 debug|Backtrace:
***  v1    0.4 debug|  0x452350: varnishd() [0x452350]
***  v1    0.4 debug|  0x452253: varnishd() [0x452253]
***  v1    0.4 debug|  0x42f148: varnishd() [0x42f148]
***  v1    0.4 debug|  0x42db5b: varnishd(VEP_Parse+0xdbb) [0x42db5b]
***  v1    0.4 debug|  0x42bc90: varnishd() [0x42bc90]
***  v1    0.4 debug|  0x439489: varnishd(VFP_Suck+0x249) [0x439489]
***  v1    0.4 debug|  0x4371ca: varnishd() [0x4371ca]
***  v1    0.4 debug|  0x4336a2: varnishd() [0x4336a2]
***  v1    0.4 debug|  0x480ed6: varnishd() [0x480ed6]
***  v1    0.4 debug|  0x480729: varnishd() [0x480729]
***  v1    0.4 debug|busyobj = 0x7f7964006020 {
***  v1    0.4 debug|  ws = 0x7f79640060a0 {
***  v1    0.4 debug|    id = \\\"bo\\\",
***  v1    0.4 debug|    {s, f, r, e} = {0x7f7964007f60, +856, (nil), +57496},
***  v1    0.4 debug|  },
***  v1    0.4 debug|  retries = 0, failed = 0, flags = {do_esi, is_gunzip},
***  v1    0.4 debug|  http_conn = 0x7f7964007fd8 {
***  v1    0.4 debug|    fd = 23,
***  v1    0.4 debug|    doclose = NULL,
***  v1    0.4 debug|    ws = 0x7f79640060a0 {
***  v1    0.4 debug|      [Alread
***  v1    0.4 debug| [...] (2135)**** v1    0.4 vsl|          0 CLI             - Rd debug.xid 999 
**** v1    0.4 vsl|          0 CLI             - Wr 200 10 XID is 999
**** v1    0.4 vsl|          0 CLI             - Rd debug.listen_address 
**** v1    0.4 vsl|          0 CLI             - Wr 200 16 127.0.0.1 46037

**** v1    0.4 vsl|       1000 Begin           c sess 0 HTTP/1
**** v1    0.4 vsl|       1000 SessOpen        c 127.0.0.1 38292 127.0.0.1:46037 127.0.0.1 46037 1485262000.472161 20
**** v1    0.4 vsl|       1000 Link            c req 1001 rxreq
**** v1    0.4 vsl|       1002 Begin           b bereq 1001 fetch
**** v1    0.4 vsl|       1002 Timestamp       b Start: 1485262000.472492 0.000000 0.000000
**** v1    0.4 vsl|       1002 BereqMethod     b GET
**** v1    0.4 vsl|       1002 BereqURL        b /
**** v1    0.4 vsl|       1002 BereqProtocol   b HTTP/1.1
**** v1    0.4 vsl|       1002 BereqHeader     b X-Forwarded-For: 127.0.0.1
**** v1    0.4 vsl|       1002 BereqHeader     b Accept-Encoding: gzip
**** v1    0.4 vsl|       1002 BereqHeader     b X-Varnish: 1002
**** v1    0.4 vsl|       1002 VCL_call        b BACKEND_FETCH
**** v1    0.4 vsl|       1002 VCL_return      b fetch
**** v1    0.4 vsl|       1002 BereqHeader     b Host: 127.0.0.1
**** v1    0.4 vsl|       1002 BackendOpen     b 23 vcl1.s1 127.0.0.1 32985 127.0.0.1 46586
**** v1    0.4 vsl|       1002 BackendStart    b 127.0.0.1 32985
**** v1    0.4 vsl|       1002 Timestamp       b Bereq: 1485262000.472828 0.000335 0.000335
**** v1    0.4 vsl|       1002 Timestamp       b Beresp: 1485262000.473342 0.000850 0.000515
**** v1    0.4 vsl|       1002 BerespProtocol  b HTTP/1.1
**** v1    0.4 vsl|       1002 BerespStatus    b 200
**** v1    0.4 vsl|       1002 BerespReason    b OK
**** v1    0.4 vsl|       1002 BerespHeader    b Content-Length: 35
**** v1    0.4 vsl|       1002 BerespHeader    b Date: Tue, 24 Jan 2017 12:46:40 GMT
**** v1    0.4 vsl|       1002 TTL             b RFC 120 10 0 1485262000 1485262000 1485262000 0 0
**** v1    0.4 vsl|       1002 VCL_call        b BACKEND_RESPONSE
**** v1    0.4 vsl|       1002 VCL_return      b deliver
**** v1    0.4 vsl|       1002 Storage         b malloc s0
**** v1    0.4 vsl|       1002 ObjProtocol     b HTTP/1.1
**** v1    0.4 vsl|       1002 ObjStatus       b 200
**** v1    0.4 vsl|       1002 ObjReason       b OK
**** v1    0.4 vsl|       1002 ObjHeader       b Content-Length: 35
**** v1    0.4 vsl|       1002 ObjHeader       b Date: Tue, 24 Jan 2017 12:46:40 GMT
**** v1    0.4 vsl|       1002 Fetch_Body      b 3 length -
***  v1    0.4 debug|Debug: Child cleanup complete
***  v1    0.4 CLI RX  101
**** v1    0.4 CLI RX|Unknown request in manager process (child not running).
**** v1    0.4 CLI RX|Type 'help' for more info.
**** v1    1.4 STDOUT poll 0x10
**   v1    1.4 R 6135 Status: 0000 (u 0.056000 s 0.040000)
*    top   1.5 TEST h.vtc FAILED
#    top  TEST h.vtc FAILED (1.506) exit=2

Possible Solution

We should probably vep_error out if no URL component was found.

Steps to Reproduce (for bugs)

varnishtest "ESI parsing crash"

server s1 {
	rxreq
	txresp -body {<esi:include src="http://foobar" />}
} -start

varnish v1 -vcl+backend {
	sub vcl_backend_response {
		set beresp.do_esi = true;
	}
} -start

client c1 {
	txreq
	rxresp
	expect resp.status == 200
}

client c1 -run
varnish v1 -expect esi_errors == 1
@fgsch

This comment has been minimized.

Show comment
Hide comment
@fgsch

fgsch Jan 25, 2017

Member

Proposed patch:

diff --git a/bin/varnishd/cache/cache_esi_parse.c b/bin/varnishd/cache/cache_esi_parse.c
index 7f17cc3..1faca8e 100644
--- a/bin/varnishd/cache/cache_esi_parse.c
+++ b/bin/varnishd/cache/cache_esi_parse.c
@@ -479,7 +479,14 @@ vep_do_include(struct vep_state *vep, enum dowhat what)
        if (l > 7 && !memcmp(p, "http://", 7)) {
                h = p + 7;
                p = strchr(h, '/');
-               AN(p);
+               if (p == NULL) {
+                       vep_error(vep,
+                           "ESI 1.0 <esi:include> host without path");
+                       vep->state = VEP_TAGERROR;
+                       AZ(vep->attr_vsb);
+                       VSB_destroy(&vep->include_src);
+                       return;
+               }
                Debug("HOST <%.*s> PATH <%s>\n", (int)(p-h),h, p);
                VSB_printf(vep->vsb, "%c", VEC_INCL);
                VSB_printf(vep->vsb, "Host: %.*s%c", (int)(p-h), h, 0);

Output:

1002 ESI_xmlerror    b ERR at 35 ESI 1.0 <esi:include> host without path
Member

fgsch commented Jan 25, 2017

Proposed patch:

diff --git a/bin/varnishd/cache/cache_esi_parse.c b/bin/varnishd/cache/cache_esi_parse.c
index 7f17cc3..1faca8e 100644
--- a/bin/varnishd/cache/cache_esi_parse.c
+++ b/bin/varnishd/cache/cache_esi_parse.c
@@ -479,7 +479,14 @@ vep_do_include(struct vep_state *vep, enum dowhat what)
        if (l > 7 && !memcmp(p, "http://", 7)) {
                h = p + 7;
                p = strchr(h, '/');
-               AN(p);
+               if (p == NULL) {
+                       vep_error(vep,
+                           "ESI 1.0 <esi:include> host without path");
+                       vep->state = VEP_TAGERROR;
+                       AZ(vep->attr_vsb);
+                       VSB_destroy(&vep->include_src);
+                       return;
+               }
                Debug("HOST <%.*s> PATH <%s>\n", (int)(p-h),h, p);
                VSB_printf(vep->vsb, "%c", VEC_INCL);
                VSB_printf(vep->vsb, "Host: %.*s%c", (int)(p-h), h, 0);

Output:

1002 ESI_xmlerror    b ERR at 35 ESI 1.0 <esi:include> host without path
@fgsch

This comment has been minimized.

Show comment
Hide comment
@fgsch

fgsch Jan 25, 2017

Member

Actually this applies to https:// as well.

Member

fgsch commented Jan 25, 2017

Actually this applies to https:// as well.

@fgsch

This comment has been minimized.

Show comment
Hide comment
@fgsch

fgsch Jan 25, 2017

Member

On a related note, having this:

<esi:include src="http://" />

leads to:

ReqURL c /http://

Member

fgsch commented Jan 25, 2017

On a related note, having this:

<esi:include src="http://" />

leads to:

ReqURL c /http://

@bsdphk

This comment has been minimized.

Show comment
Hide comment
@bsdphk

bsdphk Jan 25, 2017

Contributor

We obviously shouldn't panic and I'll fix that.

But I don't see it as our job to waste a lot of time syntax-checking esi:include URLs, they come from the backend and are therefore explicitly trusted.

Contributor

bsdphk commented Jan 25, 2017

We obviously shouldn't panic and I'll fix that.

But I don't see it as our job to waste a lot of time syntax-checking esi:include URLs, they come from the backend and are therefore explicitly trusted.

@hermunn

This comment has been minimized.

Show comment
Hide comment
@hermunn

hermunn Jan 26, 2017

Contributor

Backport review: 7de2ff8 backported as 880b4ba.

Contributor

hermunn commented Jan 26, 2017

Backport review: 7de2ff8 backported as 880b4ba.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment