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

digest: reject broken header with session protocol and without qop #9077

Closed
wants to merge 1 commit into from

Conversation

Karlson2k
Copy link
Contributor

@Karlson2k Karlson2k commented Jul 1, 2022

Session algorithms require use of cnonce.
cnonce could be used only when qop=auth or qop=auth-int. cnonce is not used when qop is empty or missing (missing qop triggers RFC2069 compatibility mode, which does not use cnonce).
When qop is empty, cnonce is not sent by curl so there is no way for the server to check calculations where cnonce is involved, like H(A1).

Cherry-picked from PR #9074

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Have you tried to add a test case that verifies this behavior?

@bagder
Copy link
Member

bagder commented Jul 1, 2022

Seeing this code made me file #9079 since it'll make it possible to just & instead of comparing three different values.

@Karlson2k
Copy link
Contributor Author

Have you tried to add a test case that verifies this behavior?

Not yet. I'm working on #9074 currently. May return to this one later, but for me it's fine if simple test case would be added by someone else. :)

Seeing this code made me file #9079 since it'll make it possible to just & instead of comparing three different values.

I like the idea.

@bagder
Copy link
Member

bagder commented Jul 1, 2022

for me it's fine if simple test case would be added by someone else.

It seems we don't even have a single test case using a Digest -sess algorithm right now so I can't copy/clone one and I don't know enough of this to make up a test case.

Can you record a working set of HTTP requests and response that we can use to create a test from?

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Jul 2, 2022

It seems we don't even have a single test case using a Digest -sess algorithm right now so I can't copy/clone one and I don't know enough of this to make up a test case.

Yes, curl doesn't have any single test for session digest. However, as no predefined pseudorandom sequence can be enforced for current libcurl (even for testing), it makes a little sense to write test for sessions, as all cnonce and response values would need to be filtered, like it is implemented for a few tests with qop=auth. Actually, any digest test with non-empty qop could be used for sessions digest as the testsuite doesn't check generated values, only presence of them.

Maybe it would make sense to bring back option for predefined pseudorandom sequence, at least for debug builds? Or make it internal only so only tests could use it, while it would not be available in public API (libcurl already have such function, like session reset for easy handle).

Can you record a working set of HTTP requests and response that we can use to create a test from?

This is the problem. Neither Apache nor Nginx supports sessions (they don't support SHA256 as well).
Maybe IIS has some support, need to check SSPI documentation.

@bagder
Copy link
Member

bagder commented Jul 2, 2022

as no predefined pseudorandom sequence can be enforced for current libcurl (even for testing),

Where did you get that from? Debug builds of curl even has a fixed random seed when it runs tests, so random is not an issue there.

@Karlson2k Karlson2k force-pushed the digest_auth_fix_02.1 branch from 8a9b4ec to b9cfc54 Compare July 6, 2022 19:34
@Karlson2k
Copy link
Contributor Author

The PR itself is correct, I think.
Rebased.

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Jul 7, 2022

Can you record a working set of HTTP requests and response that we can use to create a test from?

I've made some investigation about it. See #9074 (comment) and #9074 (comment).

@Karlson2k Karlson2k force-pushed the digest_auth_fix_02.1 branch from b9cfc54 to f6f8b8d Compare July 14, 2022 06:33
@Karlson2k
Copy link
Contributor Author

Rebased

@Karlson2k Karlson2k force-pushed the digest_auth_fix_02.1 branch from f6f8b8d to de6a782 Compare July 15, 2022 06:21
@Karlson2k
Copy link
Contributor Author

Rebased again.

@bagder bagder closed this in 3fe24ea Aug 7, 2022
@bagder
Copy link
Member

bagder commented Aug 7, 2022

thanks!

@Karlson2k Karlson2k deleted the digest_auth_fix_02.1 branch September 5, 2022 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants