The patch in bug 927711 added a "jsval" member to MmsDeliveryInfo but didn't add any code to trace the resulting values. Since these structs are stored on the heap, that means that they value they're holding can just become garbage any time a GC happens. I strongly recommend, in this order of checkins: 1) Using a numeric timestamp here, not a Date object. 2) Not using xpidl dictionaries. 3) Not using JSAPI directly, because any attempt to do so will generally go wrong.

(In reply to Boris Zbarsky [:bz] from comment #0) > The patch in bug 927711 added a "jsval" member to MmsDeliveryInfo but didn't > add any code to trace the resulting values. Since these structs are stored > on the heap, that means that they value they're holding can just become > garbage any time a GC happens. Hi Boris, thank you very much for bringing this to our attention. > > I strongly recommend, in this order of checkins: > > 1) Using a numeric timestamp here, not a Date object. Using numeric timestamp is not our expected API design. W3C [1] and we are in agreement to use Data object to represent the timestamps in general. [1] http://messaging.sysapps.org/#idl-def-MmsDeliveryInfo > 2) Not using xpidl dictionaries. Are you talking about the whole MmsDeliveryInfo structure [2]? So, converting it to webidl can solve this problem? [2] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl#18 > 3) Not using JSAPI directly, because any attempt to do so will generally go > wrong. It sounds like you're worrying about using JSAPI in this way would cause the Data object GC'ed any time (due to the wrong use of AutoJSContext?). Could you please point out which part goes wrong exactly and how we can correctly create Date objects for a specific context that wants to hold the object?

> W3C [1] and we are in agreement to use Data object to represent the timestamps in general. TC39 strongly recommends not doing that, and Date is going to be removed from WebIDL, so the spec you link to is just wrong. Further, I will personally formally object to any spec that uses a Date in a dictionary like this. There is absolutely no reason to do that: if the dictionary said "long long" the caller could still pass in a Date object and it would work fine.... All using "Date" in the dictionary does is overly restrict the caller's options (e.g. makes it impossible to pass in Date.now()). Also, the array stuff in that spec draft is rather underdefined. I suggest talking to Jonas about it if he hasn't seen this draft yet. > Are you talking about the whole MmsDeliveryInfo structure [2]? Yes. > So, converting it to webidl can solve this problem? No. It would merely make it simpler to trace it. > you're worrying about using JSAPI in this way would cause the Data object GC'ed any > time No, I'm worrying about the fact that people shouldn't ever be writing JSAPI code unless they plan to get it reviewed by at least two people very familiar with JSAPI... ;) > due to the wrong use of AutoJSContext?) No. The bug is the fact that you have gcthings (in this case Date objects) on the heap but you don't actually trace them. > Could you please point out which part goes wrong exactly The part that goes wrong is that once a GC happens mDeliverInfo[0].deliveryTimestamp will become a garbage pointer. Same for the other array entries. > how we can correctly create Date objects for a specific context that wants to hold the > object? Again, you shouldn't be creating Date objects. Just don't do it. Apart from that, the creating part is fine. Except for the fact that it's totally broken with generational GC because your objects aren't stack-rooted, of course. It's the later lifetime management that's not fine: you need to trace those Date objects.

Oh, and as far as how to trace things... First, you need to make MmsMessage cycle-collected. You need to do that anyway, otherwise you could get memory leaks due to cycles through the objects it holds, if you managed to trace them. Once it's cycle-collected, you can use its trace callback to trace things. See what ImageData does, for example, to trace its mData member. But again, there should be no need to store Date objects here; the spec should change to not do that.

Looks like this is trunk only.

Hi, Eduardo, please take a look at comment #0 and comment #2. It seems we're not recommended to use Data objects to specify the timestamps.

Comment on attachment 8336535 [details] [diff] [review] Part 1, DOM IDL Yes, this makes sense to me.

Comment on attachment 8336535 [details] [diff] [review] Part 1, DOM IDL Review of attachment 8336535 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Actually, TC39 (the people developing the JS language) has recommended against returning Date objects in new APIs. So returning long-long's seems like the way to go. It's both easier to implement and makes for a better API.

So DOMTimeStamp is unsigned long long. But we want uint64_t usually. dict gen didn't have support for it.

Mhm I guess, I should see what kind of tests/code breaks with this change ;)

Hi Tom, it seems you've already finished most of the Gecko implementation (which is exactly the same as what I'm planing to do). Do you want to directly take this bug? Btw, as you mentioned at comment #11, we need to fix some test cases. For example, test_smsservice_createsmsmessage.js which is the major one. Also, we need to have some corresponding Gaia fix to avoid breaking the compatibilities.

Comment on attachment 8337248 [details] [diff] [review] timestamp-idl Review of attachment 8337248 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks Tom!

Hi, new bug created in W3C SysApps to consider changing Date objects into long long: https://github.com/sysapps/messaging/issues/90

Comment on attachment 8337248 [details] [diff] [review] timestamp-idl I guess this is fine, though we could have just used "unsigned long long" in the dictionary too and avoided the codegen changes. Please file a followup bug to get rid of the remaining callers of convertTimeToInt: none of them should be needed, imo. r=me

Comment on attachment 8337269 [details] [diff] [review] Remove support for JS::Value from dictionary_helper_gen.py r=me

Bug 921918 introduced another |readTimestamp| which also uses Date object and was just landed. Please fix that as well. Hi Tom, letting you take over this bug since you've already come up some patches. Thanks!

Want to fix the test failures? :) https://tbpl.mozilla.org/?tree=Try&rev=fed5f0cdb444

Thanks for pointing me to this bug, Gene. Can you please make sure that we change the spec to not use Date here?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #19) > Thanks for pointing me to this bug, Gene. Can you please make sure that we > change the spec to not use Date here? Are you talking about the W3C spec? Yes, we already did that at comment #14.

sec-critical followup to a MMS implementation, so blocking+

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #20) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #19) > > Thanks for pointing me to this bug, Gene. Can you please make sure that we > > change the spec to not use Date here? > > Are you talking about the W3C spec? Yes, we already did that at comment #14. Great, thanks!

Fixes the new deliveryTimestamp.

There is other test and gonk code that needs fixing. I however don't have b2g to do it right now, so I would appreciate if you fixed the other tests.

> There is other test and gonk code that needs fixing. I however don't have > b2g to do it right now, so I would appreciate if you fixed the other tests. I can take over the testing parts but I'm afraid I wouldn't have time working on it until next Monday or Tuesday. Will catch up.

Hi Tom, I fixed |nsIDOMMozMobileMessageThread.timestamp| as well. Could you please take the review for the extra change? Thanks! Carry on sr=sicking r=bz,gene.

Waiting for try before asking for Vicamo's review: https://tbpl.mozilla.org/?tree=Try&rev=197841a868a5

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #12) > Btw, as you mentioned at comment #11, we need to fix some test cases. For > example, test_smsservice_createsmsmessage.js which is the major one. Also, > we need to have some corresponding Gaia fix to avoid breaking the > compatibilities. Open Bug 944881 to fix the Gaia part. Note that we should land the Gaia part first before landing the Gecko part.

Comment on attachment 8340628 [details] [diff] [review] Part 3, fix test cases (WIP) (r=vicamo) Review of attachment 8340628 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/tests/marionette/test_incoming_delete.js @@ +35,5 @@ > is(incomingSms.read, false, "read"); > is(incomingSms.receiver, RECEIVER, "receiver"); > is(incomingSms.sender, SENDER, "sender"); > is(incomingSms.messageClass, "normal", "messageClass"); > + ok(incomingSms.deliveryTimestamp == 0, "deliveryTimestamp is 0"); all of these should be probably be: is(incomingSms.deliveryTimestamp, 0, "deliveryTimestamp is 0");

(In reply to Boris Zbarsky [:bz] from comment #15) > Please file a followup bug to get rid of the remaining callers of > convertTimeToInt: none of them should be needed, imo. Thanks for addressing this. Fire bug 944890.

(In reply to Tom Schuster [:evilpie] from comment #30) > all of these should be probably be: is(incomingSms.deliveryTimestamp, 0, > "deliveryTimestamp is 0"); Sounds good. Will polish in the next version. Thanks!

The previous try looks good. Polish some tests by comment #30. New try: https://tbpl.mozilla.org/?tree=Try&rev=f3774171d8ad

Hi Tom, btw, I noticed the author becomes me after rebasing and generating the new patches. I didn't mean to do that. You should own the credits. I think you can regenerate the patches (part 1 & part 2) before landing. :)

Patches are ready to go. Waiting for Gaia's support at bug 944881. We hope to land both Gecko and Gaia as possible as we can at the same time to avoid breaking the compatibility.

> I noticed the author becomes me after rebasing and generating the new patches. You must be using Git or something.... hg doesn't screw up patch authorship like that, last I checked.

> We hope to land both Gecko and Gaia as possible as we can at the same time For what it's worth, it should be simple to just land the Gaia patches first (because using a Date where a number is expected should always work) and then land the Gecko patches, right?

(In reply to Boris Zbarsky [:bz] from comment #36) > > I noticed the author becomes me after rebasing and generating the new patches. > > You must be using Git or something.... hg doesn't screw up patch authorship > like that, last I checked. Neither do git ;) (In reply to Boris Zbarsky [:bz] from comment #37) > > We hope to land both Gecko and Gaia as possible as we can at the same time > > For what it's worth, it should be simple to just land the Gaia patches first > (because using a Date where a number is expected should always work) and > then land the Gecko patches, right? We can use "+value" constructs everywhere (instead of using getTime() in some places). We do have Date values in the Contacts API as well: https://mxr.mozilla.org/mozilla-central/source/dom/webidl/Contacts.webidl. Is that wrong too?

> We can use "+value" constructs everywhere Ah, yes, I guess the toString behavior is a bit different.... > Is that wrong too? Imo, yes.

Comment on attachment 8340627 [details] [diff] [review] Part 2, DOM API and implementation (sr=sicking r=bz,gene,evilpie) Review of attachment 8340627 [details] [diff] [review]: ----------------------------------------------------------------- Can't really review my own patch, but looks good.

(In reply to Boris Zbarsky [:bz] from comment #37) > > We hope to land both Gecko and Gaia as possible as we can at the same time > > For what it's worth, it should be simple to just land the Gaia patches first > (because using a Date where a number is expected should always work) and > then land the Gecko patches, right? Sounds good! For example, the new Gaia codes could be: var value = new Date(idl.timestamp); then we should be able to keep the backward compatibility no matter |idl.timestamp| is a number or a Date object.

Patches are reviewed. When will this be checked in?

We need to wait for the Gaia patch to land. I r+ed it today, so should not be long now.

Gaia patch was just landed on master few minutes ago (bug 944881). Tom, please go ahead to land. Thank you. :) We probably need to wait for Gaia patch to land V1.3 and then we can land Gecko patch to V1.3 as well, but I think we can start with the inbound/master.

Should I land these with or without commit messages?

Gene, now that we've missed 28, do we need to backport all the relevant patches? In particular, is there any way at all that this code is reachable on non-b2g?

We need to backport for firefox OS 1.3 anyway.

(In reply to Tom Schuster [:evilpie] from comment #45) > Should I land these with or without commit messages? Land with commit but try not to say "horrible security issue, please pwn us" in them. :-)

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/403f46856758 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc9232b753d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1660fc1537ef

https://hg.mozilla.org/mozilla-central/rev/403f46856758 https://hg.mozilla.org/mozilla-central/rev/6cc9232b753d https://hg.mozilla.org/mozilla-central/rev/1660fc1537ef

(In reply to Boris Zbarsky [:bz] from comment #46) > Gene, now that we've missed 28, do we need to backport all the relevant > patches? This GC hazard only happens when we attempt to return Date object through the dictionary. Right? That's what we made at Bug 927711 which simply polluted the V1.3. Other timestamps still use [implicit_jscontext] to expose the Data objects, which should still be safe. We fixed them as well just because of consistency. So, fixing V1.3 (28) and 29 should be enough. Please correct me if I'm wrong. Thanks! > In particular, is there any way at all that this code is reachable > on non-b2g? No, Messaging API is only available on the b2g platform and is only exposed to those apps which have the "sms" permission. As far as I know, Messaging App is the only certified app having such a permission. So, don't worry about that too much, IMO. Btw, we ran into this issue without breaking any test cases, so I also hope to learn how can a real hacker hack our system through this kind of GC deficiency in order to prevent us from producing security holes again in the future. :) (well, maybe we shouldn't discuss about such topics on the bugzilla?)

> This GC hazard only happens when we attempt to return Date object through the dictionary. > Right? It happens whenever an MmsMessage is created from a mobilemessage::MmsMessageData whose deliveryTimestamp() or readTimestamp() is not 0. At that point we're in a state where we can end up with a garbage pointer. Whether the pointer is _used_ after that point depends on whether anyone ever calls GetData() or GetDeliveryInfo() on that MmsMessage, as far as I can tell. > So, fixing V1.3 (28) and 29 should be enough. Correct. > we ran into this issue without breaking any test cases Presumably because the tests happened to not gc between creating the MmsDeliveryInfo and using it? An attacker would just trigger a gc at the point they wanted (e.g. by doing lots of allocations).

https://hg.mozilla.org/releases/mozilla-aurora/rev/2bc49f51742a https://hg.mozilla.org/releases/mozilla-aurora/rev/baa815736efe https://hg.mozilla.org/releases/mozilla-aurora/rev/7d2e0f2f729d

Marking [qa-] for verification purposes. If a test case materializes, we can change that.