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
base: master
from

Conversation

Projects
None yet
5 participants
@mkhon
Contributor

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

This comment has been minimized.

mention-bot commented Feb 6, 2017

@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.

Fix nonce-count generation in Curl_auth_create_digest_http_message():
- on the first invocation: keep security context returned by
InitializeSecurityContext()
- on subsequent invocations: use MakeSignature() instead of
InitializeSecurityContext() to generate HTTP digest response

@bagder bagder added the HTTP label Feb 7, 2017

@bagder

This comment has been minimized.

Member

bagder commented Feb 7, 2017

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

@MarcelRaad

Looks good and works for me.

@jay

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Member

bagder commented Feb 22, 2017

This seems to have caused #1276

@jay

This comment has been minimized.

Member

jay commented Feb 22, 2017

This seems to have caused #1276

Fixed in f4739f6

@mkhon mkhon deleted the mkhon:fix-digest-sspi branch May 22, 2017

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