Closed Bug 618341 Opened 9 years ago Closed 9 years ago

Fail hard on actual crypto errors, e.g. cipher finalize failed errors

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+
fennec 2.0b3+ ---

People

(Reporter: philikon, Assigned: rnewman)

References

Details

Attachments

(3 files)

Bug 617709 turned cipher finalize failed errors (see bug 618068) when decrypting the Sync Key into mere warnings. It should fail hard there and only accept HMAC failures as acceptable problems. Otherwise users will get into an inconsistent state without knowing or having logs.
blocking2.0: --- → ?
tracking-fennec: --- → ?
Attached file sync-log 1
Attached file sync log 2
Attachment #496862 - Flags: review?(philipp)
Encryption is also silent, and has been for a long time.

      for each (let id in this._modifiedIDs) {
        try {
          let out = this._createRecord(id);
          if (this._log.level <= Log4Moz.Level.Trace)
            this._log.trace("Outgoing: " + out);

          out.encrypt();
          up.pushData(out);
        }
        catch(ex) {
          this._log.warn("Error creating record: " + Utils.exceptionStr(ex));
        }
      ...

I think we need a coherent plan for precisely which errors should be swallowed, and which should be propagated.

Swallow HMAC errors on decryption, and propagate everything else?

Swallow network errors at all times?

Something else?
(In reply to comment #4)
> I think we need a coherent plan for precisely which errors should be swallowed,
> and which should be propagated.
> 
> Swallow HMAC errors on decryption, and propagate everything else?

Yes.

> Swallow network errors at all times?

No, The engines throw these and the Service catches, evaluates (_checkServerError) and acts appropriately (notifies UI via observers).
blocking2.0: ? → beta8+
tracking-fennec: ? → 2.0b3+
Comment on attachment 496862 [details] [diff] [review]
Rethrow exceptions we aren't handling. v1

>       catch(ex) {
>+        
>+        if (!Utils.isHMACMismatch(ex))
>+          // Rethrow anything we shouldn't handle.
>+          throw ex;
>+        

Curly braces for multiline blocks, please.
Attachment #496862 - Flags: review?(philipp) → review+
(In reply to comment #5)

> > Swallow network errors at all times?
> 
> No, The engines throw these and the Service catches, evaluates
> (_checkServerError) and acts appropriately (notifies UI via observers).

... except that, as I posted in Comment 4, *any* failure in the engine during encryption or upload is silent swallowed. The Service never got to see them.

I tested this by making _commonCrypt raise a cipher finalize error, and sync happily completed. That catch block is the culprit.
Blocks: 618492
Marking as closed. Will file another bug to revisit error swallowing throughout.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.