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.

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

CVE-2023-38545.md: add additional info #308

Closed
wants to merge 1 commit into from
Closed

CVE-2023-38545.md: add additional info #308

Show file tree
Hide file tree
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
2 changes: 2 additions & 0 deletions .github/scripts/spellcheck.words
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ DNS
dns
dnsop
DoH
DoS
doxygen
drftpd
dsa
Expand Down Expand Up @@ -652,6 +653,7 @@ runtime
Ruslan
rustc
rustls
RyotaK
Sagula
SanDisk
SAS
Expand Down
16 changes: 16 additions & 0 deletions docs/CVE-2023-38545.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,22 @@ Severity: High

HackerOne: https://hackerone.com/reports/2187833

ADDITIONAL INFO
---------------

Since the posting of this advisory, security researcher
[RyotaK](https://hackerone.com/ryotak?type=user) has
[notified](https://github.com/curl/curl-www/pull/308) that even if the buffer
size is large enough to prevent heap overflow an attacker can still use the
integer overflow of hostname length in conjunction with a crafted hostname
Copy link
Member

Choose a reason for hiding this comment

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

The reference to an integer overflow puzzles me. How is this an integer overflow?

Copy link

@Ry0taK Ry0taK Nov 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Since the hostname_len is a size_t, usually an unsigned int, casting it to the char may cause the overflow, so I called it integer overflow in the original email.
But something like the incorrect conversion between numeric types might fit more?

Copy link
Member Author

Choose a reason for hiding this comment

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

      else {
        socksreq[len++] = 3;
        socksreq[len++] = (char) hostname_len; /* one byte address length */
        memcpy(&socksreq[len], sx->hostname, hostname_len); /* w/o NULL */
        len += hostname_len;
      }

type conversion causes the overflow. there may be an overflow from size_t to char, or from char to unsigned char. assume twos complement and char is signed. it's usually a truncation though a compiler can technically do something else.

something like the incorrect conversion between numeric types might fit more?

i think integer overflow is fine and you had it right unless i'm misinterpreting the standard

Copy link
Member

@bagder bagder Nov 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah yes, you are right. I went back and checked the previous code as I had clearly blanked out on some of the specifics. It is indeed an integer overflow in there. Sorry for the noise.

longer than 255 characters to make the handshake complete successfully.

In this scenario the crafted hostname contains a destination hostname, port and
arbitrary data. The SOCKS server successfully connects to that host, sends the
arbitrary data and then curl sends the "expected" request data on that same
connection. The impact is limited because curl does not allow control
characters and null in the hostname.

AFFECTED VERSIONS
-----------------

Expand Down
Loading