--enable-x-forwarded-for and

[GoogleCodeExporter]

What steps will reproduce the problem?
1. compiling with ./configure --prefix=/usr/local/squid
--enable-async-io=16 --with-pthreads --with-aio --with-large-files
--enable-follow-x-forwarded-for
2. Enable follow_x_forwarded_for allow pixnetsrc
3.

What is the expected output? What do you see instead?
Crash like:
pid 25637 (squid), uid 65534: exited on signal 6
pid 25657 (squid), uid 65534: exited on signal 6
pid 25665 (squid), uid 65534: exited on signal 6
pid 25672 (squid), uid 65534: exited on signal 6
pid 25685 (squid), uid 65534: exited on signal 6
pid 25696 (squid), uid 65534: exited on signal 6

What version of the product are you using? On what operating system?
1. FreeBSD 7.2-RELEASE-p2 (amd64)
2. LUSCA_HEAD r14247

Please provide any additional information below.

Program received signal SIGABRT, Aborted.
[Switching to Thread 0x800f020b0 (LWP 100306)]
0x0000000800c8ba9c in thr_kill () from /lib/libc.so.7
(gdb) bt
#0  0x0000000800c8ba9c in thr_kill () from /lib/libc.so.7
#1  0x0000000800d1affb in abort () from /lib/libc.so.7
#2  0x000000000046d4fd in xassert (msg=Variable "msg" is not available.
) at debug.c:270
#3  0x000000000046ea7f in strCut (s=0x802ff9550, offset=14) at String.c:198
#4  0x000000000041fa1c in clientFollowXForwardedForDone (answer=Variable
"answer" is not available.
) at client_side.c:294
#5  0x0000000000406ce5 in aclCheckCallback (checklist=0x80578d2a8,
answer=ACCESS_ALLOWED) at acl.c:2338
#6  0x000000000042062a in clientTryParseRequest (conn=0x80578d158) at
client_side.c:4304
#7  0x0000000000421285 in clientReadRequest (fd=22, data=Variable "data" is
not available.
) at client_side.c:4428
#8  0x000000000047c256 in comm_select (msec=Variable "msec" is not available.
) at comm_generic.c:264
#9  0x000000000043e59b in main (argc=Variable "argc" is not available.
) at main.c:933
(gdb)

In String.c:
195 extern void
196 strCut(String *s, int offset)
197 {
198         assert(offset < strLen(*s));
199         s->buf[offset] = '\0';
200         s->len = offset;
201 }

In client_side.c:
291         /* skip trailing space and commas */
292         while (l > 0 && (p[l - 1] == ',' || xisspace(p[l - 1])))
293             l--;
294         strCut(&request->x_forwarded_for_iterator, l);
295         /* look for start of last item in list */
296         while (l > 0 && !(p[l - 1] == ',' || xisspace(p[l - 1])))
297             l--;

Original issue reported on code.google.com by darkkiller on 28 Jul 2009 at 7:05

[GoogleCodeExporter]

What is the value of offset and *s ?

Original comment by adrian.c...@gmail.com on 29 Jul 2009 at 4:19

• Added labels: Priority-Critical, Version-Head, Feature-X-Forwarded-For
• Removed labels: Priority-Medium, Version-1.0

[GoogleCodeExporter]

gslin@rproxy-pic-large-1 [~] (12:59) sudo gdb /usr/local/squid/sbin/squid
GNU gdb 6.1.1 [FreeBSD]
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "amd64-marcel-freebsd"...
(gdb) run -NsY
Starting program: /usr/local/squid/sbin/squid -NsY
[New LWP 100166]
[New Thread 0x800f020b0 (LWP 100166)]
[New Thread 0x800f023d0 (LWP 100185)]
[New Thread 0x800f02560 (LWP 100186)]
[New Thread 0x800f026f0 (LWP 100187)]
[New Thread 0x800f02880 (LWP 100188)]
[New Thread 0x800f02a10 (LWP 100189)]
[New Thread 0x800f02ba0 (LWP 100190)]
[New Thread 0x800f02d30 (LWP 100191)]
[New Thread 0x800f02ec0 (LWP 100192)]
[New Thread 0x800f03050 (LWP 100193)]
[New Thread 0x800f031e0 (LWP 100194)]
[New Thread 0x800f03370 (LWP 100195)]
[New Thread 0x800f03500 (LWP 100196)]
[New Thread 0x800f03690 (LWP 100197)]
[New Thread 0x800f03820 (LWP 100198)]
[New Thread 0x800f039b0 (LWP 100199)]
[New Thread 0x800f03b40 (LWP 100200)]

Program received signal SIGABRT, Aborted.
[Switching to Thread 0x800f020b0 (LWP 100166)]
0x0000000800c8ba9c in thr_kill () from /lib/libc.so.7
(gdb) bt
#0  0x0000000800c8ba9c in thr_kill () from /lib/libc.so.7
#1  0x0000000800d1affb in abort () from /lib/libc.so.7
#2  0x000000000046d4fd in xassert (msg=Variable "msg" is not available.
) at debug.c:270
#3  0x000000000046ea7f in strCut (s=0x802ff9550, offset=14) at String.c:198
#4  0x000000000041fa1c in clientFollowXForwardedForDone (answer=Variable 
"answer" is
not available.
) at client_side.c:294
#5  0x0000000000406ce5 in aclCheckCallback (checklist=0x807355698,
answer=ACCESS_ALLOWED) at acl.c:2338
#6  0x000000000042062a in clientTryParseRequest (conn=0x807355158) at 
client_side.c:4304
#7  0x0000000000421285 in clientReadRequest (fd=22, data=Variable "data" is not
available.
) at client_side.c:4428
#8  0x000000000047c256 in comm_select (msec=Variable "msec" is not available.
) at comm_generic.c:264
#9  0x000000000043e59b in main (argc=Variable "argc" is not available.
) at main.c:933
(gdb) up
#1  0x0000000800d1affb in abort () from /lib/libc.so.7
(gdb) up
#2  0x000000000046d4fd in xassert (msg=Variable "msg" is not available.
) at debug.c:270
270             abort();
(gdb) up
#3  0x000000000046ea7f in strCut (s=0x802ff9550, offset=14) at String.c:198
198             assert(offset < strLen(*s));
(gdb) p *s
$1 = {size = 36, len = 14, buf = 0x8030525e0 "61.228.130.130"}
(gdb) p offset
$2 = 14
(gdb) p
$3 = 14
(gdb) up
#4  0x000000000041fa1c in clientFollowXForwardedForDone (answer=Variable 
"answer" is
not available.
) at client_side.c:294
294             strCut(&request->x_forwarded_for_iterator, l);
(gdb) p l
$4 = 14
(gdb) p request->x_forwarded_for_iterator
$5 = {size = 36, len = 14, buf = 0x8030525e0 "61.228.130.130"}
(gdb) up
#5  0x0000000000406ce5 in aclCheckCallback (checklist=0x807355698,
answer=ACCESS_ALLOWED) at acl.c:2338
2338            checklist->callback(answer, checklist->callback_data);
(gdb) p answer
$6 = ACCESS_ALLOWED
(gdb) p checklist->callback_data
$7 = (void *) 0x802f45818
(gdb) up
#6  0x000000000042062a in clientTryParseRequest (conn=0x807355158) at 
client_side.c:4304
4304                clientCheckFollowXForwardedFor(http);
(gdb) p http
$8 = (clientHttpRequest *) 0x802f45818
(gdb) The program is running.  Exit anyway? (y or n) y

Original comment by darkkiller on 29 Jul 2009 at 5:02

[GoogleCodeExporter]

Right. This is due to the changes I made to the string API a while ago. 
strCut() was
a #define which just added the NUL character where needed without any bounds 
checking. 

This particular use of strCut() wants to cut the string -after- the string 
length -
which "works" for C strings because the string length doesn't include the 
trailing
NUL. In this case, the trailing NUL is just overwritten. The interesting issue 
is the
string length being set incorrectly - it now includes the trailing NUL when it 
shouldn't.

Anyway. The short short term fix is to just return from strCut() if l >= 
length. The
long(er) term fix (which is what I'll commit) is to audit the three places 
strCut()
is called to make sure its called correctly; then document the String functions 
a bit
better. :)

Thanks for this!

Original comment by adrian.c...@gmail.com on 29 Jul 2009 at 7:13

[GoogleCodeExporter]

Thanks, I'll try to change assert() code to workaround it.

Original comment by darkkiller on 29 Jul 2009 at 10:23

[GoogleCodeExporter]

Try patching libmem/String.c with this:

Index: String.c
===================================================================
--- String.c    (revision 14244)
+++ String.c    (working copy)
@@ -192,9 +192,23 @@
        return -1;
 }

+/*
+ * Cut the given string at the given offset.
+ * "offset" -should- be less than the length of the string but
+ * at least the client_side X-Forwarded-For code currently (ab)uses
+ * the API and passes in an out of bounds iterator. In this case,
+ * don't cut the string.
+ */
 extern void
 strCut(String *s, int offset)
 {
+       /*
+        * XXX this should eventually be removed and all code
+        * XXX which triggers it should be fixed!
+        */
+       if (offset >= strLen(*s))
+               return;
+
        assert(offset < strLen(*s));
        s->buf[offset] = '\0';
        s->len = offset;

Original comment by adrian.c...@gmail.com on 29 Jul 2009 at 2:55

[GoogleCodeExporter]

Yes, I had patched when I replied, and it works fine.

Original comment by darkkiller on 29 Jul 2009 at 5:11

[GoogleCodeExporter]

Fixed in revision r14254.

Original comment by adrian.c...@gmail.com on 30 Jul 2009 at 2:59

• Changed state: Fixed