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

Potential Incorrect Acceptance Handling in parseUrl() for Port Parameter #4200

Closed
bfelpsy opened this issue Jun 8, 2022 · 1 comment
Closed
Labels
Bug Good First Issue Great issue for a new contributor Tech Debt Non-urgent improvements

Comments

@bfelpsy
Copy link

bfelpsy commented Jun 8, 2022

The custom parseUrl() implementation contains a potential edge case issue regarding the parsing of ports.

Consider the following group in the regular expression that determines basic acceptance of the URL: StringUtilities.cpp#L61-L62

 // optional port
"(?::([[:digit:]]+))?"

This expression will accept an arbitrary digit sequence with length >=1, including numbers which are larger than the uint16_t that are used for port numbers.

On a match with port number content, StringUtilities.cpp#L90-L93 will use the custom LexicalCast.h based implementation to unconditionally convert the port:

 if (!port.empty())
{
    pUrl.port = beast::lexicalCast<std::uint16_t>(port);
}

Although benign, the code appears to incorrectly accept ports that are out of range and should have been rejected as an invalid URL. Instead it handles them as port 0 by falling back to the default uint16_t value. Additionally, port 0 is also accepted when used in the URL directly.

@nbougalis nbougalis added Good First Issue Great issue for a new contributor Bug Tech Debt Non-urgent improvements labels Jun 13, 2022
@nbougalis
Copy link
Contributor

Thank you for reporting this issue. I agree that it's a bug. The "quick" fix is to fail parsing if the port is specified and the parsed value is 0 and I think that's reasonable enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Good First Issue Great issue for a new contributor Tech Debt Non-urgent improvements
Projects
None yet
Development

No branches or pull requests

6 participants
@nbougalis @bfelpsy and others