From 990c9c36e43b0739e2951255568cf165825b4fa8 Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Tue, 21 May 2024 22:58:46 -0400 Subject: [PATCH] Fix tests and boundary condition (#410) Fix two failing tests Write a bunch more tests to expand coverage and check for errors Fix a bug where we can have no response and trigger a success --- lib/http/transport.go | 2 +- modules/http/http_readlimit_test.go | 208 +++++++++++++++++++--------- 2 files changed, 146 insertions(+), 64 deletions(-) diff --git a/lib/http/transport.go b/lib/http/transport.go index d9288e6e..f8cfdf54 100644 --- a/lib/http/transport.go +++ b/lib/http/transport.go @@ -1553,7 +1553,7 @@ func (pc *persistConn) readLoop() { closeErr = err } - if err != nil && (!pc.sawEOF || resp == nil) { + if err != nil && (!pc.sawEOF || resp == nil || resp.Status == "") { if pc.readLimit <= 0 { err = fmt.Errorf("net/http: server response headers exceeded %d bytes; aborted", pc.maxHeaderResponseSize()) } diff --git a/modules/http/http_readlimit_test.go b/modules/http/http_readlimit_test.go index 7ed76caf..90eaaeaf 100644 --- a/modules/http/http_readlimit_test.go +++ b/modules/http/http_readlimit_test.go @@ -13,6 +13,7 @@ import ( "github.com/zmap/zcrypto/tls" "github.com/zmap/zgrab2" + "github.com/zmap/zgrab2/lib/http" ) // BEGIN Taken from handshake_server_test.go -- certs for TLS server @@ -107,7 +108,13 @@ func (cfg *readLimitTestConfig) runFakeHTTPServer(t *testing.T) { } head := "HTTP/1.0 200 OK\r\nBogus-Header: X" + if cfg.customHeader != nil { + head = *cfg.customHeader + } headSuffix := fmt.Sprintf("\r\nContent-Length: %d\r\n\r\n", cfg.bodySize) + if cfg.customSuffix != nil { + headSuffix = *cfg.customSuffix + } size := cfg.headerSize - len(head) - len(headSuffix) if size < 0 { t.Fatalf("Header size %d too small: must be at least %d bytes", cfg.headerSize, len(head)+len(headSuffix)) @@ -191,6 +198,11 @@ type readLimitTestConfig struct { // If set, the error returned by the scan must contain this. expectedError string + + // If set, return a custom header + customHeader *string + + customSuffix *string } const ( @@ -198,6 +210,8 @@ const ( readLimitTestConfigHTTPSBasePort = 0x7bbc ) +func adr(s string) *string {return &s} + var readLimitTestConfigs = map[string]*readLimitTestConfig{ // The socket truncates the connection while reading the body. To the client it looks as if the // server closed the connection prior to sending Content-Length bytes; the result is success, @@ -219,84 +233,140 @@ var readLimitTestConfigs = map[string]*readLimitTestConfig{ // and the truncated body. // maxReadSize > headerSize + bodySize > bodySize > maxBodySize "truncate_body": { - tls: false, - port: readLimitTestConfigHTTPBasePort + 1, - maxBodySize: 2048, - maxReadSize: 8192, - headerSize: 64, - bodySize: 4096, + tls: false, + port: readLimitTestConfigHTTPBasePort + 1, + maxBodySize: 2048, + maxReadSize: 8192, + headerSize: 64, + bodySize: 4096, expectedStatus: zgrab2.SCAN_SUCCESS, }, "tls_truncate_body": { - tls: true, - port: readLimitTestConfigHTTPSBasePort + 1, - maxBodySize: 2048, - maxReadSize: 8192, - headerSize: 64, - bodySize: 4096, + tls: true, + port: readLimitTestConfigHTTPSBasePort + 1, + maxBodySize: 2048, + maxReadSize: 8192, + headerSize: 64, + bodySize: 4096, expectedStatus: zgrab2.SCAN_SUCCESS, }, - // The socket truncates the connection while reading the headers. The result isn't a valid HTTP - // response, so the library returns an unexpected EOF error. + // The socket truncates the connection while reading the headers. The result isn't a completely valid HTTP + // response, but we capture the output regardless // headerSize > maxReadSize "truncate_read_header": { - tls: false, - port: readLimitTestConfigHTTPBasePort + 2, - maxBodySize: 1024, - maxReadSize: 2048, - headerSize: 3072, - bodySize: 8, - expectedError: "unexpected EOF", - expectedStatus: zgrab2.SCAN_UNKNOWN_ERROR, + tls: false, + port: readLimitTestConfigHTTPBasePort + 2, + maxBodySize: 1024, + maxReadSize: 2048, + headerSize: 3072, + bodySize: 0, + expectedStatus: zgrab2.SCAN_SUCCESS, }, "tls_truncate_read_header": { - tls: true, - port: readLimitTestConfigHTTPSBasePort + 2, - maxBodySize: 1024, - maxReadSize: 2048, - headerSize: 3072, - bodySize: 8, - expectedError: "unexpected EOF", + tls: true, + port: readLimitTestConfigHTTPSBasePort + 2, + maxBodySize: 1024, + maxReadSize: 2048, + headerSize: 3072, + bodySize: 0, + expectedStatus: zgrab2.SCAN_SUCCESS, + }, + + // The socket truncates the connection while reading the status code. The result isn't a valid HTTP + // response + // headerSize > maxReadSize + "invalid_status_code": { + tls: false, + port: readLimitTestConfigHTTPBasePort + 2, + maxBodySize: 8192, + maxReadSize: 8192, + headerSize: 1024, + bodySize: 1024, + customHeader: adr("HTTP/1.0 200"), + expectedError: "malformed HTTP status code", + expectedStatus: zgrab2.SCAN_UNKNOWN_ERROR, + }, + "tls_invalid_status_code": { + tls: true, + port: readLimitTestConfigHTTPSBasePort + 2, + maxBodySize: 8192, + maxReadSize: 8192, + headerSize: 1024, + bodySize: 1024, + customHeader: adr("HTTP/1.0 200"), + expectedError: "malformed HTTP status code", + expectedStatus: zgrab2.SCAN_UNKNOWN_ERROR, + }, + + "invalid_no_status": { + tls: false, + port: readLimitTestConfigHTTPBasePort + 2, + maxBodySize: 8192, + maxReadSize: 8192, + headerSize: 1024, + bodySize: 1024, + customHeader: adr(""), + customSuffix: adr(""), + expectedError: "malformed HTTP response", + expectedStatus: zgrab2.SCAN_UNKNOWN_ERROR, + }, + + "invalid_response": { + tls: false, + port: readLimitTestConfigHTTPBasePort + 2, + maxBodySize: 8192, + maxReadSize: 8192, + headerSize: 1024, + bodySize: 1024, + customHeader: adr(""), + expectedError: "malformed HTTP response", + expectedStatus: zgrab2.SCAN_UNKNOWN_ERROR, + }, + + "invalid_low_read_limit": { + tls: false, + port: readLimitTestConfigHTTPBasePort + 2, + maxBodySize: 8192, + maxReadSize: 1, + headerSize: 1024, + bodySize: 1024, + expectedError: "malformed HTTP response", expectedStatus: zgrab2.SCAN_UNKNOWN_ERROR, }, // Happy case. None of the limits are hit. // maxReadSize >= maxBodySize > bodySize + headerSize "happy_case": { - tls: false, - port: readLimitTestConfigHTTPBasePort + 3, - maxBodySize: 8192, - maxReadSize: 8192, - headerSize: 1024, - bodySize: 1024, + tls: false, + port: readLimitTestConfigHTTPBasePort + 3, + maxBodySize: 8192, + maxReadSize: 8192, + headerSize: 1024, + bodySize: 1024, expectedStatus: zgrab2.SCAN_SUCCESS, }, "tls_happy_case": { - tls: true, - port: readLimitTestConfigHTTPSBasePort + 3, - maxBodySize: 8192, - maxReadSize: 8192, - headerSize: 1024, - bodySize: 1024, + tls: true, + port: readLimitTestConfigHTTPSBasePort + 3, + maxBodySize: 8192, + maxReadSize: 8192, + headerSize: 1024, + bodySize: 1024, expectedStatus: zgrab2.SCAN_SUCCESS, }, } // Try to get the HTTP body from a result; otherwise return the empty string. -func getBody(result interface{}) string { +func getResponse(result interface{}) *http.Response { if result == nil { - return "" + return nil } httpResult, ok := result.(*Results) if !ok { - return "" - } - response := httpResult.Response - if response == nil { - return "" + return nil } - return response.BodyText + return httpResult.Response } // Run a single test with the given configuration. @@ -307,31 +377,43 @@ func (cfg *readLimitTestConfig) runTest(t *testing.T, testName string) { IP: net.ParseIP("127.0.0.1"), } status, ret, err := scanner.Scan(target) + response := getResponse(ret) if status != cfg.expectedStatus { - t.Errorf("Wrong status: expected %s, got %s", cfg.expectedStatus, status) + t.Errorf("Wrong status: expected %s, got %s with %+v", cfg.expectedStatus, status, response) } if err != nil { if !strings.Contains(err.Error(), cfg.expectedError) { - t.Errorf("Wrong error: expected %s, got %s", err.Error(), cfg.expectedError) + t.Errorf("Wrong error: expected %s, got %s", cfg.expectedError, err.Error()) } } else if len(cfg.expectedError) > 0 { t.Errorf("Expected error '%s' but got none", cfg.expectedError) } if cfg.expectedStatus == zgrab2.SCAN_SUCCESS { - body := getBody(ret) - if body == "" { - t.Errorf("Expected success, but got no body") - } else { - if len(body) > cfg.maxBodySize || len(body) > cfg.maxReadSize { - t.Errorf("Body exceeds max size: len(body)=%d; maxBodySize=%d, maxReadSize=%d", len(body), cfg.maxBodySize, cfg.maxReadSize) - } - if !cfg.tls { - if len(body)+cfg.headerSize > cfg.maxReadSize { - t.Errorf("Body and header exceed max read size: len(body)=%d, headerSize=%d, maxReadSize=%d", len(body), cfg.headerSize, cfg.maxReadSize) - } - } + if response == nil { + t.Errorf("Expected response, but got none") + } + + statusCode := response.Status + if statusCode != "200 OK" { + t.Errorf("Expected status %s, but got %s", "200 OK", statusCode) + } + + body := response.BodyText + if body == "" { + if cfg.bodySize != 0 { + t.Errorf("Expected success, but got no body") + } + } else { + if len(body) > cfg.maxBodySize || len(body) > cfg.maxReadSize { + t.Errorf("Body exceeds max size: len(body)=%d; maxBodySize=%d, maxReadSize=%d", len(body), cfg.maxBodySize, cfg.maxReadSize) + } + if !cfg.tls { + if len(body)+cfg.headerSize > cfg.maxReadSize { + t.Errorf("Body and header exceed max read size: len(body)=%d, headerSize=%d, maxReadSize=%d", len(body), cfg.headerSize, cfg.maxReadSize) + } } + } } }