Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MPR7765: integer overflows when unmarshaling a bigarray #1718

Merged
merged 3 commits into from Apr 12, 2018

Conversation

Projects
None yet
3 participants
@xavierleroy
Copy link
Contributor

xavierleroy commented Apr 11, 2018

Malicious or corrupted marshaled data can result in a bigarray
with impossibly large dimensions that cause overflow when computing
the in-memory size of the bigarray. Disaster ensues when the data
is read in a too small memory area. This PR checks for overflows
when computing the in-memory size of the bigarray.

Closes: https://caml.inria.fr/mantis/view.php?id=7765

MPR7765: integer overflows when unmarshaling a bigarray
Malicious or corrupted marshaled data can result in a bigarray
with impossibly large dimensions that cause overflow when computing
the in-memory size of the bigarray.  Disaster ensues when the data
is read in a too small memory area.  This commit checks for overflows
when computing the in-memory size of the bigarray.

@xavierleroy xavierleroy added this to the 4.07 milestone Apr 11, 2018

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Apr 11, 2018

If accepted, this bug fix should be backported to 4.07.0.

for (i = 0; i < b->num_dims; i++) {
if (caml_umul_overflow(num_elts, b->dim[i], &num_elts))
caml_deserialize_error("input_value: size overflow for bigarray");
}

This comment has been minimized.

@gasche

gasche Apr 11, 2018

Member

Here you inlined the caml_ba_num_elts function and check internal multiplications for overflow. I find it a bit strange:

  • if overflows may occur during caml_ba_num_elts computation, then I would rather make the function safe in some way (or at least returning -1 instead of a nonsensical result)
  • if, as I would naively expect, we know that the number of elements can never be large enough to provoke an overflow, then we can just can caml_ba_num_elts here, and just keep the check for elt_size * num_elts below.

This comment has been minimized.

@xavierleroy

xavierleroy Apr 11, 2018

Author Contributor

In my mind, caml_ba_num_elts is to be used for "trusted" bigarrays, those that already passed the non-overflow checks on creation. There is some run-time cost in checking for overflow in caml_ba_num_elts.

if, as I would naively expect, we know that the number of elements can never be large enough to provoke an overflow,

We have no such guarantee when we unmarshal. The number and values of dimensions may be completely arbitrary. All multiplications must be guarded against overflow.

@gasche
Copy link
Member

gasche left a comment

And what about the further multiplications num_elts * 2 in the switch, shouldn't they also be checked?

if (caml_umul_overflow(num_elts, b->dim[i], &num_elts))
caml_deserialize_error("input_value: size overflow for bigarray");
}
/* Determine element size in bytes. Watch out for overflows (MPR#7765). */

This comment has been minimized.

@gasche

gasche Apr 11, 2018

Member

Rather than "element size", you are computing the "array size in bytes", aren't you?

@xavierleroy xavierleroy force-pushed the xavierleroy:MPR7765 branch from d103ea6 to c509c17 Apr 11, 2018

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Apr 11, 2018

I see no problem with num_elts * 2 in the complex cases: here, size is 8 or 16 times num_elts, and size did not overflow, so num_elts * 2 should not overflow.

@gasche

gasche approved these changes Apr 11, 2018

Copy link
Member

gasche left a comment

I am now convinced that the patch is correct and sufficient.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 11, 2018

@xavierleroy approved; can I let you merge and cherry-pick, or would you like me to take care of it (when the CI finishes)?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Apr 11, 2018

I believe there are other sources of security issues when unmarshaling malicious blocks, cf https://caml.inria.fr/mantis/view.php?id=7765#c19000

I'm in favor of documenting the unmarshaler as being unsafe in such a scenario (and I'm also in favor of merging this PR, of course).

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Apr 12, 2018

The CI failure was due to missing Changes entry. Added a Changes entry. Will merge soon.

@xavierleroy xavierleroy merged commit 85162ee into ocaml:trunk Apr 12, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

xavierleroy added a commit that referenced this pull request Apr 12, 2018

MPR7765: integer overflows when unmarshaling a bigarray (#1718)
Malicious or corrupted marshaled data can result in a bigarray
with impossibly large dimensions that cause overflow when computing
the in-memory size of the bigarray.  Disaster ensues when the data
is read in a too small memory area.  This commit checks for overflows
when computing the in-memory size of the bigarray.
@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Apr 12, 2018

Cherry-picked to 4.07 branch.

@xavierleroy xavierleroy deleted the xavierleroy:MPR7765 branch Apr 12, 2018

raspbian-autopush pushed a commit to raspbian-packages/ocaml that referenced this pull request Feb 3, 2019

Integer overflows when unmarshaling a bigarray
Malicious or corrupted marshaled data can result in a bigarray
with impossibly large dimensions that cause overflow when computing
the in-memory size of the bigarray.  Disaster ensues when the data
is read in a too small memory area.  This commit checks for overflows
when computing the in-memory size of the bigarray.

This patch has been modified from upstream one to use caml_ba_multov
instead of caml_umul_overflow which is unavailable in OCaml 4.05.0.

Origin: ocaml/ocaml#1718
Bug: https://caml.inria.fr/mantis/view.php?id=7765
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=895472
Bug-CVE: CVE-2018-9838

Gbp-Pq: Name 0012-Integer-overflows-when-unmarshaling-a-bigarray.patch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.