Re: bugs in http.lua?

[David Fifield <david () bamsoftware com>]

On Sun, Nov 29, 2009 at 11:40:54PM +0100, Patrik Karlsson wrote:
> I'm currently re-writinig my Citrix xml plugins to use the http
> module, but have come a cross some problems.
>
> The first problem is that the content I'm sending to the server is of
> text/xml rather than application/x-www-form-urlencoded. I manage to
> change this by calling http.post with the following table in the
> option parameter:
> { header={["Content-Type"]="text/xml"}}
>
> This solves one problem, but the buildPost function then replaces all
> spaces with pluses, which is probably right for
> application/x-www-form-urlencoded but in my case it breaks my xml
> post. I've managed to work around this in my code by calling
> http.request and http.parseResult directly instead. The downside is
> that I have to build the http headers myself.

Thanks, Patrik. I don't think the library should be messing with a
request body if it is a string. If it's a table then it's a little
different because presumably we want to emulate a form submission. I
changed the code to only set application/x-www-form-urlencoded and
encode the body if it comes in as a table.

> The next problem is that the server responds with a "HTTP/1.1 100
> Continue". So basically what the http module gets is this:
>
> HTTP/1.1 100 Continue
> Server: Citrix Web PN Server
> Date: Sun, 29 Nov 2009 22:03:42 GMT
>
> HTTP/1.1 200 OK
> Server: Citrix Web PN Server
> Date: Sun, 29 Nov 2009 22:03:42 GMT
> Content-type: text/xml
> Transfer-Encoding: chunked
> Transfer-Coding: chunked
>  
> DATA GOES HERE ....
>
> As the module splits the headers from the body by searching for
> "\r?\n\r?\n" it incorrectly ends up with the second block of HTTP
> headers as data. By calling http.request directly I have the
> possibility to cut away this excessive block of headers myself before
> sending the data along to the parseResult function. 

Removing the 100 response is correct. This is what RFC 2616 says:

10.1.1 100 Continue
   The client SHOULD continue with its request. This interim response is
   used to inform the client that the initial part of the request has
   been received and has not yet been rejected by the server. The client
   SHOULD continue by sending the remainder of the request or, if the
   request has already been completed, ignore this response. The server
   MUST send a final response after the request has been completed. See
   section 8.2.3 for detailed discussion of the use and handling of this
   status code.

However, I don't like the way that the patch does it.

  -- if we have a 100 Continue, we need to chop it of
  -- or else the next separation will endup with our 200 OK as body
  if response:match("^HTTP/1.1 100 Continue") then
    -- Try to match from HTTP/1.1 200 OK to the end
    if response:match("(HTTP/1.1 200 OK.+)") then
      response = response:match("(HTTP/1.1 200 OK.+)")
    end
  end 

The main problem with it is that you're looking for specific values for
strings that are allowed to vary. The response might not say "HTTP/1.1"
but maybe "HTTP/1.0" or even "HTTP/1.2" in the future. The
Reason-Phrases "Continue" and "OK" may be blank, in a different
language, or nonsense; the only thing that matters is the status code.

Another problem is that the code only allows a 200 status code following
the 100. If a POST results in a 301 or a 400 or something else, that's
what has to be reported to the caller. See what you can do to handle all
these cases. Don't grep for something that looks like a Status-Line
("HTTP/1.1 200 OK") to decide when the next response begins; that way is
incorrect even though the http library does it in other places, and it's
not necessary here. The 100 response ends after the first "\r?\n\r?\n"
because 100 responses aren't allowed to have a body. This is what
section 4.3 says:

   For response messages, whether or not a message-body is included with
   a message is dependent on both the request method and the response
   status code (section 6.1.1). All responses to the HEAD request method
   MUST NOT include a message-body, even though the presence of entity-
   header fields might lead one to believe they do. All 1xx
   (informational), 204 (no content), and 304 (not modified) responses
   MUST NOT include a message-body. All other responses do include a
   message-body, although it MAY be of zero length.

Ideally the process for requests would look like this:

  status = send_request
  -- Check for errors.
  while true do
    resp = read_response
    if resp.status % 100 != 100 then
      break
    end
  end
  return resp

But the current design of the http library doesn't work well for that. A
"good enough" solution is good enough for now.

David Fifield
_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/