Segfaults mod_tile.c:838 -> apr_strtok.c:46

[zenonp]

I am suddenly seeing a huge number of httpd coredumps (578 in the past three hours, coming from only 10 unique IP addresses), all of them for the same cause:

(gdb) backtrace
#0  apr_strtok (str=0x7f3a8800fbc8 "88.54.217.50", sep=sep@entry=0x7f3aa9d62389 ", ", last=last@entry=0x0) at strings/apr_strtok.c:46
#1  0x00007f3aa9d5b2da in delay_allowed (state=tileCurrent, r=0x7f3a8801a8d0) at ./src/mod_tile.c:838

This is mod_tile passing the contents of X-Forwarded-For to apr_strtok, which then barfs. I don't understand C, but I suspected a type mismatch in apr_strtok expecting string and getting integer or vice versa. So I tried to convert hex 0x7f3a8800fbc8 and 0x7f3aa9d62389 to text, and got 저 and 褀 respectively with UTF-16.

Now, I am not sure that these two hex values were actually sent to apr_strtok, nor that my hex-to-UTF-16 conversion is the correct one, but it seems very likely that these clients are sending garbage in their X-Forwarded-For. Which is easy for anyone to do, accidentally or maliciously. And mod_tile does no sanity check on X-Forwarded-For before passing to apr_strtok whatever it got from the client.

Thus, a sanity check just before mod_tile.c:838 would make a lot of sense, even if my troubleshooting is flawed somehow.

mod_tile 0.7.1, httpd 2.4.62.

[hummeltech]

Hello @zenonp,

Thank you for the report, I don't believe openstreetmap.org is using this functionality, so it probably wasn't thoroughly tested, but I think this should be fixed for the upcoming v0.8.x release, here's the commit, in case you're interested:
04a0bf0

Also, if you are able to test it out and let me know it resolved your issue, that would be helpful.

Thanks again and have a great day!

[zenonp]

Hi

Thank you for the fix. It might be a bit early to fully confirm it's working, but it's been running for one and a half hour now and I've had exactly zero new coredumps.

But I did spot a typo in mod_tile.c:2433: "determin" is missing its final "e" ;)

[hummeltech]

Thanks a bunch @zenonp, I'll submit a pull request for that typo, I've been working on increasing test coverage and fixing typos and segmentation faults (luckily I didn't find very many of them) for the last year or so, so I appreciate you helping out by testing it out for your use-case. I was planning on publishing a release of v0.8.0 soon, which all-in-all doesn't have any major changes, mainly just a refactoring of the configuration parsing code (so that it can be shared across both the Apache module and the daemon) and cleanup and consolidation of the various support-applications (e.g. render_...).