HTTP persistent connections should not be assumed for HTTP/1.0

[nnposter]

Function getPipelineMax in http.lua analyzes headers from a previously obtained HTTP response and attempts to guess how many HTTP requests can be batched together.

In particular, it assumes that persistent connections are available unless header Connection is set to close.

This is an oversimplification with respect to HTTP/1.0. See RFC 7230, section 6.3 for compliant behavior.

The following patch improves getPipelineMax by assuming that persistent connections are not available if the target response is HTTP/1.0 while missing the keep-alive connection option.

--- a/nselib/http.lua
+++ b/nselib/http.lua
@@ -872,7 +872,7 @@
 --  If the value is not available, an arbitrary value is used. If the connection
 --  is not explicitly closed by the server, this same value is attempted.
 --
---  @param response The http response - Might be a table or a raw response
+--  @param response The HTTP response table
 --  @return The max number of requests on a keep-alive connection
 local function getPipelineMax(response)
   -- Allow users to override this with a script-arg
@@ -883,16 +883,11 @@
   end
 
   if response then
-    if response.header and response.header.connection ~= "close" then
-      if response.header["keep-alive"] then
-        local max = string.match( response.header["keep-alive"], "max=(%d*)")
-        if(max == nil) then
-          return 40
-        end
-        return tonumber(max)
-      else
-        return 40
-      end
+    local hdr = response.header or {}
+    local opts = stdnse.strsplit("%s+", (hdr.connection or ""):lower())
+    if stdnse.contains(opts, "close") then return 1 end
+    if response.version >= "1.1" or stdnse.contains(opts, "keep-alive") then
+      return tonumber((hdr["keep-alive"] or ""):match("max=(%d+)")) or 40
     end
   end
   return 1

Note that this patch has a dependency on proposal #934.

Please let me know if you have any questions or concerns. Otherwise I will commit the patch in a few weeks.

[nnposter]

@dmiller-nmap FYI

[cldrn]

Looks good, please incorporate these changes whenever they are ready.

[dmiller-nmap]

A few questions:

• Are you sure the comparison operation for the version is correct? You seem to be comparing to a string "1.1" instead of a number 1.1.
• Does the spec allow for any variation in the "max=" part of the header? Like should we be case-insensitive or allow any whitespace?
• It looks like this catches any HTTP 1.0 servers that magically provide the Keep-Alive header, but is there any way to actually try pipelined requests if Connection: close is not sent? Maybe that doesn't belong in this function, though.

[nnposter]

To answer Dan's questions:

Comparison
Yes, the lexicographical string comparison is intentional as at removes the imprecision of floats.

Variations of "max="
The parsing of the Keep-Alive header is carried forward from the current code. I can look into its accuracy but I see it as a separate topic.

HTTP 1.0
No, the code does not base its key decision on the presence of Keep-Alive header but on the keep-alive option in the Connection header. Use of this option is the standardized way for HTTP/1.0 servers to signal their support for persistent connections.

It is certainly possible to try request pipe-lining against HTTP/1.0 targets but IMHO it is detrimental to do so because: (1) The RFC says that it should not be done and (2) I have dived into this issue specifically because particular real-world HTTP/1.0 targets were not working at all with the current pipe-lining approach.

[nnposter]

Hello Dan,
Are you OK with the code and the answers?

[nnposter]

As a side note, the max option in the Keep-Alive header does not seem to be standardized anywhere. The best documentation I could find is an expired RFC draft.

I do not see a reason to invest further into this unless somebody believes otherwise.