Remove toa field, fix security vulnerability.#925
Merged
ywc689 merged 1 commit intoiqiyi:develfrom Dec 8, 2023
Merged
Conversation
ywc689
reviewed
Nov 30, 2023
src/ipvs/ip_vs_proto_tcp.c
Outdated
|
|
||
| /* Sometimes, the backend dpvs system allows toa data filled by the previous dpvs system. e.g.: LB --> LB --> RS | ||
| * But disabled default. 0: disable, 1: enable; etc. */ | ||
| int dpvs_use_client_toa = 0; |
Collaborator
There was a problem hiding this comment.
- 这个参数没有配置入口；
- 如果这个变量只用在当前代码文件中，请使用 static 类型
- 原理上看，这个配置参数对dpvs 的toa没有实际意义
Contributor
Author
There was a problem hiding this comment.
1，是的，我还没想好该如何传递这个参数。
2，好
3，dpvs串联的场景，在一些大长里比较常见。前置LB需要清空，但接力的LB需要读取这个参数。我觉得有必要。除非dpvs不支持LB串联。
src/ipvs/ip_vs_proto_tcp.c
Outdated
| && (opsize == tcp_opt_len)) { | ||
| for (i = 0; i < tcp_opt_len; i++) | ||
| *(ptr - 2 + i) = TCP_OPT_NOP; | ||
| return; |
src/ipvs/ip_vs_proto_tcp.c
Outdated
Comment on lines
+766
to
+769
|
|
||
| if (dpvs_use_client_toa == 0) { | ||
| tcp_in_remove_toa(th, af); | ||
| } |
Collaborator
There was a problem hiding this comment.
这个漏洞是不是只有在dpvs 调用 tcp_in_add_toa 失败的情况下才可能被利用？如果是这样，可以只在失败的情况下调用 tcp_in_remove_toa 以减小不必要的性能损耗。
Contributor
Author
There was a problem hiding this comment.
我觉得不是，即使添加成功，但RS上内核模块读取的逻辑是不确定的。理应前置链路确保后续安全，不应该将风险往后传递。
Collaborator
There was a problem hiding this comment.
内核模块的逻辑是确定的，参考 toa.ko 的代码中的 get_toa_data 函数。
https://github.com/iqiyi/dpvs/blob/master/kmod/toa/toa.c#L348C16-L348C16
ywc689
added
the
pr/to-test-codes
Contributor
Author
|
已更新，请审阅。 updated , PTAL. |
ywc689
reviewed
Dec 4, 2023
src/ipvs/ip_vs_proto_tcp.c
Outdated
Comment on lines
+44
to
+46
| /* Sometimes, the backend dpvs system allows toa data filled by the previous dpvs system. | ||
| * See https://github.com/iqiyi/dpvs/pull/925 for more detail. | ||
| * e.g.: LB --> LB --> RS |
Contributor
Author
There was a problem hiding this comment.
OK，已更新，去除了串联模式的相关配置。
另外，如果没问题的话，请你这里 merge时，选择squash merge吧。我这里reset 分支后，不太对，变更特别多。
src/ipvs/ip_vs_proto_tcp.c
Outdated
|
|
||
| /* Sometimes, the backend dpvs system allows toa data filled by the previous dpvs system. e.g.: LB --> LB --> RS | ||
| * But disabled default. 0: disable, 1: enable; etc. */ | ||
| int dpvs_use_client_toa = 0; |
legend050709
reviewed
Dec 6, 2023
ywc689
approved these changes
Dec 7, 2023
ywc689
reviewed
Dec 7, 2023
cfc4n
added a commit
to cfc4n/dpvs
that referenced
this pull request
Dec 7, 2023
See iqiyi#925 for more detail.
cfc4n
changed the title
See iqiyi#925 for more detail.
cfc4n
force-pushed
the
remove_toa
branch
2 times, most recently
from
321b3d0 to
e74927c
Compare
ywc689
approved these changes
Dec 8, 2023
ywc689
added
pr/codes-tested-ok
请问具体细节是否可以公布了呢？ |
Contributor
Author
|
L4LB比较底层，更新比较慢，还有很多人没升级完，还需要一段时间。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
存在一定安全风险，具体细节等未来用户升级完成后公布。
UPDATE(2024-08):
漏洞详情：https://mp.weixin.qq.com/s/B4y3JV6_Jb0ew9u1vVGE4g