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

Fix buffer overflow when PAC is enabled #126

Merged
merged 1 commit into from Nov 10, 2020

Conversation

lifeibiren
Copy link
Contributor

The bug was found on Windows 10 (MINGW64) when PAC is enabled. It turned
out to be the large PAC file (more than 102400 bytes) returned by a
local proxy program with no content-length present.

libproxy/url.cpp Outdated
content_length == 0 ? PAC_HTTP_BLOCK_SIZE
: content_length - recvd, 0);
// Calculate length to recv
unsigned int length_to_read = content_length == 0 ? PAC_HTTP_BLOCK_SIZE
Copy link
Contributor

Choose a reason for hiding this comment

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

As you're touching this already, can you add parentheses around content_length - recvd?

libproxy/url.cpp Outdated
buffer = NULL;
if (content_length == 0 || content_length == recvd) {
buffer = new char[recvd + 1];
memcpy(buffer, dynamic_buffer.data(), recvd);
Copy link
Contributor

Choose a reason for hiding this comment

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

dynamic_buffer can be empty, in which case this is undefined behaviour.

@@ -479,8 +479,7 @@ char* url::get_pac() {

// Get content
unsigned int recvd = 0;
buffer = new char[PAC_MAX_SIZE];
memset(buffer, 0, PAC_MAX_SIZE);
std::vector<char> dynamic_buffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

This keeps track of the size already, which makes recvd redundant and a source of bugs if they get out of sync. If dynamic_buffer is truncated after short reads (which just adjusts the internal size) that could be avoided.

libproxy/url.cpp Outdated
int r = recv(sock, dynamic_buffer.data() + dynamic_buffer.size() - length_to_read, length_to_read, 0);

// Shrink buffer to fit
unsigned int to_shrink = length_to_read;
Copy link
Contributor

Choose a reason for hiding this comment

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

Simpler and also works with r == 0:

// Shrink buffer to fit
if (r >= 0)
	dynamic_buffer.resize(dynamic_buffer.size() - length_to_read + r);

libproxy/url.cpp Outdated
if (content_length != 0 && string(buffer).size() != content_length) {
delete[] buffer;
buffer = NULL;
if ((content_length == 0 || content_length == dynamic_buffer.size()) && !dynamic_buffer.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously it returned a valid pointer for empty buffers as well, which is not the case anymore here.

The bug was found on Windows 10 (MINGW64) when PAC is enabled. It turned
out to be the large PAC file (more than 102400 bytes) returned by a
local proxy program with no content-length present.
Copy link
Contributor

@Vogtinator Vogtinator left a comment

Choose a reason for hiding this comment

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

LGTM

@mcatanzaro
Copy link
Contributor

I'll encourage you to request a CVE for this issue using https://cveform.mitre.org/.

@carnil
Copy link

carnil commented Sep 30, 2020

I'll encourage you to request a CVE for this issue using https://cveform.mitre.org/.

CVE-2020-26154 was assigned by MITRE to this issue.

@ndowens
Copy link

ndowens commented Oct 5, 2020

Is this patch going to get merged?

@PerkinTahmaz
Copy link

hey, when will the patch be merged? or what would be the manual way applying this patch? it is assigned as a critical vulnerability

@DimStar77 DimStar77 merged commit 6d342b5 into libproxy:master Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants