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

Jump to bottom

Integer overflow in lib/vssh/libssh2.c #12983

Closed
vulnerabilityspotter opened this issue Feb 24, 2024 · 13 comments
Closed

Integer overflow in lib/vssh/libssh2.c #12983

vulnerabilityspotter opened this issue Feb 24, 2024 · 13 comments
Labels
SCP/SFTP

Comments

@vulnerabilityspotter
Copy link

I did this

Security Vulnerability Report

File: lib/vssh/libssh2.c

Function: ssh_statemachact() (Starting at line 2512)

Vulnerability Type: Integer Overflow

Location: Line 2547

Severity: High

Description:

In the ssh_statemachact() function, at line 2547, there exists a vulnerability related to an integer overflow. The vulnerability arises from the calculation of the variable size, which is assigned the value of to - from + 1.

size = to - from + 1;

If the values of to and from are such that to is sufficiently close to the maximum representable value for the data type being used, subtracting from from to may result in an overflow. Adding 1 to this potentially overflowing value could exacerbate the overflow or lead to unexpected behavior.

Exploitation Scenario:

An attacker may craft a malicious request with carefully chosen values for to and from such that to is very close to the maximum representable value for the data type, triggering an integer overflow during the calculation of size. This could potentially lead to various security issues, such as memory corruption, buffer overflows, or unexpected behavior, depending on how the size variable is subsequently used.

Impact:

The impact of this vulnerability could be severe, potentially leading to:

Memory Corruption: If the calculated size value is used to allocate memory or perform operations such as copying data, an integer overflow could result in memory corruption, leading to crashes or arbitrary code execution.

Security Bypass: In scenarios where size is used to enforce boundaries or permissions, an attacker may exploit the integer overflow to bypass security checks or gain unauthorized access to sensitive resources.

Denial of Service (DoS): A carefully crafted request exploiting the integer overflow could cause the application to enter an unexpected state or consume excessive resources, leading to a denial of service condition.

Recommendations:

Bounds Checking: Implement proper bounds checking to ensure that the values of to and from are within acceptable ranges before performing calculations.

Safe Arithmetic Operations: Consider using safer arithmetic operations or alternative calculation methods to prevent integer overflows, especially when dealing with potentially large or close-to-boundary values.

Input Validation: Validate input parameters (to and from) to ensure they adhere to expected ranges and constraints before performing calculations.

Error Handling: Implement robust error handling mechanisms to gracefully handle scenarios where input parameters result in unexpected or invalid calculations.

Severity Justification:

The presence of an integer overflow vulnerability at line 2547 poses a high risk to the security and stability of the application. Exploitation of this vulnerability could lead to severe consequences, including memory corruption, security bypass, or denial of service conditions.

Affected Versions:

This vulnerability affects all versions of the application that include the vulnerable ssh_statemachact() function.

References:

OWASP Integer Overflow
CWE-190: Integer Overflow or Wraparound
CERT Secure Coding - INT32-C

Conclusion:

The presence of an integer overflow vulnerability at line 2547 in the ssh_statemachact() function poses a high risk to the security and stability of the application. It is imperative to address this vulnerability promptly by implementing appropriate bounds checking and error handling mechanisms to prevent potential exploitation and associated security risks.

I expected the following

curl/libcurl version

7448054

operating system

All
@bagder
Copy link
Member

bagder commented Feb 24, 2024 •
edited

First, if you find and want to report security problems we clearly ask you to not report them here but instead do that at hackerone.com/curl. It is rude and inconsiderate to publish security risks in public at once.

But then: please explain exactly and with details which specific input a user needs to provide to curl or libcurl for this issue you mention to become a security problem.

The real risk I see here is that libcurl limits the transfer to an unexpected size. But then it is questionable if this can actually happen to a good-faith user.

@vulnerabilityspotter
Copy link
Author

The issue was detected by our new AI-powered vulnerability scanner, detecting that a similar pattern exists in lib/curl_range.c where the overflow is avoided with:

      totalsize = to - from;
      if(totalsize == CURL_OFF_T_MAX)
        return CURLE_RANGE_ERROR;

      data->req.maxdownload = totalsize + 1; /* include last byte */

Integer overflows in C are undefined behavior, and the behavior of the application is unknown when they arise.

@bagder
Copy link
Member

bagder commented Feb 24, 2024

Integer overflows in C are undefined behavior, and the behavior of the application is unknown when they arise.

That's a ridiculous stance that helps no one. If no known compilers on any known platforms make anything else but put a funny value in in size here, this is not a security problem. Unless you can prove me wrong.

bagder added a commit that referenced this issue Feb 24, 2024
@bagder
libssh/libssh2: return error on too big range resume
6538bc1 
Fixes #12983
@bagder bagder mentioned this issue Feb 24, 2024
Closed
@piru
Copy link

piru commented Feb 24, 2024

The issue was detected by our new AI-powered vulnerability scanner,

You really need to add some actual intelligence to the mix.

@tanepiper
Copy link

The issue was detected by our new AI-powered vulnerability scanner, detecting that a similar pattern exists in lib/curl_range.c where the overflow is avoided with:

      totalsize = to - from;
      if(totalsize == CURL_OFF_T_MAX)
        return CURLE_RANGE_ERROR;

      data->req.maxdownload = totalsize + 1; /* include last byte */

Integer overflows in C are undefined behavior, and the behavior of the application is unknown when they arise.

This is the baloney in the idiot sandwich that is this issue

Open
@bagder
Copy link
Member

bagder commented Feb 24, 2024

@vulnerabilityspotter until you have explained yourself and fixed your faulty procedure, I don't think you should continue to file new invalid issues in other projects (or in other project's mirror repos). That's just going to get you reported and/or blocked.

@bagder
Copy link
Member

bagder commented Feb 24, 2024

It can be noted that this is an integer overflow under the condition that the user asks for transferring the byte range "0 - 2^63" when at the same time the remote file has the size of at least 2^63 bytes. I think it's rare.

2^63 is 9223372036854775808 bytes. I believe it is about 8192 petabytes.

@Keremimo
Copy link

Bro even wrote the post using AI. I'm now even more certain that AI can only replace lazy people.

@s3rj1k
Copy link

s3rj1k commented Feb 25, 2024

@bagder The issue is preparing us for future where storage is limitless

@epicEaston197
Copy link

Why is this issue still open?

@jae1911
Copy link

jae1911 commented Feb 25, 2024

@bagder The issue is preparing us for future where storage is limitless

The /dev/null webscale storage.

@bagder bagder closed this as completed in f47487c Feb 25, 2024
@rockdaboot
Copy link
Contributor

For wget, we got a similar report. TBH, I find it worthy to try writing a tool for automated finding of software defects. Just because the AI tool isn't better than human experts on the first try doesn't mean the tool can't be improved. Other tools like static analyzers and fuzzers had their imperfect early days as well.

@epicEaston197
Copy link

I do feel like the AI should be used to find potential vulnerabilities but should definitely be verified by a human before an issue is actually created the AI should be used as a tool not a replacement for a human making issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
SCP/SFTP
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

libssh/libssh2: return error on too big range curl/curl
9 participants
@bagder @tanepiper @piru @rockdaboot @s3rj1k @epicEaston197 @jae1911 @Keremimo @vulnerabilityspotter