Skip to content

Prevent buffer overflow during log filter actions #6950

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.

Sign up for GitHub

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

Merged
sudheerv merged 1 commit into from
Jun 27, 2020
Merged

Prevent buffer overflow during log filter actions #6950

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions proxy/logging/LogAccess.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1144,7 +1144,7 @@ void
LogAccess::set_client_req_url(char *buf, int len)
{
if (buf) {
m_client_req_url_len = len;
m_client_req_url_len = std::min(len, m_client_req_url_len);
ink_strlcpy(m_client_req_url_str, buf, m_client_req_url_len + 1);
}
}
Expand All @@ -1153,7 +1153,7 @@ void
LogAccess::set_client_req_url_canon(char *buf, int len)
{
if (buf) {
m_client_req_url_canon_len = len;
m_client_req_url_canon_len = std::min(len, m_client_req_url_canon_len);
ink_strlcpy(m_client_req_url_canon_str, buf, m_client_req_url_canon_len + 1);
}
}
Expand All @@ -1162,7 +1162,7 @@ void
LogAccess::set_client_req_unmapped_url_canon(char *buf, int len)
{
if (buf && m_client_req_unmapped_url_canon_str) {
m_client_req_unmapped_url_canon_len = len;
m_client_req_unmapped_url_canon_len = std::min(len, m_client_req_unmapped_url_canon_len);
ink_strlcpy(m_client_req_unmapped_url_canon_str, buf, m_client_req_unmapped_url_canon_len + 1);
Copy link
Contributor

@masaori335 masaori335 Jun 29, 2020

Choose a reason for hiding this comment

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

When the buffer is not allocated (m_client_req_unmapped_url_canon_str == INVALID_STR && m_client_req_unmapped_url_canon_len == 0), this ink_strlcpy() write \0 to the INVALID_STR[0] ?

Copy link
Collaborator

@a-canary a-canary Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

ln1164: m_client_req_unmapped_url_canon_str != 0

Copy link
Contributor Author

@sudheerv sudheerv Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@masaori335 the problem is when sometimes m_client_req_unmapped_url_canon_str is still pointing to INVALID_STR, but, the passed in source buf is the actual URL which is typically much longer than INVALID_STR, (for whatever reason, haven't actually debugged why m_client_req_unmapped_url_canon_str is not correctly initialized), it'd end up overwriting the global space past the INVALID_STR.

See for example

(gdb) p INVALID_STR
$2 = 0xaa2c10 <INVALID_STR> "https://www.linkedin.com/voyager/api/typeahead/hits?q=federated&query=", 'X' <repeats 130 times>...
(gdb)

Copy link
Contributor

@masaori335 masaori335 Jul 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with this change fix the buffer overflow.

What I'm wondering is this function will call ink_strlcpy like below under the conditions.

ink_strlcpy(INVALID_STR, buf, 1);

It might not be harmful, but meaningless.

The current checks in 1164 is only for nullptr. This doesn't work for INVALID_STR, right?
It looks better to check m_client_req_unmapped_url_canon_str is INVALID_STR or not too.

if (buf && m_client_req_unmapped_url_canon_str && m_client_req_unmapped_url_canon_str != INVALID_STR) {

or

if (buf && m_client_req_unmapped_url_canon_str && m_client_req_unmapped_url_canon_len  > 0) {

Copy link
Collaborator

@a-canary a-canary Jul 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

or just if (buf && m_client_req_unmapped_url_canon_len > 0) {

Note when it sets the str to INVALID_STR, it does not set the length, intentionally.

}
}
Expand All @@ -1171,7 +1171,7 @@ void
LogAccess::set_client_req_unmapped_url_path(char *buf, int len)
{
if (buf && m_client_req_unmapped_url_path_str) {
m_client_req_unmapped_url_path_len = len;
m_client_req_unmapped_url_path_len = std::min(len, m_client_req_unmapped_url_path_len);
ink_strlcpy(m_client_req_unmapped_url_path_str, buf, m_client_req_unmapped_url_path_len + 1);
}
}
Expand All @@ -1180,7 +1180,7 @@ void
LogAccess::set_client_req_unmapped_url_host(char *buf, int len)
{
if (buf && m_client_req_unmapped_url_host_str) {
m_client_req_unmapped_url_host_len = len;
m_client_req_unmapped_url_host_len = std::min(len, m_client_req_unmapped_url_host_len);
ink_strlcpy(m_client_req_unmapped_url_host_str, buf, m_client_req_unmapped_url_host_len + 1);
}
}
Expand All @@ -1190,7 +1190,7 @@ LogAccess::set_client_req_url_path(char *buf, int len)
{
//?? use m_client_req_unmapped_url_path_str for now..may need to enhance later..
if (buf && m_client_req_unmapped_url_path_str) {
m_client_req_url_path_len = len;
m_client_req_url_path_len = std::min(len, m_client_req_url_path_len);
ink_strlcpy(m_client_req_unmapped_url_path_str, buf, m_client_req_url_path_len + 1);
}
}
Expand Down