Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0007765OCamlstandard librarypublic2018-04-06 17:412018-04-13 14:39
Reportermaxdos 
Assigned To 
PriorityurgentSeveritymajorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version4.06.0 
Target Version4.07.0+dev/beta2Fixed in Version4.07.0+dev/beta2 
Summary0007765: A integer overflow in the big array serialization module leads to arbitrary code execution
Description The bigarray module in all recent ocaml versions is capable of reading in serialized (marshalled) objects from a external source which is often used for network operations and interprocess communication.

https://github.com/ocaml/ocaml/blob/ea60609504cc53c06a2711bba51b9a37db557a28/byterun/bigarray.c#L458 [^]
A integer overflow vulnerability allows under certain circumstances an remote attacker to input a corrupt object and edit arbitrary addresses in the processes memory which leads over common techniques (in this take libc one gadget but any rop gadget or got table vector would work) to arbitrary memory access (!) and in the course also arbitrary code execution

Please check the writeup.pdf for more details !!

In variations of this the exploit is usable in all ocaml versions we tested no matter if compiled to binary or intepreted (!)
Steps To ReproduceCheck out the attached archive and run the makefile (or use the precompile executable)
the exploit can be run by executing exploit.py (python2.7 and pwntools are required)
The last step of code execution is using a one-gadget attack technique and is specific to the glibc version in use (e.g. tested is debian glibc 2.24-11+deb9u1) but can with minimal amount of work be ported to any libc version on any patch level to our knowledge

Of course you can also use the exploit.py to write your own version of exploit
Additional Information This Vulnerability in ocaml was discovered by me (maximilian.tschirschnitz@gmx.de) and the exploit collaboratly developed with (philipp.hagenlocher@tum.de)
For illustration purposes it was packaged as security challenge to demonstrate the impact of the issue (see writeup.pdf)
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0018983)
frisch (developer)
2018-04-06 17:56

I think it is a well-known fact that a program should not use the generic demarshaller on untrusted data. This should perhaps be made a bit more explicit, but the document of Marshal already says:

   Anything can happen at run-time if the object in the file does not
   belong to the given type.


> which is often used for network operations and interprocess communication.

Are you aware of actual uses of Marshal for such operations between agents that don't trust each other?
(0018984)
maxdos (reporter)
2018-04-06 18:00
edited on: 2018-04-06 18:02

Well sending corrupt data over the network should not lead to a memory corruption of my bigarray buffer ?
Of course anything can happen in a sense of wrong behaviour (segfault, exception)
But in a broader sense following this argumentation would mean ocaml is not safe for any usage over network in general
Which is not the case right ?

Yes the one i created in the example, and of course this error could allow any ocaml implementation based on this library cause to be exploitable through it

(0018985)
maxdos (reporter)
2018-04-06 18:03
edited on: 2018-04-06 18:04

I have to point out that you guys did a great job in all the other cases
The malloc and not guarded integer multiplication at this point clearly seems to be a glitch
check out this line in the same file https://github.com/ocaml/ocaml/blob/ea60609504cc53c06a2711bba51b9a37db557a28/byterun/bigarray.c#L108 [^]

Where it is implemented safe

(0018986)
frisch (developer)
2018-04-06 18:18

I agree it is worth protecting against this specific source of overflow.

But let me insist: Marshal is known to not be safe, and should really not be used for communication if you don't trust the other end.

> this argumentation would mean ocaml is not safe for any usage over network in general

How do you draw such a conclusion? If you read an integer from an unstrusted source and call Obj.magic on it to cast it to a bytes values, you can obviously let the "attacker" read/write arbitrary memory. It only means that you should not do that. And as far as I know, nobody demarshals data received from untrusted counterparties.
(0018987)
maxdos (reporter)
2018-04-06 18:24
edited on: 2018-04-06 18:40

> I agree it is worth protecting against this specific source of overflow.

Yes that is my main point, you guys did great work in protecting the rest of it so why not fix this with the protection you already implemented and make ocamls stdlib more secure :)

Also i agree that serializing over network is always dangerous

But imagine scenarios of man-in-the-middle where a package you are serializing can be corrupted or maybe you unserialize a corrupted file you got through
someone or some interface
Also we are not casting anything to byte values here but just using "black box" functions of bigarray (namely input_value which seems pretty innocent)
which is dangerous even if you warn the users
And another (admitably picky) point: im not sending a wrong type but a corrupt object of the correct type :D

and overall its a small patch which adds great security to your already pretty secure marshaling interface :)

(0018988)
frisch (developer)
2018-04-06 19:06

Even when this will be fixed, demarshaling will not really become more safe against corruption or attacks. If you expect to unmarshal a value of type "bytes" but the attacker sends a marshaled integer, you will easily end up writing at arbitrary memory location controlled by the attacker.
(0018989)
xleroy (administrator)
2018-04-06 19:56

Well spotted, thanks! We'll fix this overflow issue by the next release, of course.

@frisch is correct that there are other ways to abuse unmarshaling at the level of the OCaml application. But those can often be avoided by post-unmarshaling data validation, for example like in Gregoire Henry's PhD work. In contrast, unsafety in the run-time system is fatal and cannot be worked around. So we'll fix the unsafety ASAP.
(0018999)
Richard Jones (reporter)
2018-04-10 09:11

Just reviewing this issue for Fedora & RHEL. It looks like there isn't yet an upstream patch, is that right?
(0019000)
frisch (developer)
2018-04-10 09:32

> In contrast, unsafety in the run-time system is fatal and cannot be worked around.

Which safety properties do we expect from the demarshaler, applied to a block crafted by the attacker?

I don't see any protection against arbitrary memory reads in the demarshaler. For instance, CODE_STRING64 will get an arbitrary 64-bit integer length, and happily read (with readblock) that much memory from the input buffer without bound checks. If arbitrary memory reads are not bad enough, the same code also do:

       Field(v, size - 1) = 0;

or things like:

       *intern_dest = Make_header_allocated_here(Double_wosize, Double_tag,
                                                  intern_color);

which could write at an attacker-controlled memory location (nothing prevents intern_dest from overflowing the allocated target buffer).

Also, the attacker could trivially force the whole program to stop (just emit CODE_SHARED8 followed by \0).
(0019002)
maxdos (reporter)
2018-04-10 12:09

@frisch: but there we are talking about custom object correct ?
I think this is a different since if you allow custom objects you are risking more.
(0019004)
frisch (developer)
2018-04-10 14:14

CODE_STRING64 is not related to custom objects. The problem you reported (with bigarrays) is indeed, but I think this is irrelevant, since support for custom objects cannot be disabled.
(0019008)
xleroy (administrator)
2018-04-11 13:29

Proposed fix: https://github.com/ocaml/ocaml/pull/1718 [^]
(0019015)
xleroy (administrator)
2018-04-12 09:37

Fix reviewed and merged, will be in release 4.07.0.

@frisch is right: other sanity checks are missing in the unmarshaler and should be investigated soon.
(0019016)
frisch (developer)
2018-04-13 10:44

> @frisch is right: other sanity checks are missing in the unmarshaler and should be investigated soon.

I'm not against adding more check, but this will add quite a bit of weight to the code (I expect the impact on performance to remain small, though).

But I'm dubious at the idea of relying on post hoc checks of the unmarshal data to ensure security. I can see some use of such techniques to reduce the risk of versioning or corruption problems, but advertising this as a secure approach seems dangerous to me.

Validation of unmarshaled data requires a runtime representation of the data structure definition, and involve some non negligible runtime cost. At this point, why would one want to rely on the generic marshaling rather than a type-driven serialization (which could be more compact, btw)? The difficult part would be to deal with sharing data, but this is also quite difficult for the validator, as it needs to reject sharing of mutable data with different types.

I'd feel much more comfortable if we advised to "never unmarshal untrusted data" (and similarly, "never dynlink untrusted code", if this is not obvious).
(0019020)
maxdos (reporter)
2018-04-13 14:19

I understand your argument regarding runtime
But the point is that the way the marshal module is used it implies security
Since the user relies on that you check for example array sizes and so on
(not talking about custom objects here)

Your suggestions sounds like: Why dont we say ocaml is not safe to use over network and then we are done ?
(0019021)
frisch (developer)
2018-04-13 14:36

> But the point is that the way the marshal module is used it implies security

Absolutely not, and that's because you were somehow lead to believe that that I believe it's important to be clear in the documentation.

Marshal should **not** used in contexts where an attacker can control the data. I don't believe it is, at least in any project I'm aware of, and if it were, it's unlikely that those project perform enough check on the result of Marshal to make the use safe anyway.

> Why dont we say ocaml is not safe to use over network and then we are done ?

This has nothing to do with the use of ocaml over the network, it's about using a library documented to be unsafe on untrusted data. Of course, it's a bad idea, but this is not breaking news.
(0019022)
frisch (developer)
2018-04-13 14:39

Again: are you aware of any use of Marshal to pass data over the network, in a context where you don't trust the other side ?

- Issue History
Date Modified Username Field Change
2018-04-06 17:41 maxdos New Issue
2018-04-06 17:56 frisch Note Added: 0018983
2018-04-06 18:00 maxdos Note Added: 0018984
2018-04-06 18:01 maxdos Note Edited: 0018984 View Revisions
2018-04-06 18:02 maxdos Note Edited: 0018984 View Revisions
2018-04-06 18:03 maxdos Note Added: 0018985
2018-04-06 18:04 maxdos Note Edited: 0018985 View Revisions
2018-04-06 18:18 frisch Note Added: 0018986
2018-04-06 18:24 maxdos Note Added: 0018987
2018-04-06 18:40 maxdos Note Edited: 0018987 View Revisions
2018-04-06 19:06 frisch Note Added: 0018988
2018-04-06 19:56 xleroy Note Added: 0018989
2018-04-06 19:57 xleroy Priority high => urgent
2018-04-06 19:57 xleroy Status new => acknowledged
2018-04-06 19:57 xleroy Target Version => 4.07.0+dev/beta2
2018-04-10 09:11 Richard Jones Note Added: 0018999
2018-04-10 09:23 frisch Note View State: 0018988: public
2018-04-10 09:32 frisch Note Added: 0019000
2018-04-10 12:09 maxdos Note Added: 0019002
2018-04-10 14:14 frisch Note Added: 0019004
2018-04-11 13:29 xleroy Note Added: 0019008
2018-04-12 09:37 xleroy Note Added: 0019015
2018-04-12 09:37 xleroy Status acknowledged => resolved
2018-04-12 09:37 xleroy Resolution open => fixed
2018-04-12 09:37 xleroy Fixed in Version => 4.07.0+dev/beta2
2018-04-13 10:44 frisch Note Added: 0019016
2018-04-13 14:19 maxdos Note Added: 0019020
2018-04-13 14:36 frisch Note Added: 0019021
2018-04-13 14:39 frisch Note Added: 0019022


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker