The incoming hash of the payload is being trusted and not verified

[chrisdlangton]

Cross-referencing the vulnerability in mozilla-services/hawkauthlib and mozilla/PyHawk as it exists here also.

To understand the vulnerability in this repository, you should follow Crypto.calculateMac() https://github.com/mozilla/hawk/blob/main/lib/crypto.js#L44

It leverages generateNormalizedString() https://github.com/mozilla/hawk/blob/main/lib/crypto.js#L54

and includes an options.hash for the calculateMac (i.e. Mac Signature requires the payload hash, if the hash was provided)
https://github.com/mozilla/hawk/blob/main/lib/crypto.js#L71

It will depend on where options originated to determine if there is any assurance of a security controls provided (or lack thereof which is called a vulnerability). So we follow artifacts which was passed as options to Crypto.calculateMac() and we see artifacts populated artifacts.hash using attributes.hash https://github.com/mozilla/hawk/blob/main/lib/server.js#L113

So where is the origin of attributes? We need the origin to gain any insight of the assurance / vulnerability, right?
We see attributes set here https://github.com/mozilla/hawk/blob/main/lib/server.js#L102

This shows that we take the client provided values to populate attributes

I.e. the server-side MAC calculation uses ONLY client provided values to calculate the MAC Signature on the server, it does not do it's own calculation on the server and simply trusts the client is authentic without doing any authenticity verification itself.

[dannycoates]

What is assured by the MAC is that the artifacts have not been tampered with not that the actual payload hash was used to generate the MAC. No assurance that a hash provided in the header matches the actual hash of the payload is assumed. The MAC assures that the artifacts are genuine and nothing more.

This shows that we take the client provided values to populate attributes

This is by design. If the MAC is valid and the artifacts contains a hash we know that the hash hasn't been tampered with and can be trusted to optionally validate the integrity of the payload.

It would be rather impractical to require the entire payload to be transferred and hashed before the MAC could be validated. This is why the hash value exists in the hawk header, so the MAC can be validated quickly and independently from the payload.

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

[chrisdlangton]

Everything you've written is true, according to the current state.

Does that make it secure?

To answer that, take a step back and think about what is secure, to be secure requires assurance, assurance provided by whom, to whom.

You wrote

If only the payload was modified in flight the MAC validation will pass

Which is factual, but incomplete. To be a useful statement it needs to be complete. This is the same statement with the key part you missed, to indicate the security characteristics of it

If only the payload was modified in flight after signed by the client the MAC validation which included a hash attribute will pass on the server

That's the factual truth currently,. What security characteristic applies when:

• Client signed with a payload hash
• Client expectation is by providing the hash, the server will perform payload validation
• Client recognises it's up to the server to perform payload validation or not, so a client includes the hash in the signature because MAC validation is not optional
• An insecurely configured server skips payload validation, that should occur first. That is its right
• The server uses only client provided values (blind trust) to perform Signature calculation, nothing is validated on the server

Tell the reader what is untruthful about the full description i provide. Then tell them where they gain confidence in the security of Hawk in it's current default usage.

The issue is nothing is actually validated on the server, it is merely assembled using client values and signed. That is not validation, and Hawk servers implementating this library gain no server assurance against modified request body when a hash of the body is signed and meant to be validated, and client's equally get no assurance by providing a hash or signing the hash in the MAC.

So why then is there a hash in the MAC at all? Why bother, do or do not, the security is the same.

Perhaps it was intended to be secure, but code currently has a flaw and it's insecure until fixed

[dannycoates]

* Client expectation is by providing the hash, the server will perform payload validation

This is not implied. The documentation clearly states:

It is up to the server if and when it validates the payload for any given request, based solely on its security policy and the nature of the data included.

* Client recognises it's up to the server to perform payload validation or not, so a client includes the hash in the signature because MAC validation is not optional

This is a bad assumption. Nowhere is it implied that the payload itself is protected by the mac, only the artifacts which may include the hash value.

* The server uses only _client provided values_ (blind trust) to perform Signature calculation, nothing is validated on the server

This is false. The server holds a secret for the given id that it uses to validate the mac. This is sufficient to determine if the artifacts are genuine. An attacker would need to obtain this secret in order to tamper with the artifacts without detection.

Hawk servers implementating this library gain no server assurance against modified request body when a hash of the body is signed and meant to be validated

If hawk is insecure in the way you describe it should be easy to prove. If you can show that a request can be tampered with and pass both mac and payload validation on the server then I will agree that it is insecure and we will work to fix it.

So why then is there a hash in the MAC at all?

It can be considered an optimization to improve mac verification performance. If the request passes mac validation the hash is known to be genuine and can be used to verify the payload. Without a hash in the hawk header, to achieve payload validation the entire payload would need to be hashed before the mac could be validated. Integrating the payload hash directly into the mac would implicitly make payload validation required.

To be definitive, what you've described so far is neither a bug nor a vulnerability when mac and payload validation are used as intended. I'm not interested in changing the hawk protocol or implementation merely to meet your proposed additional assurances because the existing protocol is sufficiently secure.

[chrisdlangton]

That can all be summarised as distinction without a difference.

Concisely, the distinction is 1. MAC Signature with hash without payload validation, and 2. Signatature with hash with payload validation. I.e. both have the hash but only 1 validates it is the distinction.

The 'without a difference' in distinction without a difference is both have the hash, and the security characteristics of the hash doesn't change. The hash representation of the payload only maintained it's secure characteristic if it is validated...

So all your word gymnastics only further prove to readers the point that currently the security characteristics of the hash in the MAC Signature are ignored by default. Thank you.

The MAC Signature maintenains the same security control with or without a payload hash. This is what is the security flaw that is easily exploited.

[chrisdlangton]

This is clearly described by CWE-642: External Control of Critical State Data achieved simply by the modification of payloads (in JavaScript via malicious plugin, unconfined Ad, or CSRF/XSS), or using HTTP Desync. Because the payload modifications are only detected by the server if the HMAC verification occurs on the server, and all modifications were done without knowledge of the secret needed to generate the HMAC because the original was signed by the real client who has the key the server decides to use the trusted client provided HMAC completely violating the core concept of Authentication entirely by relying on trusted client signatures.

We have undeniable proof now from the example of CWE-807: Reliance on Untrusted Inputs in a Security Decision that has been patched in FxA by doing the validation, ergo the authentication mechanism, on the server side. i.e. the entire point of HawkAuth was to have this built in, not optional. I digress.

The bounty has been tagged as a hall of fame bug now, so before the CVE is issued and a write up is published, you may consider rethinking your view on his issue and adapt the PR I raised to fix the flaw, or your own variation of that PR.

[chrisdlangton]

I adapted the PR for you.

Mozilla abandoned the Python repo, but this repo has 1.1 million NPM downloads a week.

I suggest this get patched, an Advisory added, and CVE requested.

If the the risk team see this, please find an active maintainer ASAP

[tomrittervg]

@chrisdlangton We are re-investigating this issue to provide an updated response. I have added your test to the current hawk repo (i.e. one without your patch) and it passes. Are you able to provide a similar test that fails against the original hawk repo and illustrates there is a scenario where tampered data passes both MAC and payload validation?

tom@t moz/hawk (main *) » npm test                                                                                       127 ↵

> hawk@9.0.1 test
> lab -a @hapi/code -t 100 -L


  
  ..................................................
  ..................................................
  ..................................................
  ..........................

176 tests complete
Test duration: 63 ms
Assertions count: 241 (verbosity: 1.37)
The following leaks were detected:AggregateError, atob, btoa, AbortController, AbortSignal, EventTarget, Event, MessageChannel, MessagePort, MessageEvent, performance
Coverage: 100.00%
Lint: No issues

tom@t moz/hawk (main *) » subl test/index.js                                                                               1
tom@t moz/hawk (main *) » npm test          

> hawk@9.0.1 test
> lab -a @hapi/code -t 100 -L


  
  ..................................................
  ..................................................
  ..................................................
  ...........................

177 tests complete
Test duration: 63 ms
Assertions count: 246 (verbosity: 1.39)
The following leaks were detected:AggregateError, atob, btoa, AbortController, AbortSignal, EventTarget, Event, MessageChannel, MessagePort, MessageEvent, performance
Coverage: 100.00%
Lint: No issues

[chrisdlangton]

The code being introduced into the PR is because, as you've discovered, the current code doesn't have the functionality 'to test'.

The vulnerability being shown in a unit test case would be trying to prove a negative, which I shouldn't need to point out proving a negative is never convincing as proving nonexistence is an oxymoron.

But if you have an endpoint you could use the PoC in my gist to check the issue as an end-to-end test case, rather than until testing for code that isn't there...

[tomrittervg]

the current code doesn't have the functionality 'to test'

Are you saying that the vulnerability is not solely in the hawk library, and rather in how the library is used in an application? It sounds like you are emphasizing the point that an application that omits payload validation is vulnerable and going further to say that it is a vulnerability that hawk can be used in that way.

[chrisdlangton]

You can see the 3 scenarios in the Gist PoC now

[chrisdlangton]

Hey @tomrittervg what is the status, or plan here? I see a few options:

1. Patch (PR) merged, NPM will get the patched version soon, including a GHSA for Dependabot + CVE issued by GitHub (recommended, expected, current best practice).
2. NPM will not get the patched version but the PR will be merged in this repo + CVE issued by Mozilla, no GHSA (unexpected, old best practice because Dependabot will not alert users).
3. NPM will not get the patched version but the PR will be merged in this repo + CVE issued by Mozilla, with GHSA (Not a bad compromise).
4. Patch will be abandoned and repo archived (following Hawk in other programming languages, i.e. python) - not recommended

Or (4) status quo; it will this remain semi-abandoned, maintenance-mode only - but not really doing any maintenance in reality.

The Bugzilla ticket stated Hawk will be archived - and Python has already done that.

Or was that ticket statement not referring to Hawk repositories being archived, and rather some other meaning of archive? It might have been specific about what it means by archive so I assume it was referring to the GitHub meaning, is that right?

[chrisdlangton]

@lotas will this repo be archived? If not, what was the meaning of the archived comment? Or has the archived decision changed now?

Will there be a GHSA added, and linked to the NPM package, and version that patched the issue? If not, why would you choose to not alert the 1.1 million weekly dependants to not get notified by NPM audit?

Will the GHSA also request a CVE from GitHub so there is a Dependabot alert? If not, why would you choose to not alert the unknown (too numerous number of pages of GitHub dependant projects) millions of repos not be notified by Dependabot?

[chrisdlangton]

Requesting an update please.
I realise 6 weeks is nothing compared to nearly 3 years late on advisory - but I stress that an advisory late is better than no advisory at all

I highly recommend the GHSA be created ASAP, you may use this as a guide (I am not asking you to copy it, it's just FYI to help speed this up for you), then I'll delete mine once yours is published.

[lotas]

Hi @chrisdlangton,

thank you once again for submitting #290 patch

@lotas will this repo be archived? If not, what was the meaning of the archived comment? Or has the archived decision changed now?

I don’t think this repo will be archived, as hawk is being used by Taskcluster still (Firefox CI)

Will there be a GHSA added, and linked to the NPM package, and version that patched the issue? If not, why would you choose to not alert the 1.1 million weekly dependants to not get notified by NPM audit?

After speaking with security team internally we came to a conclusion that HAWK API documentation made it clear that payload hash validation is optional and up to developer to implement. This way, users were already notified.

Further investigation of dependent packages at https://www.npmjs.com/browse/depended/hawk shows that most of the packages are either outdated, unmaintained or use HAWK for client-side only.

Looking into github’s dependency graph https://github.com/mozilla/hawk/network/dependents?package_id=UGFja2FnZS01MDY4NTM5OTU%3D it make it look as there are million of packages dependent on this, but deeper investigation shows that “hawk” library is being referenced as dependency of a dependency (of a dependency):

• For example deperecated request module was referencing hawk
• some repositories don’t even contain hawk in package.json/lock files, does github keeps dependency tree up to date?
• older versions of jest were using request which referenced hawk

To our awareness, at the time of changing ownership of hawk library, there were other known interested parties who were using hawk on the server side besides Mozilla.

Will the GHSA also request a CVE from GitHub so there is a Dependabot alert? If not, why would you choose to not alert the unknown (too numerous number of pages of GitHub dependant projects) millions of repos not be notified by Dependabot?

For the same reasons as above

[chrisdlangton]

Same reason as above

Saying 1.1 million NPM downloads is the same as the GitHub dependency graph

It is like saying water is harmless to humans, and acid must be the same.

Or we see no one buys cars from dealership's in Australia anymore so we'll stop shipping to dealership's worldwide where we have no data, just because we know it's the same everywhere

No

Just no

Try harder maybe?