Fix bug: arbitrary file read in log manager & remove block io ops

src/log-manager/build/log-manager-nginx.k8s.dockerfile

@@ -18,7 +18,7 @@
 FROM openresty/openresty:1.15.8.3-2-alpine-fat
 
 RUN luarocks install lua-cjson && luarocks install lua-resty-jwt && \
-  luarocks install luafilesystem
+  luarocks install luafilesystem && luarocks install lua-path
 
 COPY src/nginx/nginx.conf.default /etc/nginx/conf.d/default.conf
 COPY src/nginx/nginx.conf /usr/local/openresty/nginx/conf/nginx.conf

src/log-manager/src/nginx/get_log_content.lua

@@ -14,6 +14,10 @@
 
 
 local lfs = require "lfs"
+local path = require "path"
+
+local guard = require "guard"
+local util = require "util"
 
 local function get_rotated_log(log_path)
   for file in lfs.dir(log_path) do
@@ -24,30 +28,45 @@ local function get_rotated_log(log_path)
   end
 end
 
+guard.check_token()
 
 local args = ngx.req.get_uri_args()
 local username = args["username"]
 local framework_name = args["framework-name"]
 local taskrole = args["taskrole"]
 local pod_uid = args["pod-uid"]
-local token = args["token"]
 local tail_mode = args["tail-mode"]
 
-if not token or not username or not taskrole or not framework_name or not pod_uid then
+if not username or not taskrole or not framework_name or not pod_uid then
   ngx.log(ngx.ERR, "some query parameters is nil")
   ngx.status = ngx.HTTP_BAD_REQUEST
   return ngx.exit(ngx.HTTP_OK)
 end
 
-local path_prefix = "/usr/local/pai/logs/"..username.."/".. framework_name.."/".. taskrole.."/"..pod_uid.."/"
-local log_name = ngx.var[1]
+if not util.is_input_validated(username) or not util.is_input_validated(framework_name) or
+   not util.is_input_validated(taskrole) or not util.is_input_validated(pod_uid) then
+  ngx.log(ngx.ERR, "some query parameters is invalid")
+  ngx.status = ngx.HTTP_BAD_REQUEST
+  return ngx.exit(ngx.HTTP_OK)
+end
+
+local file_prefix = "/usr/local/pai/logs"
+local log_dir = file_prefix..path.normalize("/"..username)..
+  path.normalize("/"..framework_name)..path.normalize("/"..taskrole)..path.normalize("/"..pod_uid).."/"
 
-local log_path = path_prefix..log_name
+local log_name = path.normalize(ngx.var[1])
+if not util.is_input_validated(log_name) then
+  ngx.log(ngx.ERR, "log name is invalid")
+  ngx.status = ngx.HTTP_BAD_REQUEST
+  return ngx.exit(ngx.HTTP_OK)
+end
+
+local log_path = log_dir..log_name
 ngx.log(ngx.INFO, "get log name "..log_name)
 if string.match(log_name, "^user%-.*$") then
   -- we only keep one rotated log in log manager
   if string.match(log_name, "%.1$") then
-    local parent_path = path_prefix..string.sub(log_name, 1, string.len(log_name) - 2)
+    local parent_path = log_dir..string.sub(log_name, 1, string.len(log_name) - 2)
     local rotated_log_name = get_rotated_log(parent_path)
     if not rotated_log_name then
       ngx.status = ngx.HTTP_NOT_FOUND
@@ -56,29 +75,23 @@ if string.match(log_name, "^user%-.*$") then
       log_path = parent_path.."/"..rotated_log_name
     end
   else
-    log_path = path_prefix..log_name.."/current"
+    log_path = log_dir..log_name.."/current"
   end
 end
 
 ngx.log(ngx.INFO, "get log from path"..log_path)
 
-if lfs.attributes(log_path, "mode") ~= "file" then
+if not util.is_path_under_log_dir(log_path) or not path.isfile(log_path) then
   ngx.log(ngx.ERR, log_path.." not exists")
   ngx.status = ngx.HTTP_NOT_FOUND
   return ngx.exit(ngx.HTTP_OK)
 end
 
-local logs
 if (tail_mode == "true") then
-  logs = io.popen("tail -c 16k "..log_path)
-else
-  logs = io.popen("cat "..log_path)
+  ngx.req.set_header("Range", "bytes=-16384")
 end
 
--- buffer size (8K)
-local size = 2^13
-while true do
-  local block = logs:read(size)
-  if not block then break end
-  ngx.say(block)
-end
+-- Refer https://www.openwall.com/lists/oss-security/2020/03/18/1. set_uri may cause security issue.
+-- Here we need to make sure the log_path is valid
+ngx.req.set_uri("/~/"..string.sub(path.abspath(log_path), string.len(file_prefix) + 1), true)
+

src/log-manager/src/nginx/guard.lua

@@ -15,24 +15,30 @@
 local jwt = require "resty.jwt"
 local validators = require "resty.jwt-validators"
 
-local args = ngx.req.get_uri_args()
-local jwt_token = args["token"]
-if not jwt_token then
-  ngx.status = ngx.HTTP_FORBIDDEN
-  return ngx.exit(ngx.HTTP_OK)
-end
-
-local jwt_secret = os.getenv("JWT_SECRET")
-local node_name = os.getenv("NODE_NAME")
-
-local claim_spec = {
-  sub = validators.equals("log-manager-"..node_name),
-  exp = validators.is_not_expired()
-}
-local jwt_obj = jwt:verify(jwt_secret, jwt_token, claim_spec)
-
-if not jwt_obj["verified"] then
+local function check_token()
+  local args = ngx.req.get_uri_args()
+  local jwt_token = args["token"]
+  if not jwt_token then
     ngx.status = ngx.HTTP_FORBIDDEN
-    ngx.header["Access-Control-Allow-Origin"] = "*";
     return ngx.exit(ngx.HTTP_OK)
+  end
+
+  local jwt_secret = os.getenv("JWT_SECRET")
+  local node_name = os.getenv("NODE_NAME")
+
+  local claim_spec = {
+    sub = validators.equals("log-manager-"..node_name),
+    exp = validators.is_not_expired()
+  }
+  local jwt_obj = jwt:verify(jwt_secret, jwt_token, claim_spec)
+
+  if not jwt_obj["verified"] then
+      ngx.status = ngx.HTTP_FORBIDDEN
+      ngx.header["Access-Control-Allow-Origin"] = "*";
+      return ngx.exit(ngx.HTTP_OK)
+  end
 end
+
+return {
+  check_token = check_token,
+}

src/log-manager/src/nginx/list_logs.lua

@@ -14,6 +14,9 @@
 
 local cjson = require "cjson"
 local lfs = require "lfs"
+local path = require "path"
+
+local util = require "util"
 
 local function has_file_with_pattern(path, pattern)
   for file in lfs.dir(path) do
@@ -24,10 +27,6 @@ local function has_file_with_pattern(path, pattern)
   return false
 end
 
-local function is_dir(path)
-  return lfs.attributes(path, "mode") == "directory"
-end
-
 local args = ngx.req.get_uri_args()
 local username = args["username"]
 local framework_name = args["framework-name"]
@@ -41,34 +40,43 @@ if not token or not username or not taskrole or not framework_name or not pod_ui
   return ngx.exit(ngx.HTTP_OK)
 end
 
+if not util.is_input_validated(username) or not util.is_input_validated(framework_name) or
+   not util.is_input_validated(taskrole) or not util.is_input_validated(pod_uid) then
+  ngx.log(ngx.ERR, "some query parameters is invalid")
+  ngx.status = ngx.HTTP_BAD_REQUEST
+  return ngx.exit(ngx.HTTP_OK)
+end
+
 local log_query_param = "?username="..username.."&framework-name="..framework_name..
   "&pod-uid="..pod_uid.."&taskrole="..taskrole.."&token="..token
-local path = "/usr/local/pai/logs/"..username.."/".. framework_name.."/".. taskrole.."/"..pod_uid.."/"
-local path_prefix = "/api/v1/logs/"
+local log_dir = "/usr/local/pai/logs"..path.normalize("/"..username)..
+  path.normalize("/"..framework_name)..path.normalize("/"..taskrole)..path.normalize("/"..pod_uid).."/"
+local api_prefix = "/api/v1/logs/"
 
 local ret = {}
 
-if not is_dir(path) then
+if not util.is_path_under_log_dir(log_dir) or not path.isdir(log_dir) then
   ngx.log(ngx.ERR, "log folder not exists")
   ngx.status = ngx.HTTP_NOT_FOUND
   return ngx.exit(ngx.HTTP_OK)
 end
 
-for file in lfs.dir(path) do
-  if not is_dir(path..file) then
+for file in lfs.dir(log_dir) do
+  if not path.isdir(log_dir..file) then
     if string.match(file, "^user%.pai%..*$") then
       local sub_str = string.sub(file, string.len("user.pai.") + 1)
-      ret[sub_str] = path_prefix..file..log_query_param
+      ret[sub_str] = api_prefix..file..log_query_param
     else
-      ret[file] = path_prefix..file..log_query_param
+      ret[file] = api_prefix..file..log_query_param
     end
   elseif string.match(file, "^user-.*$") then
     local sub_str = string.sub(file, string.len("user-") + 1)
-    ret[sub_str] = path_prefix..file..log_query_param
-    if has_file_with_pattern(path..file, "^@.*%.s") then
-      ret[sub_str..".1"] = path_prefix..file..".1"..log_query_param
+    ret[sub_str] = api_prefix..file..log_query_param
+    if has_file_with_pattern(log_dir..file, "^@.*%.s") then
+      ret[sub_str..".1"] = api_prefix..file..".1"..log_query_param
     end
   end
 end
 
 ngx.say(cjson.encode(ret))
+

src/log-manager/src/nginx/nginx.conf

@@ -80,6 +80,9 @@ http {
     errlog.set_filter_level(ngx.INFO)
   }
   include /etc/nginx/conf.d/*.conf;
+
+  # set lua path
+  lua_package_path "/etc/nginx/lua/?.lua;;";
 }
 env ADMIN_NAME;
 env ADMIN_PASSWORD;

src/log-manager/src/nginx/nginx.conf.default

@@ -44,7 +44,10 @@ server {
       deny all;
     }
     default_type application/json;
-    access_by_lua_file /etc/nginx/lua/guard.lua;
+    access_by_lua_block {
+      local guard = require "guard"
+      guard.check_token()
+    }
     content_by_lua_file /etc/nginx/lua/list_logs.lua;
   }
 
@@ -67,11 +70,24 @@ server {
   location ~ ^/api/v1/logs/(.*)$ {
     add_header Access-Control-Allow-Origin *;
     add_header Access-Control-Allow-Methods 'GET';
+    add_header Content-Disposition 'attachment';
     limit_except GET {
       deny all;
     }
     default_type text/plain;
-    access_by_lua_file /etc/nginx/lua/guard.lua;
-    content_by_lua_file /etc/nginx/lua/get_log_content.lua;
+    limit_rate_after 1m;
+    limit_rate 1m;
+
+    rewrite_by_lua_file /etc/nginx/lua/get_log_content.lua;
+  }
+
+  location ~ /~/(.*)$ {
+    internal;
+    add_header Accept-Ranges bytes;
+    add_header Access-Control-Allow-Origin *;
+    rewrite ^/~/(.*)$ $1 break;
+
+    root /usr/local/pai/logs/;
   }
 }
+

src/log-manager/src/nginx/util.lua

@@ -0,0 +1,37 @@
+-- Copyright (c) Microsoft Corporation
+-- All rights reserved.
+-- MIT License
+-- Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
+-- documentation files (the "Software"), to deal in the Software without restriction, including without limitation
+-- the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and
+-- to permit persons to whom the Software is furnished to do so, subject to the following conditions:
+-- The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
+-- THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING
+-- BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+-- NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+-- DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+-- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+local path = require "path"
+
+local function is_path_under_log_dir(log_path)
+  local real_path = path.abspath(log_path)
+
+  if not string.match(real_path, "^/usr/local/pai/logs/.*") then
+    return false
+  end
+  return true
+end
+
+local function is_input_validated(input)
+  if not string.match(input, "^[%w_%-%.~]+$") then
+    return false
+  end
+  return true
+end
+
+return {
+  is_path_under_log_dir = is_path_under_log_dir,
+  is_input_validated = is_input_validated,
+}
+