Fix multiple heap buffer overflow vulnerabilities in URL parsing #1045

ShadowRider09 · 2025-11-18T05:38:05Z

 - Fix heap-buffer-overflow in URL::from_string() by using string_view
 - Fix strtol overflow by manual port number parsing
 - Fix rstring_view boundary check failures
 - Fix null pointer dereference in to_int64_check()
 - Fix stack buffer overflow in iov_iterator
 and the fixed_crashes are zipped inattach files.

fixed_crash.zip

CLAassistant · 2025-11-18T05:38:13Z

All committers have signed the CLA.

lihuiba · 2025-11-19T02:07:33Z

common/iovector.cpp

        assert(_iovcnt);
        assert(n <= _v.iov_len);
        if (n < _v.iov_len) { _v += n; }
-        else { _v = *++_iov; _iovcnt--; }


This is an internal helper class inside a .cpp file. Its usage ensures the validness of *++_iov. If you are worried about this, add an assert instead.

net/http/url.cpp

lihuiba · 2025-11-19T06:01:34Z

net/http/url.cpp

    m_target = rstring_view16(pos, url.size());
    pos += p+1;
-    m_query = rstring_view16(pos, url.size() - (p + 1));
+    auto query_len = (p + 1 <= url.size()) ? (url.size() - (p + 1)) : 0;


use photon::sat_sub() to avoid overflow.

lihuiba · 2025-11-19T06:01:54Z

net/http/url.h

 class URL {
 protected:
-    const char* m_url = nullptr;
+    std::string_view m_url;


why string_view is necessary for m_url?

Looking at the code, you're absolutely right. The std::string_view m_url is NOT necessary. The original const char* m_url was fine because:
- rstring_view::operator|(const char*) calls to_abs(s) which simply does s + _offset with
the stored _length - no strlen() is involved
- All the rstring_view16 members (m_host, m_path, etc.) already store both offset and length
- The actual heap-buffer-overflow was likely caused by something else, not by the type of m_url

lihuiba · 2025-11-19T07:10:31Z

net/http/url.cpp

 namespace net {
 namespace http {

 void URL::fix_target() {


This is actually a private helper function only used in URL::from_string(). I would suggest move the function body into URL::from_string().

ShadowRider09 · 2025-11-19T12:26:21Z

Hi , I've addressed all your feedback:
- ✅ Reverted m_url to const char*
- ✅ Used assert() for iov_iterator boundary check
- ✅ Used estring_view::to_uint64_check() for port parsing
- ✅ Used photon::sat_sub() to prevent integer underflow
- ✅ Inlined fix_target() into from_string()

lihuiba · 2025-11-20T08:03:06Z

net/http/url.h


    std::string to_string() {
-        return m_url;
+        return std::string(m_url);


simply return m_url; is fine.

lihuiba · 2025-11-20T08:04:18Z

net/http/url.cpp

+        if (port_len > 0) {
+            uint64_t port_val = 0;
+            estring_view port_str(url.data(), port_len);
+            if (port_str.to_uint64_check(&port_val)) {


check for port_val > 65535

lihuiba · 2025-11-20T08:07:32Z

net/http/url.cpp

-        return;
+
+    // Fix target if needed (was previously a separate fix_target() function)
+    estring_view t = m_url | m_target;


you can still use DEFER({ ... }); to execute this code snippet on all returns. If that's ugly, define a lambda, and invoke it with DEFER.

lihuiba · 2025-11-20T08:09:39Z

net/http/url.cpp

 #include "url.h"
 #include <photon/common/alog.h>
 #include <photon/common/alog-stdstring.h>
+#include <photon/common/utility.h>


Is this necessary?

Yes, it's necessary. The file now uses:
- DEFER macro (line 35) - defined in utility.h
- sat_sub function (lines 102, 117) - also defined in utility.h

Both are required for the code to compile.

lihuiba · 2025-11-21T01:53:27Z

common/estring.h

+        // Ensure offset and length are within bounds of the string_view
+        if (s.length() < _offset + _length) {
+            // Return empty view if out of bounds
+            return estring_view("", (size_t)0);


simply return {};

lihuiba · 2025-11-21T01:55:32Z

net/http/url.cpp

-            if (need_optional_port(*this)) m_host_port = rstring_view16(m_host.offset(), m_host.length() + port_len);
+    } else if (url.front() == ':') {
+        url.remove_prefix(1); // Skip ':'
+        // Parse port number, find the end of digit sequence


The logic of finding the digits are already included in to_uint64_check(). You may not need to do it yourself?

lihuiba · 2025-11-21T06:04:55Z

net/http/url.cpp

+            m_query = rstring_view16(0, 0);
+            m_target = m_path;
+        } else {
+            p = url.find_first_of("?");


do NOT repeat similar logics, such as find_first_of("?") and its related processing.

lihuiba · 2025-11-21T06:06:51Z

net/http/url.cpp

+            if (port_str.to_uint64_check(&port_val) && port_val <= 65535) {
+                m_port = static_cast<uint16_t>(port_val);
+            } else {
+                m_port = m_secure ? 443 : 80;


consider moving the assignment m_port = m_secure ? 443 : 80; upward, immediately after m_secure is assigned, in order to avoid code duplication.

lihuiba · 2025-11-21T06:07:55Z

net/http/url.cpp

+        url.remove_prefix(port_len);
+
+        if (url.empty()) {
+            m_path = rstring_view16(pos, 0);


goto the then part of prior if (url.empty()) { ... }, in order to avoid code duplication.

lihuiba · 2025-11-24T01:46:04Z

net/http/url.cpp

-        m_query = rstring_view16(0, 0);
-        m_target = m_path;
-        return;
+    } else {


The original form of linear code structure (with multiple early returns) is more preferable than nesting elses and {...}s, as it is easier to read and understand.

If there were vulnerabilities, please try to fix them without changing code structure, unless it's not proper.

lihuiba · 2025-11-24T07:10:18Z

net/http/url.cpp

+        url.remove_prefix(1); // Skip ':'
+        pos += 1; // +1 for ':'
+        // Find end of port digits
+        auto port_end = url.find_first_not_of("0123456789");


You may not need to find the first non-digit, because estring_view::to_uint64_check() already does that. It stops at the first non-digit.

So, just call it directly.

It turns out that to_uint64_check doesn't return the number of valid digits, so it's not possible to avoid find_first_not_of() at present.

Leave it as is for now. I'll address this issue in another PR.

lihuiba · 2025-11-26T02:38:36Z

Let me address the issue directly and push to your branch.

lihuiba · 2025-11-26T03:23:17Z

Oh, I'm sorry to have mistakenly force pushed my main branch to yours, which cleaned your new commits and closed the PR, and can not be re-opened again. So I'm not able to push to your repo again to fix my mistake.

I have pushed your commits to https://github.com/lihuiba/PhotonLibOS/tree/ShadowRider and opened a temporary PR at #1057

You can pull the commits (with my update) back, push them to your repo again and reopen the PR, or let me merge the new PR.

lihuiba · 2025-11-27T02:59:52Z

@ShadowRider09 As you were not responding, we merged the PR #1057 in order to continue improving related code.

Thanks again for your contribution!

lihuiba · 2025-11-27T08:04:30Z

#1058 is the follow on PR

liulanzheng approved these changes Nov 19, 2025

View reviewed changes

lihuiba reviewed Nov 19, 2025

View reviewed changes

ShadowRider09 force-pushed the main branch 2 times, most recently from ee3c422 to 78b2565 Compare November 19, 2025 12:20

lihuiba approved these changes Nov 20, 2025

View reviewed changes

lihuiba reviewed Nov 21, 2025

View reviewed changes

lihuiba self-requested a review November 21, 2025 05:36

lihuiba reviewed Nov 21, 2025

View reviewed changes

lihuiba requested review from lihuiba and liulanzheng November 21, 2025 06:08

lihuiba reviewed Nov 24, 2025

View reviewed changes

lihuiba self-requested a review November 24, 2025 07:03

lihuiba reviewed Nov 24, 2025

View reviewed changes

lihuiba self-requested a review November 24, 2025 07:48

lihuiba closed this Nov 26, 2025

lihuiba force-pushed the main branch from 1806102 to 34eb399 Compare November 26, 2025 03:14

Fix multiple heap buffer overflow vulnerabilities in URL parsing #1045

Fix multiple heap buffer overflow vulnerabilities in URL parsing #1045

Conversation

ShadowRider09 commented Nov 18, 2025

Uh oh!

CLAassistant commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShadowRider09 commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lihuiba Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lihuiba Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lihuiba Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lihuiba commented Nov 26, 2025

Uh oh!

lihuiba commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lihuiba commented Nov 27, 2025

Uh oh!

lihuiba commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Nov 18, 2025 •

edited

Loading

lihuiba Nov 21, 2025 •

edited

Loading

lihuiba Nov 21, 2025 •

edited

Loading

lihuiba Nov 24, 2025 •

edited

Loading

lihuiba commented Nov 26, 2025 •

edited

Loading