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 memory leak in curl_sasl.c #667

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@neex
Contributor

neex commented Feb 19, 2016

If any parameter in a HTTP DIGEST challenge message is present multiple
times, memory allocated for all but the last entry should be freed.

Server answer to reproduce the leak is:

HTTP/1.1 401 hui
WWW-Authenticate: Digest algorithm=MD5, algorithm=MD5, nonce=1, nonce=2

Two blocks will be "definitely lost" in valgrind: one for the first algorithm and one for the first nonce.

Fix memory leak in curl_sasl.c
If any parameter in a HTTP DIGEST challenge message is present multiple
times, memory allocated for all but the last entry should be freed.
@jay

This comment has been minimized.

Member

jay commented Feb 19, 2016

I'm ok with this but I think that's a really weird thing to see especially if they have the same algorithm. What server are you seeing that on? It says this for algorithm in RFC 2617:

If the algorithm is not understood, the challenge should be ignored (and a different one used, if there is more than one).

I would expect a different Digest challenge is somehow separated though and prefixed with 'Digest'. Even if it is it would appear we ignore duplicate digest messages see https://github.com/curl/curl/blob/curl-7_47_1/lib/http.c#L850-L872

@dfandrich

This comment has been minimized.

Collaborator

dfandrich commented Feb 19, 2016

I was able to verify the problem and checked in test 1437 which shows it.

@neex

This comment has been minimized.

Contributor

neex commented Feb 19, 2016

The snippet in the first comment is just most simple testcase which I prepared to demonstrate the issue. The original server response (obtained by fuzzing) doesn't make any sense for me too (in fact, it was more erroneous as it had invalid algorithm field).

jay added a commit that referenced this pull request Feb 20, 2016

curl_sasl: Fix memory leak in digest parser
If any parameter in a HTTP DIGEST challenge message is present multiple
times, memory allocated for all but the last entry should be freed.

Bug: #667
@jay

This comment has been minimized.

Member

jay commented Feb 20, 2016

Thanks Emil, landed in 3fa220a. And thanks for the test Dan.

@jay jay closed this Feb 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment