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

修复解决 CVE-2023-26148 #443

Merged
merged 1 commit into from
Oct 17, 2023
Merged

修复解决 CVE-2023-26148 #443

merged 1 commit into from
Oct 17, 2023

Conversation

Huangxiaodui
Copy link
Contributor

#442
尝试通过修改HttpMessage.cpp中的业务逻辑,将Header中的\r\n转义成\r\n,即可解决漏洞中描述的问题。

@Huangxiaodui Huangxiaodui changed the title 尝试修复解决 CVE-2023-26148 修复解决 CVE-2023-26148 Oct 11, 2023
@@ -489,7 +489,22 @@ void HttpMessage::DumpHeaders(std::string& str) {
// %s: %s\r\n
str += header.first;
str += ": ";
str += header.second;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这样改更简单:

str += hv::replaceAll(header.second, "\r\n", "\\r\\n");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感谢检视。
经过验证,如果命令中存在单个\r或者单个\n,也可能存在相同的问题。如果使用replaceall,需要写成如下部分:

std::string tmp_str = hv::replaceAll(header.second, "\r", "\\r");
str += tmp_str (header.second, "\n", "\\n");

感觉这样的写法应该会比较浪费性能。同时此类场景应该是比较少了,所以原有的写法中,提前判断是否存在\r\n,可以减少正常场景下的性能损失。

@ithewei ithewei merged commit f2b969f into ithewei:master Oct 17, 2023
6 checks passed
@risicle
Copy link

risicle commented Dec 2, 2023

(Hoping this traverses the language barrier..)

Are you aware of the existence of CVE-2023-26147 too?

https://nvd.nist.gov/vuln/detail/CVE-2023-26147
https://gist.github.com/dellalibera/2be265b56b7b3b00de1a777b9dec0c7b

It looks like the same thing, but in response headers. Are you confident that this addresses addresses that issue too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants