Closed Bug 985766 Opened 10 years ago Closed 10 years ago

fx-auth-server HAWK payload verification discrepancies between implementation and onepw-protocol documentation

Categories

(Cloud Services :: Server: Firefox Accounts, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dchanm+bugzilla, Assigned: nalexander)

References

()

Details

Attachments

(1 file)

There are multiple discrepancies as to whether options.payload = true is required for certain HAWK APIs in the documentation. The current auth server code doesn't appear to require verification for some of the calls. Links below are to the routes auth section without payload: 'required'


Fetching Sync Key
The second and third diagram show that a /accounts/keys GET requires a HAWK auth options.payload = true . [1]

It doesn't appear that any POST data is being supplied so the document may need to be updated.

Changing the Password
The diagram shows that the POST to /password/change/finish sets options.payload = true. [2]

/account/reset uses payload: 'required' so this API should as well since authPW is changing too along with wrapKb.


Handling a Forgotten Password
The diagram shows the POST to /password/forgot/resend_code (verify_code) both requiring payload verification. [3][4]

I don't know what email in resend_code is used for, it doesn't appear in the handler function. Worst case seems to be manipulating the redirect to another domain.

Manipulating the verify_code doesn't seem to achieve much since the code is account specific. I think the diagram should be updated.

:warner
Can you comment on whether or not my above analysis is correct on changing the document / code?


[1] - https://github.com/mozilla/fxa-auth-server/blob/master/routes/account.js#L298
[2] - https://github.com/mozilla/fxa-auth-server/blob/master/routes/password.js#L110
[3] - https://github.com/mozilla/fxa-auth-server/blob/master/routes/password.js#L225
[4] - https://github.com/mozilla/fxa-auth-server/blob/master/routes/password.js#L275
Flags: needinfo?(warner-bugzilla)
That sounds right. In general, all HAWK-protected POSTs should set options.payload = true. GETs don't need it, obviously, there's no payload to protect.

We included 'email' in resend_code to plan for a future where you've got more than one recovery email address attached to a given account. We figured that you'd ask for a reset code to be sent to a specific address, rather than everything on file. It may be unnecessary in resend_code, because we can store the specific email address in the DB next to the code, but I think it made something easier in the current implementation.

(incidentally, the original protocol design didn't depend upon HAWK for anything.. it did explicit encrypt+MAC on both requests and responses. When we switched to the simpler "onepw" protocol, where we're relying on TLS anyways, we removed some of that crypto, and rely on HAWK for request integrity instead. The danger, of course, is that it's easy to forget to set that flag)

I'll update the docs to make it clear that the payload needs to be verified on all requests, and will file bugs on fxa-auth-server for any endpoints that aren't doing verification.
Flags: needinfo?(warner-bugzilla)
More study:

* When options.payload = false, an MitM attacker could intercept a valid
  request, and replace the payload with values of their own choosing. But for
  fxa-auth-server, all requests are submitted inside TLS, so attackers cannot
  see valid requests in the first place. As a result, options.payload = true
  is a "nice to have" but not mandatory.

* (When we were doing SRP, "don't depend upon TLS" was an explicit design
  goal, so we had to assume that an attacker *could* see valid requests, so
  options.payload = true was very important. Now that we're willing to rely
  upon TLS, it's somewhat less important.)

* The fxa-auth-server has the following endpoints:

 /v1/account/create               POST
 /v1/account/login                POST (authPW)
 /v1/account/device               HAWK-GET
 /v1/account/keys                 HAWK-GET
 /v1/recovery_email/status        HAWK-GET
 /v1/recovery_email/resend_code   HAWK-POST no-payload-hash
 /v1/recovery_email/verify_code   POST (code)
 /v1/account/reset                HAWK-POST yes-payload-hash
 /v1/account/destroy              POST (authPW)
 /v1/password/change/start        POST (authPW)
 /v1/password/change/finish       HAWK-POST no-payload-hash
 /v1/password/forgot/send_code    POST no-payload-hash
 /v1/password/forgot/resend_code  HAWK no-payload-hash
 /v1/password/forgot/verify_code  HAWK no-payload-hash
 /v1/session/destroy              HAWK no-payload-hash
 /v1/certificate/sign             HAWK yes-payload-hash
 /v1/get_random_bytes             POST
 /v1/verify_email                 GET
 /v1/complete_reset_password      GET

* So I concur that it's only recovery_email/resend_code and
  password/change/finish that could be checking payload hashes but are not

* 1: I believe (but must check) that HAWK authentication has this property:
  if the client hashes their payloads, their messages will be accepted both
  by servers who check payload hashes, and by servers who do not. (But of
  course if the server checks, the client must hash). false->false ok,
  false->true rejected, true->false ok, true->true ok.

* 2: I believe (but must check) these endpoints are only hit by pages served
  by fxa-content-server, not by code baked into Firefox.

* If 1 and 2 are both true, then we should be able to safely turn on
  options.payload on fxa-auth-server, because its only clients are already
  satisfying that requirement. No client changes will be necessary.

* If 1 is true but 2 is not, then we must start by changing all client code
  (in firefox, android, wherever) to options.payload = true, wait for the old
  clients to be retired, then change the server to match.

* If 1 is not true, then we need to coordinate changes on both sides, and
  somehow release them at the same time. Since these checks are nice-to-haves
  rather than mandatory, it may be easier to defer this change to the next
  flag day than to roll it out now.
1 is true: HAWK clients basically compute a value (let's call it X, although
in the code it's named "hash") that is either H(payload), for clients that
participate in payload-hashing, or an empty string. Then they include a
header with "mac=" HMAC(key, H(headers + X)) and "hash=X". The server always
checks the top-level mac= (using whatever value of hash= was supplied), but
only checks that X=H(payload) if the server cares to check payloads. So a
server which does *not* check payloads will accept messages from both kinds
of client.

2 might not be true. I see zero gecko hits for password/change/finish, but
two non-test hits for recovery_email/resend_code:

* http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/fxa/FxAccountClient10.java#750
* http://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsClient.jsm#178

But what we actually care about is that all clients always include H(payload)
in their requests. This is true in the two HAWK client code functions I've
looked at:

* fxa-js-client (which is what fxa-content-server serves up): https://github.com/mozilla/fxa-js-client/blob/master/client/lib/request.js#L89

* gecko FxAccountsClient uses
http://mxr.mozilla.org/mozilla-central/source/services/common/hawkrequest.js#75

But I can't figure out where fennec's implementation is. Nick, could you
check this for me, or tell me where to look?


So I think, if fennec is always including the payload hash, then it'll be
safe to turn on checking on the server. If it isn't, then resending
email-verification codes from fennec would break if we turned it on.
Flags: needinfo?(nalexander)
> So I think, if fennec is always including the payload hash, then it'll be
> safe to turn on checking on the server. If it isn't, then resending
> email-verification codes from fennec would break if we turned it on.

It's a little difficult to see this from MXR [1], but Fennec is in fact only included the payload hash in calls to /certificate/sign.  That might not be right -- we might want to include the payload hash in calls to /create/account, for example.

I would be happy to make Fennec sign payloads everywhere, but we have clients in the wild that don't, hampering our freedom :(

[1] See http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/net/HawkAuthHeaderProvider.java#64, which is called for FxAccount requests by http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/fxa/FxAccountClient10.java#204 and for Sync storage requests by http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java#357.
Flags: needinfo?(nalexander)
It sounds like that HAWK payload hashing is a nice to have with the current protocol since all traffic will be over TLS. An attacker with the capabilities to change the payload (POST body) would already have access to the returned tokens earlier and could have made the requests themselves. Furthermore, forcing options.payload = true on the server would break certain clients in the wild.

I agree that we should delay this until the next flag day if possible.

Sam
Does this affect the FxOS client as well? That might things more tricky since we may have to support FxOS 1.4 clients forever.
Flags: needinfo?(spenrose)
David: for FxOS 1.4, we have preffed-off the calls to services/fxaccounts, and therefore to this use of Hawk. In the case of an uplift, which is not currently on the table, we would have to revisit this issue. Status quo is that FxOS 1.4 will not access fxa-auth-server. FxOS 1.5 will.
Flags: needinfo?(spenrose)
Sam, is FxOS using the same FxAccountsClient.jsm cited above? If so, it should already be sending the payload hashes, and is forward-compatible with a server that checks them.
Flags: needinfo?(spenrose)
Brian: yes we are using services/fxaccounts/FxAccountsClient.jsm in mozilla-central per Comment #3. Apologies for not grokking the upshot that we are therefore in the clear.
Flags: needinfo?(spenrose)
(In reply to Nick Alexander :nalexander from comment #4)

> It's a little difficult to see this from MXR [1], but Fennec is in fact only
> included the payload hash in calls to /certificate/sign.

Rats. So it's the "false" on
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java#367
that's the culprit, right?

> That might not be right -- we might want to include the payload hash in
> calls to /create/account, for example.

Actually /create/account is not HAWK-authenticated (it's a free-for-all, no
auth necessary). But yeah, there are others where it'd be nice to include.

Here are the FxA API calls (POST+HAWK) my quick grep found in Fennec:

 /recovery_email/resend_code
 /session/destroy
 /certificate/sign

Everything else is either a GET (so payload-hash doesn't matter) or is a
non-HAWK POST (authenticated by other means). Of these three, the current
server is only enforcing payload-hash on /certificate/sign.

(the password-change flow uses POST+HAWK endpoints, but so far that's all
done in web-content, so it's fxa-js-client's responsibility)

> I would be happy to make Fennec sign payloads everywhere, but we have
> clients in the wild that don't, hampering our freedom :(

Yeah. Let's turn that on as soon as possible, to minimize the scope of the
problem in the future. In particular, if/when we move password-change or
account-reset out of web-content and down into chrome, we'll be hitting new
APIs, and it'd be nice to get payload-hashes on those from day one.

If we were able to turn on Fennec payload-hashes before FF29 is released,
then e.g. 6 months from now, we could turn on server checking, and the only
failure it should induce would be /recovery_email/resend_code for the handful
of folks still running 6-month-old pre-release code (FF29 nightly/aurora/beta
on Fennec). That's probably tolerable :).
(In reply to David Chan [:dchan] from comment #5)

> It sounds like that HAWK payload hashing is a nice to have with the current
> protocol since all traffic will be over TLS. An attacker with the
> capabilities to change the payload (POST body) would already have access to
> the returned tokens earlier and could have made the requests themselves.
> Furthermore, forcing options.payload = true on the server would break
> certain clients in the wild.
> 
> I agree that we should delay this until the next flag day if possible.

Agreed. I'll update the docs to say that clients should always use
options.payload = true, but with a note that says the server is not yet
enforcing this while we wait for all clients to be updated.
Depends on: 986664
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Attachment #8403471 - Flags: review?(rnewman)
Tested on device; creating an account and syncing appeared fine.
Comment on attachment 8403471 [details] [review]
Link to Github pull-request: https://github.com/mozilla-services/android-sync/pull/448

Nits on PR.
Attachment #8403471 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/integration/fx-team/rev/c0e6f3b758b4
OS: Linux → Android
Hardware: x86_64 → All
Aw, crap.  Bad bug number.  I really don't care to reland right now :(
Not sure why this never got resolved, but it landed long ago.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.