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

cf-socket: error if address can't be copied #15784

Closed
wants to merge 1 commit into from
Closed

Conversation

jay
Copy link
Member

@jay jay commented Dec 19, 2024

  • When converting Curl_addrinfo to Curl_sockaddr_ex, if the address length is too large then return error CURLE_TOO_LARGE.

Prior to this change the address structure was truncated on copy, and the length shortened which I think is incorrect.

AFAICS the only time it could conceivably happen is when a UNIX socket path is too long, and even then curl should've accounted for that by having a structure that is large enough to store it. This is why I added a DEBUGASSERT for debug builds, because I don't think it should ever happen.

Closes #xxxx


Discovered while working on #15748 but I don't see how it could be exploited.

@jay jay added the tidy-up label Dec 19, 2024
@bagder bagder requested a review from icing December 19, 2024 22:17
Copy link
Contributor

@icing icing left a comment

Choose a reason for hiding this comment

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

Nice catch. We should fail in such circumstances.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Seems like the right thing to do, yes!

- When converting Curl_addrinfo to Curl_sockaddr_ex, if the address
  length is too large then return error CURLE_TOO_LARGE.

Prior to this change the address structure was truncated on copy, and
the length shortened which I think is incorrect.

AFAICS the only time it could conceivably happen is when a UNIX socket
path is too long, and even then curl should've accounted for that by
having a structure that is large enough to store it. This is why I added
a DEBUGASSERT for debug builds, because I don't think it should ever
happen.

Closes curl#15784
@jay jay closed this in 5536741 Dec 22, 2024
@jay jay deleted the safe_addrlen branch December 22, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants