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

Fix heap overflow in set_browser_os() #2592

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

Sea-n
Copy link
Contributor

@Sea-n Sea-n commented Dec 10, 2023

I found this heap-buffer-overflow issue when debugging with ASAN (-fsanitize=address). And here is the proposed solution.

image

Problematic Code

char browser_type[BROWSER_TYPE_LEN] = "";
logitem->browser = verify_browser (ua, browser_type);
logitem->browser_type = xstrdup (browser_type);

if (!memcmp (logitem->browser_type, "Crawlers", 8)) {
    // ...
}

Access Log PoC

127.0.0.0 - - [10/Dec/2023:19:30:00 +0800] "GET / HTTP/1.1" 200 1234 "-" "Edge/42"

Reason

Since the User-Agent resolved to browser_type="Edge", it only have 4+1 bytes length. When memcmp() for 8 bytes, it will touch the improper memory.

Proposed Solution 1: Use strncmp instead

I think using strncmp instead of memcmp is the most intuitive way to avoid this problem.
The string length should be 8 char + null = 9 bytes.

if (!strncmp (logitem->browser_type, "Crawlers", 9)) {
    // ...
}

Proposed Solution 2: Compare to local variable instead

If we prefer memcmp (not sure about performance between two function), we can use the local variable browser_type instead of logitem->browser_type.

-if (!memcmp (logitem->browser_type, "Crawlers", 8)) {
+/* Compare to local variable instead of logitem->browser_type because
+ * xstrdup'd one might have length less than "Crawlers". */
+if (!memcmp (browser_type, "Crawlers", 9)) {
     // ...
 }

We can change to solution 2 (with or without comments) if you preferred.

@allinurl
Copy link
Owner

Merged. Appreciate your attention to detail. I see strncmp() fitting since we are dealing with string comparisons in this context.

@allinurl allinurl closed this Dec 11, 2023
@allinurl allinurl reopened this Dec 11, 2023
@allinurl allinurl merged commit 5138abb into allinurl:master Dec 11, 2023
5 checks passed
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.

2 participants