609202 - Consider possible race conditions (and approaches thereto) during upgrade

Status: RESOLVED DUPLICATE of bug 568569

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

Had a good discussion about this tonight with Philipp.

On connection, a client goes through a whole dance: fetching, checking, and replacing the meta record to note a version upgrade; transforming its own sync key using PBKDF2; downloading and/or generating new keys; and uploading them.

In the simplified crypto proposal, the passphrase is upgraded by using the new syncID as input to PBKDF2. All clients need to do this with the same syncID in order to have a seamless upgrade. Because the GET of meta and the PUT of the new contents are separate, a possible race condition exists.

We need to ensure that no nasty hiccups could occur if the syncID and keys get confused during a simultaneous 'first sync': for example, if 

  Passphrase = "hello"
  A: GET meta -- decides to upgrade, new syncID = aaa
  A: PUT meta
  B: GET meta -- decides to upgrade, new syncID = bbb
  B: PUT meta
  A: replace sync key = F("hello", aaa)
  B: replace sync key = F("hello", bbb)

B's meta is in place, but A has the wrong sync key. At the end of a sync, meta is again PUT...

Various schemes might address this, but they could run into problems detecting the difference between a 'change passphrase' event and a race screwup. (E.g., "store the old passphrase so you can regen!".)

At the very least, we should consider whether there is a chance of a problem here, and whether we should do anything about it. (The window between GET and PUT of meta is small.)

There are possible solutions we could put in place, which Philipp and I characterize thusly:

---
20:25:14 <@philiKON> if we make it fetch a new meta just before uploading the new meta, we might detect hte race
...
20:28:18 <@rnewman> re fetch before uploading: that only narrows the race window
20:28:27 <@rnewman> to eliminate it you need a server-side atomic construct
20:28:52 <@philiKON> yes
20:29:07 <@philiKON> i'm ok with narrowing it for now
20:29:25 <@philiKON> we could do another server rev at some point
20:29:39 <@rnewman> I'm pretty sure that the only way to completely avoid untidy screwups, with possible difficult recovery, is to either lock on the server, or have a pure functional way of computing the next sync ID from both clients
20:29:49 <@philiKON> indeed
20:29:51 <@rnewman> double-checking meta is good enough for me for now
20:30:16 <@rnewman> but it would be good to consider if we can add some locking client behavior before the server actually supports it
20:30:22 <@rnewman> e.g., those PUT/POST/GET params
---

In short: make sure that aaa == bbb, or prevent B from computing its own new syncID.

It might also be worth exploring the idea of keeping the old passphrase around until a successful sync...

Comment 1

[Philipp von Weitershausen [:philikon]]

Old bugs FTW :)