bug: 插件 jwt-auth 的 function get_real_payload 存在 key 和 exp 被恶意修改的可能 about apisix HOT 15 OPEN

Our tech-team did some modification as follow:

local function get_real_payload(key, auth_conf, payload)
    local real_payload = {
        key = key,
        exp = ngx_time() + auth_conf.exp
    }
    if payload then
        local extra_payload = core.json.decode(payload)
-        core.table.merge(real_payload, extra_payload)
+        core.table.merge(extra_payload, real_payload)
+        return extra_payload
    end
    return real_payload
end

LGTM. Would you like to create a PR to fix it?

Ok, I'll try it.

It will be my first GITHUB PR.

from apisix.

could you please update the issue description in english?

from apisix.

@jaspertian , I can not follow your mean .
real_payload is a temporarily value , why are you mean cover real_payload ?

local function get_real_payload(key, auth_conf, payload)
    local real_payload = {
        key = key,
        exp = ngx_time() + auth_conf.exp
    }
    if payload then
        local extra_payload = core.json.decode(payload)
        core.table.merge(real_payload, extra_payload)
    end
    return real_payload
end

from apisix.

@jaspertian , I can not follow your mean . real_payload is a temporarily value , why are you mean cover real_payload ?

local function get_real_payload(key, auth_conf, payload)
    local real_payload = {
        key = key,
        exp = ngx_time() + auth_conf.exp
    }
    if payload then
        local extra_payload = core.json.decode(payload)
        core.table.merge(real_payload, extra_payload)
    end
    return real_payload
end

按照我的理解，Real-payload 中的 key 值表示的是 CONSUMER, 当客户端携带这个 JWT 访问时，会依据 key 值寻找 CONSUMER 并获取 SECRET 进行签名的验证。所以这个 key 值不能随意被 extra-payload 中的值覆盖。

同理， exp 值代表 JWT 失效时间，是通过 CONSUMER 中的配置值计算得出的，也不能被 extra-payload 中的值覆盖。

所以，real_payload 不是 temporarily value ，而是需要严格保护的值。

from apisix.

关于 key 值在验证中的使用，可参考 jwt-aut.lua 中的 rewrite 方法：

`
local user_key = jwt_obj.payload and jwt_obj.payload.key
if not user_key then
    return 401, {message = "missing user key in JWT token"}
end

local consumer_conf = consumer_mod.plugin(plugin_name)
if not consumer_conf then
    return 401, {message = "Missing related consumer"}
end

local consumers = consumer_mod.consumers_kv(plugin_name, consumer_conf, "key")

local consumer = consumers[user_key]
if not consumer then
    return 401, {message = "Invalid user key in JWT token"}
end
`

from apisix.

@hanqingwu

@jaspertian , I can not follow your mean . real_payload is a temporarily value , why are you mean cover real_payload ?

local function get_real_payload(key, auth_conf, payload)
    local real_payload = {
        key = key,
        exp = ngx_time() + auth_conf.exp
    }
    if payload then
        local extra_payload = core.json.decode(payload)
        core.table.merge(real_payload, extra_payload)
    end
    return real_payload
end

如您所示的代码，最后会 return real_payload 作为最终的 jwt payload ，所以其中的值应当确保没有被恶意篡改。

from apisix.

1. yes , from source code , if client payload contain key value will overwrite plugin setting , the key value from client url paramter also can be real_payload key, so I think key no need to fix.

2. when payload contain exp will overwrite plugin set, and get different exp value between plugin set, so I think it will be a vulnerability .
   curl -G --data-urlencode 'payload={"uid":10000,"uname":"test",exp: 43200}' http://127.0.0.1:9080/apisix/plugin/jwt/sign?key=user-key -i

3. finaly I suggest fix like these.

--- a/apisix/plugins/jwt-auth.lua
+++ b/apisix/plugins/jwt-auth.lua
@@ -249,13 +249,13 @@ end
 
 local function get_real_payload(key, auth_conf, payload)
     local real_payload = {
-        key = key,
-        exp = ngx_time() + auth_conf.exp
+        key = key
     }
     if payload then
         local extra_payload = core.json.decode(payload)
         core.table.merge(real_payload, extra_payload)
     end
+    real_payload.exp = ngx_time() + auth_conf.exp
     return real_payload
 end

from apisix.

1. yes , from source code , if client payload contain key value will overwrite plugin setting , the key value from client url paramter also can be real_payload key, so I think key no need to fix.
2. when payload contain exp will overwrite plugin set, and get different exp value between plugin set, so I think it will be a vulnerability .
   curl -G --data-urlencode 'payload={"uid":10000,"uname":"test",exp: 43200}' http://127.0.0.1:9080/apisix/plugin/jwt/sign?key=user-key -i
3. finaly I suggest fix like these.

--- a/apisix/plugins/jwt-auth.lua
+++ b/apisix/plugins/jwt-auth.lua
@@ -249,13 +249,13 @@ end
 
 local function get_real_payload(key, auth_conf, payload)
     local real_payload = {
-        key = key,
-        exp = ngx_time() + auth_conf.exp
+        key = key
     }
     if payload then
         local extra_payload = core.json.decode(payload)
         core.table.merge(real_payload, extra_payload)
     end
+    real_payload.exp = ngx_time() + auth_conf.exp
     return real_payload
 end

The key field can still be overridden by malicious payload

from apisix.

Our tech-team did some modification as follow:

local function get_real_payload(key, auth_conf, payload)
    local real_payload = {
        key = key,
        exp = ngx_time() + auth_conf.exp
    }
    if payload then
        local extra_payload = core.json.decode(payload)
-        core.table.merge(real_payload, extra_payload)
+        core.table.merge(extra_payload, real_payload)
+        return extra_payload
    end
    return real_payload
end

from apisix.

Our tech-team did some modification as follow:

local function get_real_payload(key, auth_conf, payload)
    local real_payload = {
        key = key,
        exp = ngx_time() + auth_conf.exp
    }
    if payload then
        local extra_payload = core.json.decode(payload)
-        core.table.merge(real_payload, extra_payload)
+        core.table.merge(extra_payload, real_payload)
+        return extra_payload
    end
    return real_payload
end

LGTM. Would you like to create a PR to fix it?

from apisix.

The get_real_payload is only used in endpoint /apisix/plugin/jwt/sign, right?
This endpoint is used to generate JWT token for users, it doesn't have any influence on gateway or upstream.
Why do we need to worry about data tampering？

from apisix.

The get_real_payload is only used in endpoint /apisix/plugin/jwt/sign, right?

Yes.

This endpoint is used to generate JWT token for users, it doesn't have any influence on gateway or upstream.

Yes.

Why do we need to worry about data tampering？

1. In practice many user need a centralized jwt ISSUER and this function is a potential solutions.
2. The function is a public announced API in official document.
3. There is not any comment or warning that the function is experimental and can't be involved in production environment.

So the bug should be fixed.

from apisix.

But the real_payload (the key) and the extra_payload are all from the request, it is acceptable that the extra_payload can modify the real_payload?

local function get_real_payload(key, auth_conf, payload)
    local real_payload = {
        key = **key**,
        exp = ngx_time() + auth_conf.exp
    }
    if payload then
        local extra_payload = core.json.decode(**payload**)
        core.table.merge(real_payload, extra_payload)
    end
    return real_payload
end

from apisix.

But the real_payload (the key) and the extra_payload are all from the request, it is acceptable that the extra_payload can modify the real_payload?

local function get_real_payload(key, auth_conf, payload)
    local real_payload = {
        key = **key**,
        exp = ngx_time() + auth_conf.exp
    }
    if payload then
        local extra_payload = core.json.decode(**payload**)
        core.table.merge(real_payload, extra_payload)
    end
    return real_payload
end

The key had been validated in function gen_token and only valid key will pass to get_real_payload.

Please refer to code line 396 - 413

local function gen_token()
    local args = core.request.get_uri_args()
    if not args or not args.key then
        return core.response.exit(400)
    end

    local key = args.key
    local payload = args.payload
    if payload then
        payload = ngx.unescape_uri(payload)
    end

    local consumer_conf = consumer_mod.plugin(plugin_name)
    if not consumer_conf then
        return core.response.exit(404)
    end

    local consumers = consumer_mod.consumers_kv(plugin_name, consumer_conf, "key")

    core.log.info("consumers: ", core.json.delay_encode(consumers))
    local consumer = consumers[key]
    if not consumer then
        return core.response.exit(404)
    end

from apisix.