879537 - Contacts: Be more strict when setting date properties

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

Attachment: Only accept strings and Date instances for Date properties

Found by the fuzzer, if you pass an object as one of the Date properties you get 'JavaScript Error: "can't convert aBday to primitive type"'.

Comment 1

[Gregor Wagner [:gwagner]]

Comment on attachment 758262 [details] [diff] [review]
Only accept strings and Date instances for Date properties

Review of attachment 758262 [details] [diff] [review]:
-----------------------------------------------------------------

+testcase please.
thanks!

Comment 2

[Reuben Morais [:reuben]]

Attachment: Slightly better version

I can't actually reduce the testcase… I know that specific value that's causing it, but I'm having a hard time replicating it in a reduced testcase. Apparently it even depends on the values in the blob's view.

Comment 3

[Reuben Morais [:reuben]]

(In reply to Reuben Morais [:reuben] from comment #2)
> I can't actually reduce the testcase… I know that specific value that's
> causing it, but I'm having a hard time replicating it in a reduced testcase.
> Apparently it even depends on the values in the blob's view.

Ugh, I blame the deep fried sandwich, it's affecting my brain. Let's try that again:

I know the value that causes the error, it happens whenever I pass an object with a blob property as one of the Date attributes (bday, anniversary). The assertion comes from the Date constructor, since it tries to do ToPrimitive on the passed value, and fails. This patch worksaround that by only calling new Date() when the value is a string, or a number, and skips that entirely if it's already a Date instance. Can I land this without a test? :)

Comment 4

[Gregor Wagner [:gwagner]]

Comment on attachment 759992 [details] [diff] [review]
Slightly better version

Review of attachment 759992 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/contacts/ContactManager.js
@@ -423,5 @@
> -      // if (aBday instanceof Date) {
> -      //   this._bday = aBday;
> -      // } else if (typeof aBday === "string" || typeof aBday === "number") {
> -      //   this._bday = new Date(aBday);
> -      // }

Hm can you upload that again? I would like to see the original version :) I hope that's not the code that is checked in.

Comment 5

[Reuben Morais [:reuben]]

So now that WebIDL contacts is about to land, it'll only make sense to land this in b2g18… How should I proceed?

Comment 6

[Gregor Wagner [:gwagner]]

(In reply to Reuben Morais [:reuben] from comment #5)
> So now that WebIDL contacts is about to land, it'll only make sense to land
> this in b2g18… How should I proceed?

Still land it on birch first so we have the same code if the webidl patch gets backed out.

Comment 7

[Reuben Morais [:reuben]]

Attachment: Patch

Comment 8

[Reuben Morais [:reuben]]

Also, I think the problem here is that XPConnect treats Dates as JSObjects under the hood, and doesn't filter out garbage that is not a date :(

Comment 9

[Reuben Morais [:reuben]]

Thanks!

https://hg.mozilla.org/projects/birch/rev/5d42d5a6a922

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d42d5a6a922

Comment 11

[Gregor Wagner [:gwagner]]

We don't want exceptions in our contactManager code that could completely break the contacts, sms, dialer.... app.

Comment 12

[bhavana bajaj [:bajaj]]

:gwagner , Is this testable ?How sure can we be that they will be no fallouts and how risky is the patch?

Comment 13

[bhavana bajaj [:bajaj]]

needsinfo on :gwagner here.

Comment 14

[Wayne Chang [:wchang]]

ni? reporter: can you describe the user impact here so triage can decide on if this should block?

Comment 15

[Reuben Morais [:reuben]]

(In reply to bhavana bajaj [:bajaj] from comment #12)
> :gwagner , Is this testable ?How sure can we be that they will be no
> fallouts and how risky is the patch?

Pretty sure. This patch only reduces the types of input we accept, making the chance of fallouts minimal.

(In reply to Wayne Chang [:wchang] from comment #14)
> ni? reporter: can you describe the user impact here so triage can decide on
> if this should block?

If a malicious or broken app uses the Contacts API incorrectly, we'll die inside chrome code, with no useful information to the user/developer of what went wrong. The app will very likely stop running correctly.

Comment 16

[bhavana bajaj [:bajaj]]

This could be taken as an improvement on v1.2 but given comment 15 not a blocker as of today we are on par with 1.0.1 and this is an enhancement to the existing state but would not block.