-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Fixes related to digest authorisation #9074
Conversation
440ebde
to
03fbd6a
Compare
03fbd6a
to
3cd4168
Compare
Is there a security impact with this change? The second two points especially
sounds like there might be.
|
It is mostly improvements, except the last item (reset auth), but currently it breaks tests 2024, 2027, 2030. |
3cd4168
to
444e7d8
Compare
PR has been simplified by moving some commits to separate PRs |
I've checked popular web server software and found out:
Firefox bugtracker has related bug opened 18 years ago and still not closed: https://bugzilla.mozilla.org/show_bug.cgi?id=270447 |
As IIS is the only server, which supports The old, already abandoned IE works fine. IE uses the same IIS <-> IE log
EDGE gets IIS <-> EDGE log
Chrome behaviour is similar to EDGE's one (to be more precise: actually Edge behaves like Chrome). IIS <-> Chrome log
Firefox re-requests password each page reload (however there is no extra responses with IIS <-> Firefox log
I've tested libcurl (with the new IIS <-> libcurl (unpatched, static 'cnonce') log
And finally I've tested patched libcurl (new IIS <-> libcurl (patched, variable 'cnonce') log
Probably Summary: seems that IIS implemented correctly Full logs are available. I can upload them somewhere if needed. |
Obviously, Chrome/Chromium implemented Firefox seems to have the same bug (or, most probably, the bug from Firefox was copied to Chrome). Both major browsers have the same bug. The patch brings the best combination: variable |
9350ff0
to
6a3a97f
Compare
To simplify review, I've moved some commits to other PRs and removed from this one. |
6a3a97f
to
7a81e99
Compare
Rebased |
7a81e99
to
cf35f5d
Compare
Rebased again. I opened partially related Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1344488 |
RFC 7616 (and 2617) requires to use the first nonce and cnonce values for A1 calculation until the next Digest challenge received.
cf35f5d
to
fb72324
Compare
I've rebased your branch on upstream/master. Regarding the last commit I don't understand what it fixes. Can you please add a commit message that explains why it is necessary? I don't see the advantage to changing it, and I don't see that the RFC requires it. Could there be a compatibility issue from doing this? digest: generate new random cnonce every time when cnonce is used |
The last commit adds Unfortunately seems that RFC assumes that From https://datatracker.ietf.org/doc/html/rfc7616#section-3.3: From https://datatracker.ietf.org/doc/html/rfc7616#section-3.4: Man-In-The-Middle may collect many requests with the same In contract, RFC defines how and when Similar discussion was summirised nicely at the FireFox bug description: https://bugzilla.mozilla.org/show_bug.cgi?id=270447#c10 |
cnonce is supposed to be unique for every request. Reuse of cnonce makes password protection weaker.
fb72324
to
fea2127
Compare
Currently (without this PR merged) libcurl requests with With As this PR fixes Every algorithm which uses In short: any valid implementation of HTTP Digest Auth must work just fine with variated |
As a reference: both Firefox and Chrome (Chromium) generate new |
On Thu, Jun 30, 2022 at 03:18:19PM -0700, Karlson2k wrote:
- there is no security breach in current form, it
only improves security by making it harder to decode username and password
by sniffing traffic
If it's harder to sniff with this patch, then that makes it easier to sniff
now, in the current code. The question is, how easy?
|
Yes, it is not simple, but the idea of nonce is to add huge difference to the source data. When |
Similar to the other Digest PR: Can we start with the problem situation so that we all can understand what the change is needed for? Then we get a common base and can discuss what alternatives we have to fix the problem. This PR immediately got deep into the weeds and I fear we miss details then. |
This PR fixes a few things related to "session" digest HTTP authorisation:
H(A1)
value and reuse it for the next requestscnonce
whencnonce
is used in calculation (for both session and non-session)Some commits were moved to separate PRs:
I've made some investigation.
See #9074 (comment) and #9074 (comment)