From 0f3798bc71f162c212f1877fd1712d5d04f5460d Mon Sep 17 00:00:00 2001
From: samugi <samuele@konghq.com>
Date: Thu, 16 Feb 2023 16:55:26 +0100
Subject: [PATCH 1/2] feat(request-transformer): untrusted_lua sandbox

Return error when untrusted_lua is 'off' and lua expressions are
detected
Execute only sandbox-allowed code when untrusted_lua = 'on'
Execute with no restriction when untrusted_lua = 'sandbox'
---
 kong/plugins/request-transformer/access.lua   |  30 +-
 .../36-request-transformer/02-access_spec.lua | 427 ++++++++++++++++++
 2 files changed, 452 insertions(+), 5 deletions(-)

diff --git a/kong/plugins/request-transformer/access.lua b/kong/plugins/request-transformer/access.lua
index f0772995025..f63d6fc6414 100644
--- a/kong/plugins/request-transformer/access.lua
+++ b/kong/plugins/request-transformer/access.lua
@@ -2,6 +2,7 @@ local multipart = require "multipart"
 local cjson = require("cjson.safe").new()
 local pl_template = require "pl.template"
 local pl_tablex = require "pl.tablex"
+local sandbox = require "kong.tools.sandbox"
 
 local table_insert = table.insert
 local get_uri_args = kong.request.get_query
@@ -23,6 +24,8 @@ local pairs = pairs
 local error = error
 local rawset = rawset
 local pl_copy_table = pl_tablex.deepcopy
+local lua_enabled = sandbox.configuration.enabled
+local sandbox_enabled = sandbox.configuration.sandbox_enabled
 
 local _M = {}
 local template_cache = setmetatable( {}, { __mode = "k" })
@@ -74,6 +77,17 @@ local function param_value(source_template, config_array, template_env)
     return nil
   end
 
+  if not lua_enabled then
+    -- Detect expressions in the source template
+    local expr = str_find(source_template, "%$%(.*%)")
+    if expr then
+      return nil, "loading of untrusted Lua code disabled because " ..
+                  "'untrusted_lua' config option is set to 'off'"
+    end
+    -- Lua is disabled, no need to render the template
+    return source_template
+  end
+
   -- find compiled templates for this plugin-configuration array
   local compiled_templates = template_cache[config_array]
   if not compiled_templates then
@@ -498,7 +512,9 @@ function _M.execute(conf)
       }
       local loader = lazy_loaders[key]
       if not loader then
-        -- we don't have a loader, so just return nothing
+        if lua_enabled and not sandbox_enabled then
+          return _G[key]
+        end
         return
       end
       -- set the result on the table to not load again
@@ -511,13 +527,17 @@ function _M.execute(conf)
     end,
   }
 
-  local template_env = setmetatable({
+  local template_env = {}
+  if lua_enabled and sandbox_enabled then
+    -- load the sandbox environment to be used to render the template
+    template_env = pl_copy_table(sandbox.configuration.environment)
     -- here we can optionally add functions to expose to the sandbox, eg:
-    -- tostring = tostring,  -- for example
+    -- tostring = tostring,
     -- because headers may contain array elements such as duplicated headers
     -- type is a useful function in these cases. See issue #25.
-    type = type,
-  }, __meta_environment)
+    template_env.type = type
+  end
+  setmetatable(template_env, __meta_environment)
 
   transform_uri(conf, template_env)
   transform_method(conf)
diff --git a/spec/03-plugins/36-request-transformer/02-access_spec.lua b/spec/03-plugins/36-request-transformer/02-access_spec.lua
index 760964b9699..5579b5cb953 100644
--- a/spec/03-plugins/36-request-transformer/02-access_spec.lua
+++ b/spec/03-plugins/36-request-transformer/02-access_spec.lua
@@ -2228,6 +2228,433 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function()
   end)
 end)
 
+describe("Plugin: request-transformer(access) [#" .. strategy .. "] untrusted_lua=off", function()
+  local client
+
+  lazy_setup(function()
+    local bp = helpers.get_db_utils(strategy, {
+      "routes",
+      "services",
+      "plugins",
+    })
+
+    local route1 = bp.routes:insert({
+      hosts = { "testLuaOff.test" }
+    })
+
+    local route2 = bp.routes:insert({
+      hosts = { "testLuaOff2.test" }
+    })
+
+    bp.plugins:insert {
+      route = { id = route1.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          headers = {"x-added:some_value"},
+        }
+      }
+    }
+
+    bp.plugins:insert {
+      route = { id = route2.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          headers = {"x-added:$(require('inspect')(string.sub(query_params.user, 1, 3)))"},
+        }
+      }
+    }
+
+    assert(helpers.start_kong({
+      database = strategy,
+      untrusted_lua = "off",
+      plugins = "bundled, request-transformer",
+      nginx_conf = "spec/fixtures/custom_nginx.template",
+    }))
+  end)
+
+  lazy_teardown(function()
+    helpers.stop_kong()
+  end)
+
+  before_each(function()
+    client = helpers.proxy_client()
+  end)
+
+  after_each(function()
+    if client then client:close() end
+  end)
+
+
+  it("correctly handles plain text value", function()
+    local r = assert(client:send {
+      method = "GET",
+      path = "/",
+      headers = {
+        host = "testLuaOff.test",
+        ["Content-Type"] = "application/x-www-form-urlencoded",
+      }
+    })
+    assert.response(r).has.status(200)
+    local value = assert.request(r).has.header("x-added")
+    assert.equals("some_value", value)
+  end)
+
+  it("does not render lua expressions", function()
+    local pattern = [[loading of untrusted Lua code disabled because 'untrusted_lua' config option is set to 'off']]
+    local start_count = count_log_lines(pattern)
+    local r = assert(client:send {
+      method = "GET",
+      path = "/",
+      query = {
+        user = "foo"
+      },
+      headers = {
+        host = "testLuaOff2.test",
+        ["Content-Type"] = "application/x-www-form-urlencoded",
+      }
+    })
+    assert.response(r).has.status(500)
+    helpers.wait_until(function()
+      local count = count_log_lines(pattern)
+      return count - start_count >= 1
+    end, 5)
+  end)
+end)
+
+describe("Plugin: request-transformer(access) [#" .. strategy .. "] untrusted_lua=on", function()
+  local client
+
+  lazy_setup(function()
+    local bp = helpers.get_db_utils(strategy, {
+      "routes",
+      "services",
+      "plugins",
+    })
+
+    local route1 = bp.routes:insert({
+      hosts = { "testLuaOff1.test" },
+    })
+
+    bp.plugins:insert {
+      route = { id = route1.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          headers = {"x-added:$(require('inspect')(string.sub(query_params.user, 1, 3)))"},
+        }
+      }
+    }
+
+    assert(helpers.start_kong({
+      database = strategy,
+      untrusted_lua = "on",
+      plugins = "bundled, request-transformer",
+      nginx_conf = "spec/fixtures/custom_nginx.template",
+    }))
+  end)
+
+  lazy_teardown(function()
+    helpers.stop_kong()
+  end)
+
+  before_each(function()
+    client = helpers.proxy_client()
+  end)
+
+  after_each(function()
+    if client then client:close() end
+  end)
+
+  describe("using template", function()
+    it("renders the Lua expression without restrictions", function()
+      local r = assert(client:send {
+        method = "GET",
+        path = "/",
+        query = {
+          user = "foo123"
+        },
+        headers = {
+          host = "testLuaOff1.test",
+          ["Content-Type"] = "application/x-www-form-urlencoded",
+        }
+      })
+      assert.response(r).has.status(200)
+      local value = assert.request(r).has.header("x-added")
+      assert.equals('"foo"', value)
+    end)
+  end)
+end)
+
+describe("Plugin: request-transformer(access) [#" .. strategy .. "] untrusted_lua=sandbox", function()
+  local client
+
+  lazy_setup(function()
+    local bp = helpers.get_db_utils(strategy, {
+      "routes",
+      "services",
+      "plugins",
+    })
+
+    local route1 = bp.routes:insert({
+      hosts = { "testLuaSandbox1.test" },
+    })
+
+    local route2 = bp.routes:insert({
+      hosts = { "testLuaSandbox2.test" },
+    })
+
+    local route3 = bp.routes:insert({
+      hosts = { "testLuaSandbox3.test" },
+    })
+
+    bp.plugins:insert {
+      route = { id = route1.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          headers = {"x-added:$(type(query_params.user))"},
+        }
+      }
+    }
+
+    bp.plugins:insert {
+      route = { id = route2.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          headers = {"x-added:$(string.sub(query_params.user, 1, 3))"},
+        }
+      }
+    }
+
+    bp.plugins:insert {
+      route = { id = route3.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          headers = {"x-added:$(require('inspect')('somestring'))"},
+        }
+      }
+    }
+
+    assert(helpers.start_kong({
+      database = strategy,
+      untrusted_lua = "sandbox",
+      plugins = "bundled, request-transformer",
+      nginx_conf = "spec/fixtures/custom_nginx.template",
+    }))
+  end)
+
+  lazy_teardown(function()
+    helpers.stop_kong()
+  end)
+
+  before_each(function()
+    client = helpers.proxy_client()
+  end)
+
+  after_each(function()
+    if client then client:close() end
+  end)
+
+  describe("using template", function()
+    it("should succeed when template accesses allowed Lua function from sandbox", function()
+      local r = assert(client:send {
+        method = "GET",
+        path = "/",
+        query = {
+          user = "foo"
+        },
+        headers = {
+          host = "testLuaSandbox1.test",
+          ["Content-Type"] = "application/x-www-form-urlencoded",
+        }
+      })
+      assert.response(r).has.status(200)
+      local value = assert.request(r).has.header("x-added")
+      assert.equals("string", value)
+    end)
+
+    it("should fail when template tries to access non allowed Lua function from sandbox", function()
+      local pattern = [[attempt to index global 'string' %(a nil value%)]]
+      local start_count = count_log_lines(pattern)
+      local r = assert(client:send {
+        method = "GET",
+        path = "/",
+        query = {
+          user = "foo"
+        },
+        headers = {
+          host = "testLuaSandbox2.test",
+          ["Content-Type"] = "application/x-www-form-urlencoded",
+        }
+      })
+      assert.response(r).has.status(500)
+      helpers.wait_until(function()
+        local count = count_log_lines(pattern)
+        return count - start_count >= 1
+      end, 5)
+    end)
+
+    it("should fail when template tries to require non whitelisted module from sandbox", function()
+      local pattern = [[require 'inspect' not allowed within sandbox]]
+      local start_count = count_log_lines(pattern)
+
+      local r = assert(client:send {
+        method = "GET",
+        path = "/",
+        query = {
+          user = "foo"
+        },
+        headers = {
+          host = "testLuaSandbox3.test",
+          ["Content-Type"] = "application/x-www-form-urlencoded",
+        }
+      })
+      assert.response(r).has.status(500)
+
+      helpers.wait_until(function()
+        local count = count_log_lines(pattern)
+        return count - start_count >= 1
+      end, 5)
+    end)
+  end)
+end)
+
+describe("Plugin: request-transformer(access) [#" .. strategy .. "] untrusted_lua_sandbox_requires", function()
+  local client
+
+  lazy_setup(function()
+    local bp = helpers.get_db_utils(strategy, {
+      "routes",
+      "services",
+      "plugins",
+    })
+
+    local route1 = bp.routes:insert({
+      hosts = { "testLuaRequires.test" },
+    })
+
+    bp.plugins:insert {
+      route = { id = route1.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          headers = {"x-added:$(require('inspect')({query_params.user1,query_params.user2}))"},
+        }
+      }
+    }
+
+    assert(helpers.start_kong({
+      database = strategy,
+      untrusted_lua = "sandbox",
+      untrusted_lua_sandbox_requires =  "inspect",
+      plugins = "bundled, request-transformer",
+      nginx_conf = "spec/fixtures/custom_nginx.template",
+    }))
+  end)
+
+  lazy_teardown(function()
+    helpers.stop_kong()
+  end)
+
+  before_each(function()
+    client = helpers.proxy_client()
+  end)
+
+  after_each(function()
+    if client then client:close() end
+  end)
+
+  describe("using template", function()
+    it("should successfully require whitelisted module from sandbox", function()
+      local r = assert(client:send {
+        method = "GET",
+        path = "/",
+        query = {
+          user1 = "foo",
+          user2 = "bar",
+        },
+        headers = {
+          host = "testLuaRequires.test",
+          ["Content-Type"] = "application/x-www-form-urlencoded",
+        }
+      })
+      assert.response(r).has.status(200)
+      local value = assert.request(r).has.header("x-added")
+      assert.equals('{ "foo", "bar" }', value)
+    end)
+  end)
+end)
+
+describe("Plugin: request-transformer(access) [#" .. strategy .. "] untrusted_lua_sandbox_environment", function()
+  local client
+
+  lazy_setup(function()
+    local bp = helpers.get_db_utils(strategy, {
+      "routes",
+      "services",
+      "plugins",
+    })
+
+    local route1 = bp.routes:insert({
+      hosts = { "testLuaRequires.test" },
+    })
+
+    bp.plugins:insert {
+      route = { id = route1.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          headers = {"x-added:$(string.format('u1:%s;u2:%s', query_params.user1,query_params.user2))"},
+        }
+      }
+    }
+
+    assert(helpers.start_kong({
+      database = strategy,
+      untrusted_lua = "sandbox",
+      untrusted_lua_sandbox_environment =  "string",
+      plugins = "bundled, request-transformer",
+      nginx_conf = "spec/fixtures/custom_nginx.template",
+    }))
+  end)
+
+  lazy_teardown(function()
+    helpers.stop_kong()
+  end)
+
+  before_each(function()
+    client = helpers.proxy_client()
+  end)
+
+  after_each(function()
+    if client then client:close() end
+  end)
+
+  describe("using template", function()
+    it("should successfully access whitelisted Lua variables from sandbox", function()
+      local r = assert(client:send {
+        method = "GET",
+        path = "/",
+        query = {
+          user1 = "foo",
+          user2 = "bar",
+        },
+        headers = {
+          host = "testLuaRequires.test",
+          ["Content-Type"] = "application/x-www-form-urlencoded",
+        }
+      })
+      assert.response(r).has.status(200)
+      local value = assert.request(r).has.header("x-added")
+      assert.equals('u1:foo;u2:bar', value)
+    end)
+  end)
+end)
+
 describe("Plugin: request-transformer (thread safety) [#" .. strategy .. "]", function()
   local db_strategy = strategy ~= "off" and strategy or nil
 

From 086ce4f75c618740948bbd37ee84ce67bca7d0a9 Mon Sep 17 00:00:00 2001
From: samugi <samuele@konghq.com>
Date: Mon, 27 Feb 2023 12:30:47 +0100
Subject: [PATCH 2/2] chore(*): changelog

---
 CHANGELOG.md                                  |   2 +
 .../36-request-transformer/02-access_spec.lua | 220 +++++++++---------
 2 files changed, 112 insertions(+), 110 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1f280c14b4e..5b0928b6b42 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -143,6 +143,8 @@
 - **Request-Transformer**: fix an issue where requests would intermittently
   be proxied with incorrect query parameters.
   [10539](https://github.com/Kong/kong/pull/10539)
+- **Request Transformer**: honor value of untrusted_lua configuration parameter
+  [#10327](https://github.com/Kong/kong/pull/10327)
 
 ### Changed
 
diff --git a/spec/03-plugins/36-request-transformer/02-access_spec.lua b/spec/03-plugins/36-request-transformer/02-access_spec.lua
index 5579b5cb953..76687101d62 100644
--- a/spec/03-plugins/36-request-transformer/02-access_spec.lua
+++ b/spec/03-plugins/36-request-transformer/02-access_spec.lua
@@ -2228,6 +2228,116 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function()
   end)
 end)
 
+describe("Plugin: request-transformer (thread safety) [#" .. strategy .. "]", function()
+  local db_strategy = strategy ~= "off" and strategy or nil
+
+  lazy_setup(function()
+    local bp = helpers.get_db_utils(db_strategy, {
+      "routes",
+      "services",
+      "plugins",
+    }, { "request-transformer", "pre-function" })
+
+    local route = bp.routes:insert({
+      hosts = { "test_thread_safety.test" }
+    })
+
+    bp.plugins:insert {
+      route = { id = route.id },
+      name = "pre-function",
+      config = {
+        access = {
+          [[
+            local delay = kong.request.get_header("slow_body_delay")
+            local orig_read_body = ngx.req.read_body
+            ngx.ctx.orig_read_body = orig_read_body
+            ngx.req.read_body = function()
+              ngx.sleep(tonumber(delay))
+              return orig_read_body()
+            end
+          ]]
+        },
+        header_filter = {
+          [[
+            ngx.req.read_body = ngx.ctx.orig_read_body or ngx.req.read_body
+          ]]
+        },
+      }
+    }
+
+    bp.plugins:insert {
+      route = { id = route.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          querystring = { "added_q:yes_q" },
+          headers = { "added_h:yes_h" },
+          body = { "added_b:yes_b" }
+        }
+      }
+    }
+
+    assert(helpers.start_kong({
+      database = db_strategy,
+      plugins = "bundled, request-transformer",
+      nginx_conf = "spec/fixtures/custom_nginx.template",
+      nginx_worker_processes = 1
+    }))
+  end)
+
+  lazy_teardown(function()
+    helpers.stop_kong()
+  end)
+
+  it("sends requests with the expected values for headers, body, query", function()
+    local race_conditions = ""
+
+    local get_handler = function(header_val, body_param_val, query_param_val, delay)
+      return function()
+        local tmp_client = helpers.proxy_client()
+        local r = assert(tmp_client:send({
+          method = "POST",
+          path = "/request",
+          headers = {
+            ["Content-Type"] = "application/json",
+            host = "test_thread_safety.test",
+            slow_body_delay = delay,
+            h = header_val
+          },
+          body = {
+            k = body_param_val
+          },
+          query = {
+            q = query_param_val
+          }
+        }))
+
+        assert.response(r).has.status(200)
+        local header = assert.request(r).has.header("h")
+        local body_param = assert.request(r).has.jsonbody().params.k
+        local query_param = assert.request(r).has.queryparam("q")
+        if header_val ~= header then
+          race_conditions = race_conditions .. fmt("expected: %s, received: %s", header_val, header)
+        end
+        if body_param_val ~= body_param then
+          race_conditions = race_conditions .. fmt("expected: %s, received: %s", body_param_val, body_param)
+        end
+        if query_param ~= query_param_val then
+          race_conditions = race_conditions .. fmt("expected: %s, received: %s", query_param, query_param_val)
+        end
+        tmp_client:close()
+      end
+    end
+
+    local thread_1 = ngx.thread.spawn(get_handler("vh1", "b1", "vq1", 2))
+    local thread_2 = ngx.thread.spawn(get_handler("vh2", "b2", "vq2", 0))
+    ngx.thread.wait(thread_1)
+    ngx.thread.wait(thread_2)
+
+    assert.equals("", race_conditions)
+  end)
+end)
+
 describe("Plugin: request-transformer(access) [#" .. strategy .. "] untrusted_lua=off", function()
   local client
 
@@ -2654,114 +2764,4 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "] untrusted_lu
     end)
   end)
 end)
-
-describe("Plugin: request-transformer (thread safety) [#" .. strategy .. "]", function()
-  local db_strategy = strategy ~= "off" and strategy or nil
-
-  lazy_setup(function()
-    local bp = helpers.get_db_utils(db_strategy, {
-      "routes",
-      "services",
-      "plugins",
-    }, { "request-transformer", "pre-function" })
-
-    local route = bp.routes:insert({
-      hosts = { "test_thread_safety.test" }
-    })
-
-    bp.plugins:insert {
-      route = { id = route.id },
-      name = "pre-function",
-      config = {
-        access = {
-          [[
-            local delay = kong.request.get_header("slow_body_delay")
-            local orig_read_body = ngx.req.read_body
-            ngx.ctx.orig_read_body = orig_read_body
-            ngx.req.read_body = function()
-              ngx.sleep(tonumber(delay))
-              return orig_read_body()
-            end
-          ]]
-        },
-        header_filter = {
-          [[
-            ngx.req.read_body = ngx.ctx.orig_read_body or ngx.req.read_body
-          ]]
-        },
-      }
-    }
-
-    bp.plugins:insert {
-      route = { id = route.id },
-      name = "request-transformer",
-      config = {
-        add = {
-          querystring = { "added_q:yes_q" },
-          headers = { "added_h:yes_h" },
-          body = { "added_b:yes_b" }
-        }
-      }
-    }
-
-    assert(helpers.start_kong({
-      database = db_strategy,
-      plugins = "bundled, request-transformer",
-      nginx_conf = "spec/fixtures/custom_nginx.template",
-      nginx_worker_processes = 1
-    }))
-  end)
-
-  lazy_teardown(function()
-    helpers.stop_kong()
-  end)
-
-  it("sends requests with the expected values for headers, body, query", function()
-    local race_conditions = ""
-
-    local get_handler = function(header_val, body_param_val, query_param_val, delay)
-      return function()
-        local tmp_client = helpers.proxy_client()
-        local r = assert(tmp_client:send({
-          method = "POST",
-          path = "/request",
-          headers = {
-            ["Content-Type"] = "application/json",
-            host = "test_thread_safety.test",
-            slow_body_delay = delay,
-            h = header_val
-          },
-          body = {
-            k = body_param_val
-          },
-          query = {
-            q = query_param_val
-          }
-        }))
-
-        assert.response(r).has.status(200)
-        local header = assert.request(r).has.header("h")
-        local body_param = assert.request(r).has.jsonbody().params.k
-        local query_param = assert.request(r).has.queryparam("q")
-        if header_val ~= header then
-          race_conditions = race_conditions .. fmt("expected: %s, received: %s", header_val, header)
-        end
-        if body_param_val ~= body_param then
-          race_conditions = race_conditions .. fmt("expected: %s, received: %s", body_param_val, body_param)
-        end
-        if query_param ~= query_param_val then
-          race_conditions = race_conditions .. fmt("expected: %s, received: %s", query_param, query_param_val)
-        end
-        tmp_client:close()
-      end
-    end
-
-    local thread_1 = ngx.thread.spawn(get_handler("vh1", "b1", "vq1", 2))
-    local thread_2 = ngx.thread.spawn(get_handler("vh2", "b2", "vq2", 0))
-    ngx.thread.wait(thread_1)
-    ngx.thread.wait(thread_2)
-
-    assert.equals("", race_conditions)
-  end)
-end)
 end