diff --git a/.github/workflows/apisix-test.yml b/.github/workflows/apisix-test.yml index 978b8fa0..f10b6ace 100644 --- a/.github/workflows/apisix-test.yml +++ b/.github/workflows/apisix-test.yml @@ -21,9 +21,7 @@ jobs: - name: Install CPAN run: | - curl -s -L http://xrl.us/cpanm > ../cpanm - chmod +x ../cpanm - sudo mv ../cpanm /bin/cpanm + sudo apt install -y cpanminus - name: Install Test::Nginx run: | diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 21f03e3f..d624a1c8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -7,11 +7,11 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 - - uses: leafo/gh-actions-lua@v8 - with: - luaVersion: "luajit-openresty" - - uses: leafo/gh-actions-luarocks@v4 - - run: luarocks install luacheck + - name: Install LuaRocks + run: | + sudo apt -y install lua5.1 liblua5.1-0-dev + curl -fsSL https://raw.githubusercontent.com/apache/apisix/master/utils/linux-install-luarocks.sh | bash + - run: sudo luarocks install luacheck - run: luacheck lib run_tests: @@ -22,22 +22,22 @@ jobs: - 1.19.3.1 runs-on: ubuntu-latest - container: - image: openresty/openresty:${{ matrix.openresty_version }}-alpine-fat - # --init runs tinit as PID 1 and prevents the 'WARNING: killing the child process' spam from the test suite - options: --init steps: - name: Install deps run: | - apk add --no-cache curl perl bash wget git perl-dev libarchive-tools - ln -s /usr/bin/bsdtar /usr/bin/tar + wget -O - https://openresty.org/package/pubkey.gpg | sudo apt-key add - + echo "deb http://openresty.org/package/ubuntu $(lsb_release -sc) main" | sudo tee /etc/apt/sources.list.d/openresty.list + sudo apt-get update + sudo apt-get -y install openresty + sudo apt-get -y install curl perl bash wget git libperl-dev libarchive-tools + curl -fsSL https://raw.githubusercontent.com/apache/apisix/master/utils/linux-install-luarocks.sh | bash - name: Install CPAN - run: curl -s -L http://xrl.us/cpanm > /bin/cpanm && chmod +x /bin/cpanm + run: sudo apt install -y cpanminus - name: Cache - uses: actions/cache@v2 + uses: actions/cache@v4 with: path: | ~/.cpan @@ -45,17 +45,19 @@ jobs: key: ${{ runner.os }}-${{ matrix.openresty_version }}-cache - name: Install Test::Nginx - run: cpanm -q -n Test::Nginx + run: sudo cpanm -q -n Test::Nginx - name: Install Luacov - run: luarocks install luacov + run: sudo luarocks install luacov - uses: actions/checkout@v2 - name: Run tests env: TEST_COVERAGE: '1' - run: /usr/bin/prove -I../test-nginx/lib t/*.t + run: | + export PATH=/usr/local/openresty/nginx/sbin:/usr/local/openresty/luajit/bin:$PATH + /usr/bin/prove -I../test-nginx/lib t/*.t - name: Coverage run: | diff --git a/lib/resty/http.lua b/lib/resty/http.lua index 0fffb431..9983c735 100644 --- a/lib/resty/http.lua +++ b/lib/resty/http.lua @@ -10,6 +10,7 @@ local str_lower = string.lower local str_upper = string.upper local str_find = string.find local str_sub = string.sub +local str_byte = string.byte local tbl_concat = table.concat local tbl_insert = table.insert local ngx_encode_args = ngx.encode_args @@ -128,6 +129,99 @@ local DEFAULT_PARAMS = { local DEBUG = false +-- ideated from: +-- luacheck: push max code line length 300 +-- https://github.com/hyperium/http/blob/60fbf319500def124cabb21c8fe8533cf209ce58/src/uri/path.rs#L484-L540 +-- luacheck: pop +local is_allowed_path = {} +for b = 0, 255 do + is_allowed_path[b] = (b == 0x21 -- ! + or (b >= 0x24 and b <= 0x3B) -- $, %, &, ', (, ), *, +, ,, -, ., /, 0-9, :, ; + or b == 0x3D -- = + or (b >= 0x40 and b <= 0x5F) -- @, A-Z, [, \, ], ^, _ + or (b >= 0x61 and b <= 0x7A) -- a-z + or b == 0x7C -- | + or b == 0x7E -- ~ + or b == 34 -- " + or b == 123 -- { + or b == 125 -- } + or b >= 127) -- UTF-8 / DEL +end + +local function validate_path(value) + if type(value) ~= "string" then return false end + + for i = 1, #value do + if not is_allowed_path[str_byte(value, i)] then + return false + end + end + return true +end + +local is_allowed_query = {} +for b = 0, 255 do + is_allowed_query[b] = is_allowed_path[b] +end +-- Query logic matches Path logic, but adds 0x3F (?) +is_allowed_query[0x3F] = true + +local function validate_query(value) + if type(value) ~= "string" then return false end + + for i = 1, #value do + if not is_allowed_query[str_byte(value, i)] then + return false + end + end + return true +end + +-- Pre-compute the Token table (based on Go's httpguts.isTokenTable) +-- https://github.com/golang/net/blob/60b3f6f8ce12def82ae597aebe9031753198f74d/http/httpguts/httplex.go#L15-L93 +local is_token_char = {} +local tokens = "!#$%&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~" + +for i = 1, #tokens do + is_token_char[str_byte(tokens, i)] = true +end + +local function validate_header_name(name) + if type(name) ~= "string" or name == "" then return false end + + for i = 1, #name do + if not is_token_char[str_byte(name, i)] then + return false + end + end + return true +end + +-- Pre-compute the Header Value table +local is_valid_header_value_char = {} +for b = 0, 255 do + -- Logic: Not a CTL, OR is LWS (Space 32 or Tab 9) + -- isCTL: b < 32 or b == 127 + -- isLWS: b == 32 or b == 9 + local is_ctl = (b < 32 or b == 127) + local is_lws = (b == 32 or b == 9) + + if not is_ctl or is_lws then + is_valid_header_value_char[b] = true + else + is_valid_header_value_char[b] = false + end +end + +local function validate_header_value(value) + if type(value) ~= "string" then return false end + for i = 1, #value do + if not is_valid_header_value_char[str_byte(value, i)] then + return false + end + end + return true +end function _M.new(_) local sock, err = ngx_socket_tcp() @@ -311,6 +405,11 @@ local function _format_request(self, params) local version = params.version local headers = params.headers or {} + local path = params.path + if not validate_path(path) then + return nil, "invalid characters found in path" + end + local query = params.query or "" if type(query) == "table" then query = "?" .. ngx_encode_args(query) @@ -318,12 +417,16 @@ local function _format_request(self, params) query = "?" .. query end + if not validate_query(query) then + return nil, "invalid characters found in query" + end + -- Initialize request local req = { str_upper(params.method), " ", self.path_prefix or "", - params.path, + path, query, HTTP[version], -- Pre-allocate slots for minimum headers and carriage return. @@ -336,14 +439,25 @@ local function _format_request(self, params) -- Append headers for key, values in pairs(headers) do key = tostring(key) + if not validate_header_name(key) then + return nil, "invalid characters found in header key" + end if type(values) == "table" then for _, value in pairs(values) do + value = tostring(value) + if not validate_header_value(value) then + return nil, "invalid characters found in header value" + end req[c] = key .. ": " .. tostring(value) .. "\r\n" c = c + 1 end else + values = tostring(values) + if not validate_header_value(values) then + return nil, "invalid characters found in header value" + end req[c] = key .. ": " .. tostring(values) .. "\r\n" c = c + 1 end @@ -736,7 +850,11 @@ function _M.send_request(self, params) params.headers = headers -- Format and send request - local req = _format_request(self, params) + local req, err = _format_request(self, params) + if not req then + return nil, err + end + if DEBUG then ngx_log(ngx_DEBUG, "\n", req) end local bytes, err = sock:send(req) diff --git a/t/21-character-validation.t b/t/21-character-validation.t new file mode 100644 index 00000000..47f82848 --- /dev/null +++ b/t/21-character-validation.t @@ -0,0 +1,494 @@ +use Test::Nginx::Socket 'no_plan'; +use Cwd qw(cwd); + +my $pwd = cwd(); + +$ENV{TEST_NGINX_RESOLVER} = '8.8.8.8'; +$ENV{TEST_COVERAGE} ||= 0; + +our $HttpConfig = qq{ + lua_package_path "$pwd/lib/?.lua;/usr/local/share/lua/5.1/?.lua;;"; + error_log logs/error.log debug; + + init_by_lua_block { + if $ENV{TEST_COVERAGE} == 1 then + jit.off() + require("luacov.runner").init() + end + } +}; + +no_long_string(); + +run_tests(); + +__DATA__ + +=== TEST 1: Rejects header injection in path +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/b\\r\\nInjected: true" + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } +--- request +GET /a +--- response_body +invalid characters found in path +--- no_error_log +[error] + + + +=== TEST 2: Rejects header injection in query +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/b", + query = "key=value\\r\\nInjected: true" + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } +--- request +GET /a +--- response_body +invalid characters found in query +--- no_error_log +[error] + + + +=== TEST 3: Rejects header injection in header key +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/b", + headers = { + ["Test-Header\\r\\nInjected"] = "value" + } + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } +--- request +GET /a +--- response_body +invalid characters found in header key +--- no_error_log +[error] + + + +=== TEST 4: Rejects header injection in header value +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/b", + headers = { + ["Test-Header"] = "value\\r\\nInjected: true" + } + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } +--- request +GET /a +--- response_body +invalid characters found in header value +--- no_error_log +[error] + + + +=== TEST 5: Allows normal requests with valid spaces and tabs in headers +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/b", + headers = { + ["Test-Header"] = "value \\t something" + } + } + + if not res then + ngx.say(err) + else + ngx.status = res.status + ngx.say(res.headers["Test-Header"]) + end + + httpc:close() + '; + } + location = /b { + content_by_lua ' + ngx.header["Test-Header"] = ngx.req.get_headers()["Test-Header"] + ngx.say("OK") + '; + } +--- request +GET /a +--- response_body +value something +--- no_error_log +[error] + + + +=== TEST 6: Rejects spaces in path +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/foo bar" + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } +--- request +GET /a +--- response_body +invalid characters found in path +--- no_error_log +[error] + + + +=== TEST 7: Rejects spaces in query literal +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/b", + query = "key=value with space" + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } +--- request +GET /a +--- response_body +invalid characters found in query +--- no_error_log +[error] + + + +=== TEST 8: Rejects invalid characters in header key +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/b", + headers = { + ["Test:Header"] = "value" + } + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } +--- request +GET /a +--- response_body +invalid characters found in header key +--- no_error_log +[error] + + + +=== TEST 9: Allows non-English characters (UTF-8) in path +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/path/你好" + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } + location /path/ { + echo "OK"; + } +--- request +GET /a +--- response_body +OK +--- no_error_log +[error] + + + +=== TEST 10: Allows non-English characters (UTF-8) in query +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/b", + query = "key=你好" + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } + location = /b { + echo "OK"; + } +--- request +GET /a +--- response_body +OK +--- no_error_log +[error] + + + +=== TEST 11: Allows non-English characters (UTF-8) in header value +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/b", + headers = { + ["X-Test"] = "你好" + } + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } + location = /b { + echo "OK"; + } +--- request +GET /a +--- response_body +OK +--- no_error_log +[error] + + + +=== TEST 12: Allows exotic but valid characters in path and query +--- http_config eval: $::HttpConfig +--- config + location = /a { + content_by_lua ' + local http = require "resty.http" + local httpc = http.new() + httpc:connect{ + scheme = "http", + host = "127.0.0.1", + port = ngx.var.server_port + } + + local res, err = httpc:request{ + method = "GET", + path = "/_a-Z~.!$&*+,;=:@", + query = "k=_a-Z~.!$&*+,;=:@" + } + + if not res then + ngx.say(err) + else + ngx.say("OK") + end + + httpc:close() + '; + } + location /_a-Z { + echo "OK"; + } +--- request +GET /a +--- response_body +OK +--- no_error_log +[error] +