Crash on workspace overflow

[slimhazard]

We have received a report of Varnish crashing, with the panic message indicating an assertion failure in VMOD geoip2. The assertion failure appears to have resulted from Req workspace overflow, evidently due to a request that was flooded with headers. The request could have resulted from a bug, or it might have been a deliberate attempt to provoke the overflow.

Relevant excerpts from the panic:

Assert error in vmod_geoip2_lookup(), vmod_geoip2.c line 148:
  Condition((addr) != 0) not true.
[...]
  ws = 0x7f449d427178 {
    OVERFLOWED id = \"Req\",
    {s, f, r, e} = {0x7f449d429db0, +7136, (nil), +512584},
  },
[...]
    hdrs {
      \"GET\",
      \"/robots.txt\",
      \"HTTP/1.1\",
[...]
      \"user-agent: RyteBot/1.0.0 (Linux; Android 8.0.0; Nexus 5X Build/OPR4.170623.006) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/93.0.4577.82 Mobile Safari/537.36 (+https://bot.ryte.com/)\",
[...]
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*;q=0.8\",
      \"accept: text/html,application/xhtml+xml;q=0.9,*/*

The assertion is AN(addr) at the start of vmod_geoip2_lookup(). I assume that the address couldn't be allocated after the workspace overflow.

I suggest that instead of the assertion, the VMOD invokes VCL failure if addr is NULL.

There are at least 35 instances of the accept header in the request, presumably quite a bit more, but this is where the buffer for /var/log/messages ran out of room. That could have been a bug in whatever the "ryte bot" is, or could have been an attack, possibly just a test to see what happens if the request header is flooded. Either way, this amounts to DOS, if you think about it. We really don't want the VMOD to expose such a risk.

[fgsch]

Thanks for the report. I will push a PR with a fix shortly.

[fgsch]

@slimhazard Can you take a look at #43 and let me know if that works for you? Thank you.

[slimhazard]

@fgsch lgtm. I was thinking of invoking VCL failure, but if the function returns NULL, the caller can check the value in VCL and decide to return(fail), or choose to do something else. Or just let a header get no value. It's more flexible this way.

[fgsch]

Thanks for checking it. I will tag a release and also cherry-pick that commit for the other branch later today.

Re VCL failure, we could call VRT_fail() indeed, but we might want to do that in a separate PR, and also apply it to other error paths. That said, I'm not a big fan of that mechanism and if we were to use it, I'd like to make it optional.

In any case, I think we agree on the approach so I'm going to close this as fixed now.