Closed Bug 914353 Opened 6 years ago Closed 6 years ago

Electrolysis: Make new and legacy crypto implementation switchable at runtime

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached patch crypto-switch (obsolete) — Splinter Review
Most of the legacy crypto API doesn't work in content processes. But the new function getRandomValues was fixed to work for FirefoxOS (Bug 673432). So we should at least expose those, and shim the rest of the functions. I hope we don't need to forward them all later, but I am not sure what our deprecation plan is.
Attachment #801790 - Flags: review?(khuey)
Blocks: core-e10s
Attached patch catch nullptr (obsolete) — Splinter Review
For some reason ContentParent::RecvGetRandomValues wasn't checking if Crypto::GetRandomValues returned nullptr, that seems like a mistake to me.
Attachment #801822 - Flags: review?(khuey)
Comment on attachment 801822 [details] [diff] [review]
catch nullptr

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

Killing the child seems too harsh here.  I think you should just send back a zero length array and throw an exception on the child side.

::: dom/ipc/ContentParent.cpp
@@ +2501,5 @@
>                                     InfallibleTArray<uint8_t>* randomValues)
>  {
>      uint8_t* buf = Crypto::GetRandomValues(length);
> +    if (!buf)
> +        return false;

brace your ifs
Attachment #801822 - Flags: review?(khuey) → review-
Comment on attachment 801790 [details] [diff] [review]
crypto-switch

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

I'm not sure I understand what you're trying to do here.  Why would you de-virtualize all of the methods on mozilla::dom::Crypto but not do the same for the methods on nsCrypto?
Attachment #801790 - Flags: review?(khuey) → review-
Attached patch crypto-switch v2 (obsolete) — Splinter Review
I asked Brian Smith to comment in this bug on whether we can go ahead and ignore the other functions.
Attachment #801790 - Attachment is obsolete: true
Attachment #802623 - Flags: review?(khuey)
Attachment #802623 - Attachment is obsolete: true
Attachment #802623 - Flags: review?(khuey)
Attached patch crypto-switch v2Splinter Review
Attachment #802629 - Flags: review?(khuey)
Attached patch catch nullptr v2Splinter Review
Attachment #801822 - Attachment is obsolete: true
Attachment #802630 - Flags: review?(khuey)
19:13 <briansmith> evilpie: proposal to remove them from desktop firefox completely
19:14 <briansmith> so that we do not need to implement them in e10s
https://hg.mozilla.org/mozilla-central/rev/668d0e08fdf3
https://hg.mozilla.org/mozilla-central/rev/4c0b7b4edad3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Blocks: 918119
You need to log in before you can comment on or make changes to this bug.