(In reply to Kris Maglione [:kmag] from comment #25) > Anyway. This is actually quite scary... > > So, I think the problem is [here](https://searchfox.org/mozilla-central/rev/3f1856fc5adc6b1662e5388c67e33fa322c5b323/js/xpconnect/src/XPCConvert.cpp#107) where we convert the return value. It essentially does [`JS::Value::setDouble`](https://searchfox.org/mozilla-central/rev/3f1856fc5adc6b1662e5388c67e33fa322c5b323/js/public/Value.h#478) which just [does a bitwise cast](https://searchfox.org/mozilla-central/rev/3f1856fc5adc6b1662e5388c67e33fa322c5b323/js/public/Value.h#414). Since JS values use NaN boxing to determine the type of the object, that has the effect making us interpret the value as the wrong type if we have an odd NaN value. > > That shouldn't be exploitable by numbers that come from JavaScript, since it generally shouldn't be able to generate those weird NaN values. But for values that we get from the network, or `ArrayBuffer`s, that could open an exploit. > > I'm not sure whether we should fix this in the XPConnect type conversion code or add stronger checks to the JS engine, though. We should maybe do both. Well, we can probably have more than one strategy to avoid situations like this. We might want to avoid such values to get into our C++ variables from the beginning, such that we do not need to mitigate (and can have maybe just an assert) on read ? As you mention "values that we get from the network" I assume you intend (also) IPC, and it would be indeed nice if already [`HangMonitorParent::RecvHangEvidence`](https://searchfox.org/mozilla-central/rev/15b656909e77d3048d4652b894f79a8c719b4b86/dom/ipc/ProcessHangMonitor.cpp#823) (or its direct caller on IPC level) would return an IPC_FAIL here, potentially just killing the bogus child. The question is, if having a `NaN` check for `double` (`float`, ...) values already in the IPC layer would be correct - I just hope that we do not abuse `NaN` to signal a missing parameter or such anywhere...
Bug 1776658 Comment 27 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(In reply to Kris Maglione [:kmag] from comment #25) > Anyway. This is actually quite scary... > > So, I think the problem is [here](https://searchfox.org/mozilla-central/rev/3f1856fc5adc6b1662e5388c67e33fa322c5b323/js/xpconnect/src/XPCConvert.cpp#107) where we convert the return value. It essentially does [`JS::Value::setDouble`](https://searchfox.org/mozilla-central/rev/3f1856fc5adc6b1662e5388c67e33fa322c5b323/js/public/Value.h#478) which just [does a bitwise cast](https://searchfox.org/mozilla-central/rev/3f1856fc5adc6b1662e5388c67e33fa322c5b323/js/public/Value.h#414). Since JS values use NaN boxing to determine the type of the object, that has the effect making us interpret the value as the wrong type if we have an odd NaN value. > > That shouldn't be exploitable by numbers that come from JavaScript, since it generally shouldn't be able to generate those weird NaN values. But for values that we get from the network, or `ArrayBuffer`s, that could open an exploit. > > I'm not sure whether we should fix this in the XPConnect type conversion code or add stronger checks to the JS engine, though. We should maybe do both. Well, we can probably have more than one strategy to avoid situations like this. We might want to avoid such values to get into our C++ variables from the beginning, such that we do not need to mitigate (and can have maybe just an assert) on read ? As you mention "values that we get from the network" I assume you intend (also) IPC, and it would be indeed nice if already [`HangMonitorParent::RecvHangEvidence`](https://searchfox.org/mozilla-central/rev/15b656909e77d3048d4652b894f79a8c719b4b86/dom/ipc/ProcessHangMonitor.cpp#823) (or its direct caller on IPC level) would return an IPC_FAIL here, potentially just killing the bogus child. The question is, if having a `NaN` check for `double` (`float`, ...) values already in the IPC layer would be correct - I just hope that we do not abuse `NaN` to signal a missing parameter or such anywhere... But having a stricter check on incoming IPC parameters in response to IPC fuzzing seems actually quite straight-forward?