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 auth fixes #8912

Closed
wants to merge 3 commits into from
Closed

Digest auth fixes #8912

wants to merge 3 commits into from

Conversation

Karlson2k
Copy link
Contributor

@Karlson2k Karlson2k commented May 25, 2022

RFC 7616 (and 2617) requires values to be "unquoted" before used for digest calculations. The same mentioned in the code comments.
realm="DOMAIN\\host" and realm=DOMAN\\host are different realms, but they are processed as identical values at the moment, which is not compliant with RFC. See RFC7616.
The only place where unquoting can be done correctly is header parsing function.
This PR adds unquoting (de-escaping) of all values during header parsing and quoting (escaping) of the values during header forming. This approach should be most straightforward and easy to read/maintain as all values are processed in the same way as required by RFC.

The second commit adds detection of more erroneous situation during header parsing.
The third commit adds fix for NULL dereference discovered by Fuzzer. Server header may not define realm, so code must tolerate NULL value for digest->realm.

Can be squashed into single commit.

@Karlson2k Karlson2k force-pushed the digest_auth_fix_01 branch 4 times, most recently from bf044a1 to 5eae1c8 Compare May 25, 2022
@Karlson2k
Copy link
Contributor Author

@Karlson2k Karlson2k commented May 25, 2022

Failed tests:

The PR looks to be ready.

@Karlson2k
Copy link
Contributor Author

@Karlson2k Karlson2k commented May 26, 2022

The last patch also fixed wrongly hashed (nil) string when realm is not present in header from server.

@Karlson2k
Copy link
Contributor Author

@Karlson2k Karlson2k commented May 26, 2022

Rebased

@Karlson2k Karlson2k changed the title Digest auth fix Digest auth fixes May 26, 2022
Karlson2k added 3 commits May 30, 2022
RFC 7616 (and 2617) requires values to be "unquoted" before used for
digest calculations. The only place where unquoting can be done
correctly is header parsing function (realm="DOMAIN\\host" and
realm=DOMAN\\host are different realms).
This commit adds unquoting (de-escaping) of all values during header
parsing and quoting of the values during header forming. This approach
should be most straightforward and easy to read/maintain as all values
are processed in the same way as required by RFC.
Invalid headers should not be processed otherwise they may create
a security risk.
Server headers may not define "realm", avoid NULL pointer dereference
in such cases.
@Karlson2k
Copy link
Contributor Author

@Karlson2k Karlson2k commented May 30, 2022

Rebased again.

bagder
bagder approved these changes Jun 2, 2022
@bagder
Copy link
Member

@bagder bagder commented Jun 2, 2022

Thanks!

@bagder bagder closed this in 3a6fe0c Jun 2, 2022
bagder pushed a commit that referenced this issue Jun 2, 2022
Invalid headers should not be processed otherwise they may create
a security risk.

Closes #8912
bagder pushed a commit that referenced this issue Jun 2, 2022
Server headers may not define "realm", avoid NULL pointer dereference
in such cases.

Closes #8912
@Karlson2k Karlson2k deleted the digest_auth_fix_01 branch Jun 2, 2022
@Karlson2k
Copy link
Contributor Author

@Karlson2k Karlson2k commented Jun 2, 2022

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants