Skip to content

Hash Digest noncedata #491

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

Conversation

squidcontrib
Copy link
Contributor

@squidcontrib squidcontrib commented Oct 10, 2019

These commits together

  1. Hash the noncedata for Digest nonces before encoding,
    to match the documentation.
  2. Encode Digest nonces using hex, rather than base64.

David Fifield added 2 commits October 10, 2019 16:41
This makes the code match the documentation comment:
    /*
     * We use H(nonce_data) so the nonce is meaningless to the reciever.
     * So our nonce looks like base64(H(timestamp,pointertohash,randomdata))
     */
This is to reduce the risk of client parsing errors caused by the
nonce-string containing a '=' byte.

Hexadecimal encoding also matches the examples in RFC 2617.
@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 Oct 10, 2019
@yadij
Copy link
Contributor

yadij commented Oct 11, 2019

OK to test

@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Oct 11, 2019
HASH H;
SquidMD5Init(&Md5Ctx);
SquidMD5Update(&Md5Ctx, reinterpret_cast<const uint8_t *>(&nonce->noncedata), sizeof(nonce->noncedata));
SquidMD5Final((unsigned char *) H, &Md5Ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid C-style casts in his C++ code. Also the H parameter here is an uint8_t[] in the md5.h API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to a reinterpret_cast<uint8_t *>.

The API in md5.h specifies uint8_t[16], but in md5.c it's unsigned char[16].

SQUIDCEXTERN void SquidMD5Final(uint8_t digest[16], struct SquidMD5Context *context);
void
SquidMD5Final(unsigned char digest[16], struct SquidMD5Context *ctx)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, okay, that can be fixed later then.

David Fifield added 2 commits October 14, 2019 11:24
Suggested here:
squid-cache#491 (comment)

Not casting doesn't work:
```
Config.cc: In function ‘void authDigestNonceEncode(digest_nonce_h*)’:
Config.cc:118:22: error: invalid conversion from ‘void*’ to ‘char*’ [-fpermissive]
     CvtHex(H, nonce->key);
               ~~~~~~~^~~
```
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 14, 2019
I missed these earlier because I was looking for "base64", not "b64".
@squidcontrib
Copy link
Contributor Author

I just pushed another change to rename identifiers: UserRequest.nonceb64 to UserRequest.noncehex and authenticateDigestNonceNonceb64 to authenticateDigestNonceNonceHex. I missed these earlier because I only searched for "base64", not "b64". Strangely, the documentation comment for UserRequest.nonceb64 was already a hex string.

@rousskov rousskov added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Oct 16, 2019
@yadij yadij added S-waiting-for-more-reviewers needs a reviewer and/or a second opinion and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Oct 18, 2019
Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

LGTM, some style changes which really should happen but I'm not going to block on those.

{
digest_nonce_h *nonce = NULL;

if (nonceb64 == NULL)
if (noncehex == NULL)
Copy link
Contributor

@yadij yadij Oct 18, 2019

Choose a reason for hiding this comment

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

Suggested change
if (noncehex == NULL)
if (!noncehex)


if ((nonce == NULL) || (strcmp(authenticateDigestNonceNonceb64(nonce), nonceb64)))
if ((nonce == NULL) || (strcmp(authenticateDigestNonceNonceHex(nonce), noncehex)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ((nonce == NULL) || (strcmp(authenticateDigestNonceNonceHex(nonce), noncehex)))
if (!nonce || strcmp(authenticateDigestNonceNonceHex(nonce), noncehex) != 0)

@@ -23,7 +23,7 @@
#include "SquidTime.h"

Auth::Digest::UserRequest::UserRequest() :
nonceb64(NULL),
noncehex(NULL),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
noncehex(NULL),
noncehex(nullptr),

@yadij yadij added the M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels label Oct 18, 2019
squid-anubis pushed a commit that referenced this pull request Oct 20, 2019
These commits together
1. Hash the noncedata for Digest nonces before encoding,
   to match the documentation.
2. Encode Digest nonces using hex, rather than base64.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Oct 20, 2019
@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 Oct 21, 2019
squidadm pushed a commit to squidadm/squid that referenced this pull request Nov 2, 2019
These commits together
1. Hash the noncedata for Digest nonces before encoding,
   to match the documentation.
2. Encode Digest nonces using hex, rather than base64.
yadij pushed a commit that referenced this pull request Nov 2, 2019
These commits together
1. Hash the noncedata for Digest nonces before encoding,
   to match the documentation.
2. Encode Digest nonces using hex, rather than base64.
squidcontrib pushed a commit to squidcontrib/squid that referenced this pull request Jan 29, 2020
This is a follow-up to squid-cache#491, which hashed what was previously revealed
as plaintext. Removing the pointer from the input to the hash removes
the possibility that someone could recover a pointer by reversing a
hash. Having the pointer as input was not adding anything: Squid
remembers all outstanding nonces, so it really only requires uniqueness,
which is already guaranteed by the authenticateDigestNonceFindNonce
loop.
squid-anubis pushed a commit that referenced this pull request Feb 8, 2020
This is a follow-up to #491 (b20ce97), which hashed what was previously
revealed as plaintext. Removing the pointer from the input to the hash
removes the possibility that someone could recover a pointer by
reversing a hash. Having the pointer as input was not adding anything:
Squid remembers all outstanding nonces, so it really only requires
uniqueness, which is already guaranteed by the
authenticateDigestNonceFindNonce loop.
squidadm pushed a commit to squidadm/squid that referenced this pull request Mar 18, 2020
This is a follow-up to squid-cache#491 (b20ce97), which hashed what was previously
revealed as plaintext. Removing the pointer from the input to the hash
removes the possibility that someone could recover a pointer by
reversing a hash. Having the pointer as input was not adding anything:
Squid remembers all outstanding nonces, so it really only requires
uniqueness, which is already guaranteed by the
authenticateDigestNonceFindNonce loop.
yadij pushed a commit that referenced this pull request Mar 19, 2020
This is a follow-up to #491 (b20ce97), which hashed what was previously
revealed as plaintext. Removing the pointer from the input to the hash
removes the possibility that someone could recover a pointer by
reversing a hash. Having the pointer as input was not adding anything:
Squid remembers all outstanding nonces, so it really only requires
uniqueness, which is already guaranteed by the
authenticateDigestNonceFindNonce loop.
squidadm pushed a commit to squidadm/squid that referenced this pull request Mar 26, 2020
This is a follow-up to squid-cache#491 (b20ce97), which hashed what was previously
revealed as plaintext. Removing the pointer from the input to the hash
removes the possibility that someone could recover a pointer by
reversing a hash. Having the pointer as input was not adding anything:
Squid remembers all outstanding nonces, so it really only requires
uniqueness, which is already guaranteed by the
authenticateDigestNonceFindNonce loop.
yadij pushed a commit that referenced this pull request Mar 29, 2020
This is a follow-up to #491 (b20ce97), which hashed what was previously
revealed as plaintext. Removing the pointer from the input to the hash
removes the possibility that someone could recover a pointer by
reversing a hash. Having the pointer as input was not adding anything:
Squid remembers all outstanding nonces, so it really only requires
uniqueness, which is already guaranteed by the
authenticateDigestNonceFindNonce loop.
@rousskov rousskov removed the S-waiting-for-more-reviewers needs a reviewer and/or a second opinion label May 12, 2020
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.

5 participants