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

[dhcp6relay] a couple memory access protections #9851

Merged
merged 3 commits into from
Jan 26, 2022

Conversation

yxieca
Copy link
Contributor

@yxieca yxieca commented Jan 26, 2022

Why I did it

  1. the strcpy and buffer allocation is not safe, it corrupts 1 byte on the stack. Depending on the memory layout, it may or may not cause issue immediately.
  2. message type is not validated before updating the counter. Which could cause segment fault.

How I did it

  1. Remove the unsafe strcpy, use config->interface.c_str() instead.
  2. Check message type before updating counters.

How to verify it

The issue (1) caused segment fault on a specific platform. The fix was validated there. Issue (2) was precautionary. Added log in case it triggers.

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Use config->interface->c_str() directly without strcpy. The original strcpy code has
memory corruption due to adding null terminator.

Signed-off-by: Ying Xie <[email protected]>
@yxieca yxieca requested a review from lguohan as a code owner January 26, 2022 03:22
@yxieca yxieca linked an issue Jan 26, 2022 that may be closed by this pull request
@yxieca yxieca merged commit 1ac140a into sonic-net:master Jan 26, 2022
@yxieca yxieca deleted the dhcp6relay branch January 26, 2022 17:58
qiluo-msft pushed a commit that referenced this pull request Jan 31, 2022
Why I did it
the strcpy and buffer allocation is not safe, it corrupts 1 byte on the stack. Depending on the memory layout, it may or may not cause issue immediately.
message type is not validated before updating the counter. Which could cause segment fault.

How I did it
Remove the unsafe strcpy, use config->interface.c_str() instead.
Check message type before updating counters.

How to verify it
The issue (1) caused segment fault on a specific platform. The fix was validated there. Issue (2) was precautionary. Added log in case it triggers.
@kellyyeh kellyyeh added the Request for 202111 Branch For PRs being requested for 202111 branch label Feb 16, 2022
judyjoseph pushed a commit that referenced this pull request Feb 22, 2022
Why I did it
the strcpy and buffer allocation is not safe, it corrupts 1 byte on the stack. Depending on the memory layout, it may or may not cause issue immediately.
message type is not validated before updating the counter. Which could cause segment fault.

How I did it
Remove the unsafe strcpy, use config->interface.c_str() instead.
Check message type before updating counters.

How to verify it
The issue (1) caused segment fault on a specific platform. The fix was validated there. Issue (2) was precautionary. Added log in case it triggers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dhcp_relay] dhcp6relay memory issue resulting in segfault
5 participants