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 nonce-count generation in Curl_auth_create_digest_http_message() #1251

Closed
wants to merge 1 commit into from

Conversation

mkhon
Copy link
Contributor

@mkhon mkhon commented Feb 6, 2017

  • on the first invocation: keep security context returned by
    InitializeSecurityContext()
  • on subsequent invocations: use MakeSignature() instead of
    InitializeSecurityContext() to generate HTTP digest response

Fixes #870

@mention-bot
Copy link

@mkhon, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @captain-caveman2k and @yangtse to be potential reviewers.

- on the first invocation: keep security context returned by
InitializeSecurityContext()
- on subsequent invocations: use MakeSignature() instead of
InitializeSecurityContext() to generate HTTP digest response
@bagder
Copy link
Member

bagder commented Feb 7, 2017

(The test 1903 failure is probably unrelated as it seems to do this intermittently.)

Copy link
Member

@MarcelRaad MarcelRaad left a comment

Choose a reason for hiding this comment

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

Looks good and works for me.

@jay
Copy link
Member

jay commented Feb 17, 2017

This looks good however why is the have_context variable needed? If context is set then you have context, and if it's not then you don't right? It seems to me we'd want to del context in cleanup even if have_context was not set, but when would it happen that there is context and it's not set?

edit: nm I see what you did, typically for that I'd expect context to be a pointer ie CtxtHandle *context. I have started some changes for that and a test.

@jay jay closed this in f77dabe Feb 20, 2017
@jay
Copy link
Member

jay commented Feb 20, 2017

I've just landed this with a test and modified your code slightly.

I got rid of have_context by using a pointer to the http context that is NULL when there is no context. Also I renamed it as http_context so it is not accidentally confused in the future with the SASL/md5 version of context of the function in the same file. Further, I moved the MakeSignature block to before a new context is created so that if MakeSignature fails we delete the context and fall back on creating a new context. Your code is otherwise the same. So basically it is this now:

if(http_context) {
do makesignature, if it fails verbose warn and delete http_context and set to null
}
if(!http_context) {
create new http_context
}

Thanks!

@mkhon
Copy link
Contributor Author

mkhon commented Feb 20, 2017

@jay I am not sure this Curl_safefree() is ok:

resp = malloc(output_token_len + 1);
if(!resp) {
free(output_token);

Curl_safefree(digest->http_context);

return CURLE_OUT_OF_MEMORY;

}

I think if the context is ok it should be kept for the subsequent invocations.

@jay
Copy link
Member

jay commented Feb 21, 2017

I think if the context is ok it should be kept for the subsequent invocations.

Freeing the context there isn't needed but not for the reason you think. When CURLE_OUT_OF_MEMORY the connection is cleaned up, including digests, so that cleans up the valid context. I removed it in af5fbb1.

@bagder
Copy link
Member

bagder commented Feb 22, 2017

This seems to have caused #1276

@jay
Copy link
Member

jay commented Feb 22, 2017

This seems to have caused #1276

Fixed in f4739f6

@mkhon mkhon deleted the fix-digest-sspi branch May 22, 2017 08:46
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants