Skip to content

Fix auth digest refcount integer overflow #585

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

Closed
wants to merge 3 commits into from

Conversation

desbma-s1n
Copy link
Contributor

@desbma-s1n desbma-s1n commented Mar 31, 2020

This fixes a possible overflow of the nonce reference counter in the
digest authentication scheme, found by security researchers
@synacktiv.

It changes references to be an 64 bits unsigned integer. This makes
overflowing the counter impossible in practice.

@squid-prbot
Copy link
Collaborator

Can one of the admins verify this patch?

@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Mar 31, 2020
... in busy long-running Squids.

An impractical-to-overflow counter is also easier to deal with in code.
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thanks a lot of working on this bug! I have committed two adjustments. Please see if you agree with them and revert or adjust further as needed.

If you accept my changes, we will need to adjust the PR description accordingly. That description will become the future commit message (and needs reformatting anyway).

Unlike your initial version, I suspect that my change will increase the nonce structure size a bit, but I do not think that increase is important. If it is important, then the whole structure should be refactored to reduce waste anyway.

@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Mar 31, 2020
@rousskov rousskov changed the title Fix possible auth digest refcount integer overflow Fix auth digest refcount integer overflow Mar 31, 2020
@rousskov
Copy link
Contributor

OK to test

@yadij
Copy link
Contributor

yadij commented Mar 31, 2020

Typically a nonce reference count should not exceed 7 (client HTTP request, server HTTP request, ICAP request, ICAP response, HTTP cache, auth credentials cache, transaction ALE) - plus or minus a few transaction state objects.

2^16 is far larger than this particular counter should ever reach, even assuming the admin sets some reasonably large re-use count for nonce'. I think we would be better served adding overflow checks rather than increasing the limit like this. That can be achieved by converting this struct to a class and adding a RefCount Pointer rather than this ad-hoc reference counting (which is long overdue anyway).

If you wish to take that on please do. It is relatively simple work, though time consuming to find all the code lines needing tweaks for the new ref-count API.

@rousskov
Copy link
Contributor

I think we would be better served adding overflow checks rather than increasing the limit like this.

What makes overflow checks superior to a never-overflowing counter?

If you wish to take that on please do.

FWIW, I am not interested in doing that work at this time, but can undo my polishing commits if they are in the way.

@yadij
Copy link
Contributor

yadij commented Mar 31, 2020

I think we would be better served adding overflow checks rather than increasing the limit like this.

What makes overflow checks superior to a never-overflowing counter?

I this case; detection of suspicious activity.

If you wish to take that on please do.

FWIW, I am not interested in doing that work at this time, but can undo my polishing commits if they are in the way.

They are not a problem either way.

FTR; If none wants to take on the full update I am okay with this going in as-is.

@desbma-s1n
Copy link
Contributor Author

2^16 is far larger than this particular counter should ever reach, even assuming the admin sets some reasonably large re-use count for nonce'.

We have developed a proof of concept internally.
We send a lot a requests to cause the overflow of the signed integer which is undefined behavior but typically causes the integer to wrap around to -2^16.
Then the code skips the decrementations because of the < 0 check in authDigestNonceUnlink. We can carefully control when the counter reaches 0 again, to free the nonce while a request is still in flight, which can be exploited to cause a use after free.

Please see if you agree with them and revert or adjust further as needed.

Looks good to me although the 64 bits are probably overkill.

To reach such value would require a number of open file descriptors that is simply not possible.
By default squid is compiled with with-filedescriptors=65536, that number can be increased so is for example the default Linux ulimit but can any OS really handle 2^32 file descriptors?

@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Mar 31, 2020
@desbma-s1n
Copy link
Contributor Author

If you accept my changes, we will need to adjust the PR description accordingly. That description will become the future commit message (and needs reformatting anyway).

I have updated the description as requested.

Unlike your initial version, I suspect that my change will increase the nonce structure size a bit, but I do not think that increase is important. If it is important, then the whole structure should be refactored to reduce waste anyway.

On 64 bits architectures, it should not change the struct layout, as both the field before and after are already 64bits wide.

@rousskov
Copy link
Contributor

rousskov commented Mar 31, 2020

Alex: What makes overflow checks superior to a never-overflowing counter?

Amos: detection of suspicious activity.

IMO, comparable detection is possible and, in fact, is easier to do correctly (as well as report more useful details) when there are no overflows.

Amos: Typically a nonce reference count should not exceed 7

Author: We send a lot a requests to cause the overflow of the signed integer

Clearly, the possible range of this particular counter is rather uncertain/mysterious. Unless there is a provable certainty that this counter cannot exceed the number of file descriptors, even in the presence of transaction leaks and other "minor" bugs in the surrounding code, I think it is best to go with a 64-bit counter that cannot overflow even under those special circumstances. Fewer things to worry about...

With Amos' blessing, I am clearing this PR for merging. If automated tests pass, the PR will be auto-merged in a few days. @desbma-s1n, thanks again for working on this bug!

@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-author author action is expected (and usually required) labels Mar 31, 2020
squid-anubis pushed a commit that referenced this pull request Apr 2, 2020
This fixes a possible overflow of the nonce reference counter in the 
digest authentication scheme, found by security researchers 
@synacktiv.

It changes `references` to be an 64 bits unsigned integer. This makes 
overflowing the counter impossible in practice.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Apr 2, 2020
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 2, 2020
squidadm pushed a commit to squidadm/squid that referenced this pull request Apr 12, 2020
This fixes a possible overflow of the nonce reference counter in the
digest authentication scheme, found by security researchers
@synacktiv.

It changes `references` to be an 64 bits unsigned integer. This makes
overflowing the counter impossible in practice.
yadij pushed a commit that referenced this pull request Apr 13, 2020
This fixes a possible overflow of the nonce reference counter in the
digest authentication scheme, found by security researchers
@synacktiv.

It changes `references` to be an 64 bits unsigned integer. This makes
overflowing the counter impossible in practice.
squidadm pushed a commit to squidadm/squid that referenced this pull request Apr 13, 2020
This fixes a possible overflow of the nonce reference counter in the
digest authentication scheme, found by security researchers
@synacktiv.

It changes `references` to be an 64 bits unsigned integer. This makes
overflowing the counter impossible in practice.
yadij pushed a commit that referenced this pull request Apr 14, 2020
This fixes a possible overflow of the nonce reference counter in the
digest authentication scheme, found by security researchers
@synacktiv.

It changes `references` to be an 64 bits unsigned integer. This makes
overflowing the counter impossible in practice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants