calculateMac() should not be deprecated and split into two different versions

[shawm11]

The calculateMac() in the crypto.js file should not deprecated, and I propose the changes made in this pull request be reverted.

Calculating the payload hash before the MAC in the way that is done in calculateServerMac() does not make sense. For the server, the MAC calculation verifies that the client-provided artifacts (which includes the payload hash) in the Authorization request header were not tampered with or corrupted. If the MAC in the header is bad, there should be no attempt to validate the payload because the payload hash cannot be trusted and the server should immediately reject the request from the client. By validating the payload before verifying the MAC, servers using calculateServerMac() could unnecessarily expend processing time and resources by calculating the payload hash for a request that is to be rejected because of a bad MAC even if the payload passes validation.

The name generateRequestMac() is misleading because the calculation is not only for creating a MAC for a client request header. A server can use the same type of calculation to create a MAC that is included in the Server-Authorization header of its response. The client can then check the MAC in the header, including the server's payload hash if the server chose to include a payload hash. The documentation clearly states this.

An Aside About Payload Validation

There seems to be some misunderstanding about the purpose of the payload validation and why it is optional. As stated in the Payload Validation section of the documentation, a valid payload hash (or valid MAC) alone does not guarantee the payload is valid and does not guarantee the client is allowed to access the resource it is requesting. The server attempting to validate the payload in some cases, such as a GET request from a client, would not make sense because there is supposed to be no payload and therefore nothing to validate.

Payload validation can provide an optional redundant layer of security against certain man-in-the-middle (MITM) tampering attacks. SSL/TLS also provides protection against MITM attacks, and most modern REST API services require SSL/TLS. Eran Hammer authored Hawk with the understanding that most REST API services would be using SSL/TLS and many of those would want to skip the redundant protection provided by payload validation to save time and computing resources. At the time he authored Hawk, Eran Hammer was a proponent in the "security in layers" where a system does not rely on a single mechanism, such as SSL/TLS, for protection. The security in layers by having both payload validation and SSL/TLS is optional because it may not be suitable for all systems. It is not a security vulnerability. The documentation states the advantages of payload validation and warns of its limitations.

As mentioned in the Security Considerations, payload validation does not replace SSL/TLS either because an attacker can still intercept communication and view the payload as unencrypted plain text if SSL/TLS is not used, which can be a problem if the payload contains sensitive or secret data that is not to be leaked to a third-party. However, if SSL/TLS is unavailable or unreliable, payload validation may provide sufficient or better-than-nothing protection against MITM attacks if the payload in clear text does not contain sensitive or secret data or the payload is encrypted data.

[chrisdlangton]

The entire post reads like a demand for preference prioritising.

Consider that the change is linked to a PR and issue and take a look over those for the missing context in this issue.

While the OP points out that there is in fact more than one use case, by mentioning option body integrity - the OP contradicted the OP own point when the OP demanded to roll back the changes that take away this optional validation.

It's worth pointing out that the deprecation is due to a split where 2 distinct use cases exist and 1 method had attempted and failed to implement both. The PoC exploit demonstrates the flaw, and the issue tracks other languages (e.g. the successor in Rust and the FXA in TypeScript) that do not have such a flaw present for the same Hawk protocol.

Given this, splitting up the method for each use case was the decision of maintainers - who like the OP wanted to maintain current functionality (no security mechanism that provide assurance) and the optional functionality the protocol should have, and was present in other libraries.

Finally, had the OP followed the code in the deprecation notice, the OP would see that one of the new methods is nothing more than a rename, preserving the functionality perfectly.

This was necessary because an in place rename would have some users that expected one use case (OP preference) to be no change where the other use case would have no change and remain vulnerable.

Therefore if the OP dislikes integrity validation the OP can rename the method to the new one before the next major version (where breaking changes are expected) and it performs the same functionality, preserving all functionality before and after deprecation.

The OP can also choose keep the backwards compatible reference to the deprecated method and just ignore the deprecation notice. It's unlikely the lib will spring back to life and actually release a major version update where deprecations are removed and actually break backwards compatibility at that time.

Or consider using a library designed for payload integrity validation and adopt the new method that implements it - the library otherwise is cruft that provides no security assurance at all, but that's each user's own call

[shawm11]

The entire post reads like a demand for preference prioritising.

Consider that the change is linked to a PR and issue and take a look over those for the missing context in your issue.

I have read the pull request and the issue. It is still unclear to me what problem the pull request is supposed to solve.

While you point out that there is in fact more than your use case, by mentioning option body integrity - you contradicted your own point when you demanded to roll back the changes that take away this optional validation.

Rolling back the changes introduced by that pull request does not take away the optional payload validation. Payload validation was already there and functional before those changes. The changes force the server calculate the payload hash simply because the client provided one, even if the server policy does not entail payload validation.

If the server policy does entail payload validation, the changes from that pull request force the the server's authenticate() method to calculate the payload hash twice before calculating the MAC with no additional benefit. Calculating the payload hash in calculateServerMac() merely shifts the "trust" from the payload hash to the request payload. The payload is just as untrustworthy as the payload hash. Therefore, the extra payload hash calculation introduced by the pull request does not improve the security of the protocol.

The MAC calculation is for checking if the set of artifacts, which may include the payload hash, provided by the client is actually from the client and has not been tampered with. The payload hash is not and does not need to be trusted when calculating the MAC. Checking the MAC should not depend on validating the payload because Hawk allows for instances where the payload is not available within the same request as the payload hash.

It's worth pointing out that the deprecation is due to a split where 2 distinct use cases exist and 1 method had attempted and failed to implement both. The PoC exploit demonstrates the flaw, and the issue tracks other languages (e.g. the successor in Rust and the FXA in TypeScript) that do not have such a flaw present for the same Hawk protocol.

What are the two use cases that the earlier code failed to implement?

Given this, splitting up the method for each use case was the decision of maintainers - who like yourself wanted to maintain current functionality (no security mechanism that provide assurance) and the optional functionality the protocol should have, and was present in other libraries.

The MAC calculation that is done by both the client and the server is the primary security mechanism that provides assurance. That is true with or without the changes introduced by the pull request. The MAC calculation allows for the a request/response without the proper authentication to be rejected as "unauthorized" without the need to process the payload. Payload validation simply provides optional extra assurance. The protocol is secure without it. I am not sure what libraries you are referring to, but that is how it is with other implementations of the Hawk protocol.

Finally, had you followed the code in the deprecation notice, you would see that one of the new methods is nothing more than a rename, preserving the functionality perfectly.

Why are two new methods needed? Payload validation is optional for both the client and the server. The client can choose to validate the server response payload and the server can choose to validate the client request payload. If the server or the client choose not to validate the payload, the protocol is still secure because it still achieves its main objective: client-server authentication. This makes the warning in calculateServerMac() simply incorrect. Using the payload hash provided by the client does not mean there is no integrity checking.

This was necessary because an in place rename would have some users that expected one use case (your preference) to be no change where the other use case would.. have no change and remain vulnerable.

Remain vulnerable to what? In what scenario was the protocol vulnerable?

Therefore if you dislike integrity validation you can rename the method and it performs the same functionality. You can keep the backwards compatible reference to the deprecated method and ignore the deprecation notice. It's unlikely the lib will spring back to life and actually release a major version update where deprecations are removed and actually break backwards compatibility at that time.

Requiring developers to modify the code of an imported library for a basic usage destroys the purpose of the library. Also, protocols are usually not updated often because a good protocol typically does not need to be updated very often. Because Hawk is a protocol specified by a code implementation, maintenance updates to that code are required from time to time.

Or consider using a library designed for payload integrity validation and adopt the new method that implements it - the library otherwise is cruft that provides no security assurance at all, but that's your call

The Hawk protocol without the pull request changes provided payload integrity validation as an option for years. If extra payload protection is desired, use TLS in addition to Hawk payload validation. Again, the security assurance in the Hawk protocol is primarily provided by the MAC in the header, not the payload hash.

[chrisdlangton]

I have read the pull request and the issue. It is still unclear to me what problem the pull request is supposed to solve.

How about the linked issue to the python library? or the bugzilla referenced for the bounty which awarded Hall of Fame?

Rolling back the changes introduced by that pull request does not take away the optional payload validation.

The rest of this section ignores the last point, it was proven and awarded as a valid finding, check out the PoC against the version with the problem.

I highly encourage the OP and maintainer of the PHP Hawk implementation to review this issue here in this library. I suspect that PHP lib also has this vulnerability too - if they had copied this, FXA, or the Python implementations of Hawk.

Should I continue this quote, response thread? It would be beneficial if the OP would like to get up to speed and try starting again.

Alternatively use the function as is, ignore the deprecation notice, and if in future there ever is the unlikely major version release - then, at that time, consider adopting the other methods for each of their use cases when the major release drops deprecated methods.

[djmitche]

I'll catch up on the content here but I want to remind everyone that this repo is covered by the Mozilla community participation guidelines. Please keep the conversation on the issues and not the people involved.

[chrisdlangton]

Happy to replace "you" with "OP" or "the maintainer", less concise but if it's a policy I apologise for missing this.

Let's keep this on track, deprecation is standard.
Backwards compatibility was maintained.
Unless there's a major version release, discussing method names in retrospect to a merge is pointless - rolling back breaks backwards compatibility now for users using new methods.

To be clear, a roll back now is actually a major version bump, right?

[fkiriakos07]

After internal discussions regarding this Github issue and the original Github issue, we have decided to revert the changes introduced in PR #290, and archive this repository.

Based on the analysis conducted, we confirmed that all possible scenarios have been already accounted for in regards to validation of a payload hash and its associated authentication process.

Four situations are possible:

• Hash and the Payload are present
  • Validation calculates payload hash and fails if provided hash doesn’t match
• Hash is present but Payload is not
  • Only hash value is being verified against “mac” in authorization header
• Hash is not present but Payload is
  • Validation fails, because hash is not present
• None of those provided
  • No validation happens, case is covered

Previous analysis also concludes that the first case has no false negatives or positives.

All scenarios are covered:

• If the hash in the hawk header was modified in flight the MAC validation will fail
• If the hash and the payload are modified in flight the MAC validation will fail
• If only the payload was modified in flight the MAC validation will pass, but the payload validation will fail
• If the hash doesn't match the payload hash for any other reason payload validation will fail

Payload validation section in specification explains that payload validation is optional and server implementation is to decide whether to enforce it or not.

Why We Are Reverting #290

The Hawk library recently underwent modifications through pull request #290 that introduced a new approach to payload hash validation. Despite the well-intentioned effort to improve security, after careful reviews and discussions with the security team, we have decided to revert these changes for the following reasons:

1. The changes have not demonstrated any clear vulnerability that they mitigate. The original design already accounted for potential security risks appropriately.

2. The introduction of payload hash validation before verifying the MAC introduces an unnecessary server load and potential for Denial of Service (DoS) vulnerabilities as it adds additional steps that process the payload prematurely.

3. The implementation of these changes led to console warnings that may be confusing to users and developers and ultimately reduces the library's usability and clarity.

4. No releases have been deployed since the merge of #290, so reverting these changes won't impact current users. This allows us to maintain stability and backward compatibility.

5. Retaining these changes would cause this repository to diverge from the existing implementations in other languages

Why We Are Archiving the Repository

We are moving the repository to an archived status because both the protocol and documentation have reached completion, with no further updates or changes necessary.

It is also consistent with the approach we took with our Python library — which was also archived.

Archiving the repository offers a clear signal to the community that while the existing code will remain available for reference or fork, no further updates or support should be expected.

Moving Forward

We understand that these decisions may be met with varying perspectives, and we appreciate the community's passion and contributions. It is our hope that by taking these steps now, we can ensure a more secure and sustainable future for authentication practices.

We thank everyone involved for their understanding, and we look forward to continuing our collective effort to build safe and effective tools for the Mozilla community and beyond.

[chrisdlangton]

TL;DR this has re-introduced a known mitm attack vector, that was deemed a risk by the bug bounty program reviewers. A Hall of Fame acknowledgement was granted because there is PoC exploit, and only after I supplied this patch and it was merged.

Ultimately, it is of no value for me to see this tremendously risky, proven, authentication bypass vulnerability remediated now - I repeat, I get nothing from posting this update, patched or unpatched I am not a hawk user and have already been awarded the bounty, this makes no difference to me.

I only post again for the sake of USERS BEWARE this was a shortshighted decision made by maintainers who seem to be splittered off from Mozilla and willfully ignorant to Mozilla risk-based decision making practices.

---

Previous analysis also concludes that the first case has no false negatives or positives.

It wasn't "analysis" at all, if so, what was tested? What were the rests? Are there both positive and negative testing with a null hypothesis to show the analysis was unbiased?

No, it was just a complaint without basis in any actual testing or rationale.

No data-driven or risk-based thinking was applied, nor testing of any description.

A complaint is self-serving to the individual and a minority at most, whereas the bug bounty program and formal risk management practices were serving all of Mozilla end-users - users who aren't involved in a mere Github complaint like this..

---

Let's address each misguided rationale from "Why We Are Reverting"

1. The changes have not demonstrated any clear vulnerability that they mitigate. The original design already accounted for potential security risks appropriately.

This is a misunderstanding - no changes can "demonstrate" vulnerabilities - they only remediate them.

What does demonstrate a vulnerability?
The PoC which was submitted to the mozilla bug bounty program;
Gist PoC

2. The introduction of payload hash validation before verifying the MAC introduces an unnecessary server load and potential for Denial of Service (DoS) vulnerabilities as it adds additional steps that process the payload prematurely.

To break that down, there's some security jargon used here, and the rationale is convoluted

potential for Denial of Service (DoS) vulnerabilities

Show a PoC or it is a misguided theory based on perverse incentives.

A misguided theory because a library that is designed to do cryptographic operations must perform.. cryptographic operations of course! Other than a percieved DoS as a result of intended cryptographic operations, you should at least PoC the undesired functionality and report that Bugzilla, anything less is just a unfounded claim, and less than what was provable in the bug bounty report, PoC, and this patch.

This is a segue to this part:

The introduction of payload hash validation before verifying the MAC introduces an unnecessary server load

Cryptographic operations inherent in the design, and present in libraries by Mozilla in other programming languages (Go, Rust, Python) as well as third party mohawk in this language.

Until the server generates a signature it has no possibility to perform signature validation at all.

A risk decision that has been made already. i.e. a MITM is mitigated because known MITM risks outweigh perceived DoS

The only thing "unnecessary" here, is the entire library serving no necessary function at all unless it generates a signature on the server to compare because using client provided signature is not verifying the signature at all, thus the PoC proves the mitm attack vector.

3. The implementation of these changes led to console warnings that may be confusing to users and developers and ultimately reduces the library's usability and clarity.

Deprecation notices are a standard practice and actually a positive usability feature, by calling them warnings as a negative, rather than what they actually are, you are clearly misrepresenting a usability feature as a problem and do not have a valid point that any reasonable developer would agree with.

Call them what they are, a usability feature, a depreacation notice. Do not mislead the public with FUD to make a point for personal gain.

4. No releases have been deployed since the merge of Security fix #290, so reverting these changes won't impact current users. This allows us to maintain stability and backward compatibility.

No releases because the library is not actively maintained.

This allows us to maintain stability and backward compatibility.

Backward compatibility is not inhibited, again with the FUD - or is is will-full lies at this point?

5. Retaining these changes would cause this repository to diverge from the existing implementations in other languages

Show some evidense, this patch made this library consistent with other hawk libraries by Mozilla in other programming languages (Go, Rust, Python) as well as third party mohawk in this language. This analysis was actually provided in the bugzilla thread, had you checked and not just made unfounded claims you would learn the patch is not needed in those because the vulnerability is not present at all to be patched.

References

• Gist PoC
• The incoming hash of the payload is being trusted and not verified #284 for Javascript
• mozilla-services/hawkauthlib
• mozilla/fxa (typescript) as a reference implementation for workarounds.
• bugzilla id 1719614 - Incorrect hash validation in hawk library tracking the specification weakness with goal of identifying vulnerabilities where it is implemented.
• bugzilla id 1719964 - Incorrect hash validation in Firefox Accounts containing the PoC
• disclosure timeline

[chrisdlangton]

Consider that you have this statement

This library is in "maintenance mode" -- no features will be added, and only security-related bugfixes will be applied.

You may choose to reconsider the patch, or perhaps remove the misleading statement