Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust wasm plugin painc #1280

Closed
1 task done
jaymie9019 opened this issue Sep 4, 2024 · 11 comments · Fixed by #1295
Closed
1 task done

rust wasm plugin painc #1280

jaymie9019 opened this issue Sep 4, 2024 · 11 comments · Fixed by #1295
Assignees
Labels
help wanted Extra attention is needed

Comments

@jaymie9019
Copy link
Contributor

jaymie9019 commented Sep 4, 2024

If you are reporting any crash or any potential security issue, do not
open an issue in this repo. Please report the issue via ASRC(Alibaba Security Response Center) where the issue will be triaged appropriately.

  • I have searched the issues of this repository and believe that this is not a duplicate.

Ⅰ. Issue Description

自行开发了一个基于 higress rust wasm 的 sdk 的插件在生产环境遇到了 panic,

Ⅱ. Describe what happened

截图中可以可以看到
image

当然这不是一个必现的问题,只有一些特殊的 case 才会遇到,最后我排查到的问题

https://github.com/alibaba/higress/blob/main/plugins/wasm-rust/src/plugin_wrapper.rs 文件中的一行代码

       for (k, v) in self.get_http_request_headers() {
            self.req_headers.insert(k, v);
        }

在这行代码中,底层调用了 hostcalls.rs 中的这个方法

    pub(super) fn deserialize_map(bytes: &[u8]) -> Vec<(String, String)> {
        let mut map = Vec::new();
        if bytes.is_empty() {
            return map;
        }
        let size = u32::from_le_bytes(<[u8; 4]>::try_from(&bytes[0..4]).unwrap()) as usize;
        let mut p = 4 + size * 8;
        for n in 0..size {
            let s = 4 + n * 8;
            let size = u32::from_le_bytes(<[u8; 4]>::try_from(&bytes[s..s + 4]).unwrap()) as usize;
            let key = bytes[p..p + size].to_vec();
            p += size + 1;
            let size =
                u32::from_le_bytes(<[u8; 4]>::try_from(&bytes[s + 4..s + 8]).unwrap()) as usize;
            let value = bytes[p..p + size].to_vec();
            p += size + 1;
            map.push((
                String::from_utf8(key).unwrap(),
                String::from_utf8(value).unwrap(),
            ));
        }
        map
    }

这行代码 String::from_utf8(value).unwrap(), 发生了 panic, 原因是有些特殊的 header 头的值无法被转成 utf8 格式的字符串,产生 error,所以 unwrap 就直接 panic。 我排查的思路是找到这个 header,然后打印出 traceId, 然后再去 access-log 中打开某个请求头的日志打印,再用 traceId 查询看下到底长什么样子。 这里给下截图,我把对应的头找到并且打印了出来

image

这里给两个方案

方案1,使用 String::from_utf8_lossy

for (k, v) in self.get_http_request_headers_bytes() {
         let header_value= String::from_utf8_lossy(v.as_slice()).to_string();
          self.req_headers.insert(k, header_value);
        }

方案2

        for (k, v) in self.get_http_request_headers_bytes() {
            if let Ok(header_value) = String::from_utf8(v) {
                self.req_headers.insert(k, header_value);
            }
        }

方案2更安全一些,但是丢弃了部分 header。这里更偏向于方案 1

@007gzs
Copy link
Collaborator

007gzs commented Sep 4, 2024

header里包含非ascii字符符合http协议标准么

@007gzs
Copy link
Collaborator

007gzs commented Sep 4, 2024

go的string应该也是只支持utf8的把, @CH3CHO 确认下header里有乱码的话go的plugin 是怎么处理的,可以统一一下

@jaymie9019
Copy link
Contributor Author

header里包含非ascii字符符合http协议标准么

对的,的确不符合 http 协议规范,正常的 header 头也不会出现这个问题,这次出问题的 header 是 cookie, 生产环境有些老业务逻辑对 cookie 有些不太好的实践导致的。

@007gzs
Copy link
Collaborator

007gzs commented Sep 4, 2024

对的,的确不符合 http 协议规范,正常的 header 头也不会出现这个问题,这次出问题的 header 是 cookie, 生产环境有些老业务逻辑对 cookie 有些不太好的实践导致的。

临时处理可以在自己插件里重写 on_http_request_headers 处理下,这种非标准的需要确定下怎么处理,但是panic不太合适

@jaymie9019
Copy link
Contributor Author

对的,的确不符合 http 协议规范,正常的 header 头也不会出现这个问题,这次出问题的 header 是 cookie, 生产环境有些老业务逻辑对 cookie 有些不太好的实践导致的。

临时处理可以在自己插件里重写 on_http_request_headers 处理下,这种非标准的需要确定下怎么处理,但是panic不太合适

嗯,我自己的问题已经解决了,zty 让我来提个 issue

@johnlanni
Copy link
Collaborator

@jaymie9019 我建议用方案2,但是对于过滤掉的header应该打warning日志

@johnlanni johnlanni added the help wanted Extra attention is needed label Sep 5, 2024
@johnlanni
Copy link
Collaborator

@jaymie9019 可以给 rust sdk 提个PR 哈,也建议给 proxy wasm上游社区提

@007gzs
Copy link
Collaborator

007gzs commented Sep 5, 2024

@jaymie9019 可以给 rust sdk 提个PR 哈,也建议给 proxy wasm上游社区提

上游提供了get_http_request_headers_bytes,这个问题是我在wrapper的实现里直接调用了 get_http_request_headers,这个函数是期望header里全部为ascii码的。其实主要是上游很多直接unwrap导致不符合期望的会直接panic

@jaymie9019
Copy link
Contributor Author

jaymie9019 commented Sep 9, 2024

@jaymie9019 可以给 rust sdk 提个PR 哈,也建议给 proxy wasm上游社区提

上游提供了get_http_request_headers_bytes,这个问题是我在wrapper的实现里直接调用了 get_http_request_headers,这个函数是期望header里全部为ascii码的。其实主要是上游很多直接unwrap导致不符合期望的会直接panic

ok~

@007gzs
Copy link
Collaborator

007gzs commented Sep 9, 2024

@007gzs
Copy link
Collaborator

007gzs commented Sep 9, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
Archived in project
3 participants