lauriewired / artful Goto Github PK

@LaurieWired

I just saw the defcon video about ARTful...

It's was a nice presentation. Well done. And I like such investigations.
You are smart.

As a long time C++ software engineer (>30 years C++, now retired) I have some comments.

Before starting, please note, that I did not dig very deep into the docs and I did not try to run any code, so it's all theory and I used some assumptions from my experiences with such things, so the whole comment certainly needs to be proved.

I think, in your first try, you were already near to your initial goal, but you probably have another language background and did not expect how things work in C++ (or C).

Now, what happens if you do this?:

benignMethod = maliciousMethod;

As far as I understand the docs of the JNI interface (never worked with it, but it uses familar technics and it's pretty obvious),

• you use GetFieldLong to read a field value into a local variable (so the private long, which is really a pointer, is copied into a void* variable that is living on the stack of the hooking function)
• you use SetFieldLong to write a value to the field

It's common, that the value of artMethod is a void* and not an ArtMethod*, because the structure of objects (here ArtMethod) are nearly always kept secret (reasoning see below).

So with the line above, you only change the value of the local variable benignMethod on the stack, but this is not used by the app later, and the stack with the changed variable is then cleared after the function call.

Instead, what you wanted to do is using SetFieldLong(benignMethod, ...) with the Value from the GetFieldLong(maliciousMethod, ...).
This would have put the ArtMethod pointer to the maliciousMethod into the ArtMethod pointer of the benignMethod inside the class structure.

Well, we do not really know where that pointer in that class is used. But it's likely that it is at least used by the application to call the function.
I guess this would change the method as intended, but it probably only affects the application, if the class structure is not shared between apps.

It's important, that if it really works as intended, it would be a future proof mechanism, because it only uses the defined API (well, despite ignoring the private...).

Now, what does the memcopy do?

It is one indirection level deeper. It copies the whole ArtMethod data structure from maliciousMethod over to the benignMethod structure, all the values and all the pointers (in the range of your configured size).
It may be possible that this affects more than the app itself, which I would call a major security hole.
I am not sure what you show there in the log, but in the defcon video it looks much like it also injected the maliciousMethod into other apps (e.g.I see Youtube)?

BUT

the memcpy has several problems, it is at least dangerous.

Such system code usually hides the implementation of a structure from the outside world for a good reason.

The structure is hidden intentionally, by only giving a pointer and some values to the API user but never revealing the structure of such an object.
The object implementation is kept hidden.
You usually cannot access the real header file (art_method.h etc.) when compiling your app, so you don't know anything about the structure.
Well, it is open source, so you can have a look. But it is still not a good idea, even for a malicious software.

Hiding the structure is necessary, because in C++ different compilers, compiler options, and even runtime changes (polymorphic behavior) may create very different structures.
Such memory surfaces could be allocated by malloc() and it's usage and structure could even be changed dynamically (not very usual but it happens).

At compilation time the compiler could notice that a variable is not used and optimize it away. Or it is free to use a different sequence of the variables than that in the source code (this is not usual but I have seen such things). The object mayu include pointers to it's methods or they could be optimized away if not used. It is also common to align variables to word boundaries because the CPU has usually faster access to a whole word than to the same data as a sequence of bytes , so there might be additional gaps in speed optimized compilations and everything packed without gaps when optimizing for low memory consumption.

Actually, your compiler and compile options are usually not the same like those used when compiling the ROM. Especially if you think that the ROM may be several years old, but your code is compiled now.

The ROM developer could have used debugging flags or macros and compiled very different data structures than your fully optimized code.
It's also common to add some fields in mission critical systems, e.g. for counting objects to detect object leakage at runtime and warn about it before the app becomes unusable. Hardened systems can also add padding around buffers and structures with certain numbers that can be checked regularly to detect buffer overflows.

So, there are quite a lot of reasons why the JNI interface hides those implementation details from the API user and handles those internally.

The only parts that must match is the calling convention (e.g. parameters on stack or partly in registers?, return address before or after the parameters, etc.) and some basic data sizes, like pointers and numbers.

Oh and 32 or 64 bit compilation usually also result in different structures.

If you use fixed numbers for the offsets and the size, you will probably get into trouble. It is only valid for the code you compile, not for the code of the ROM.

Additionally, with a well designed interface (for a language like java) you have no chance to calculate the offsets and size at runtime.
This could be done if you could get the addresses of the fields, but as far as I see, JNI (intentionally) does not have such functions but only provides the values.
Sometimes, like with artMethod you still get a void pointer, and if you could access an array of those structures (that are in the same memory surface and really laid out consecutively without other objects in between!) you could calculate the minimum distance and hope it's the size of a single structure.

Becasue of the implementation secret, you needed to copy the interesting part (another assumption) of the declaration of ArtMethod from the art_method.h file.
If you already use offsetof(), you can also use sizeof(ArtMethod) to determine the size of the structure.
But as explained above, those are probably not valid, anyways.

Another difficulty may be, that copying the structure might create memory leaks and other complications.
E.g. if it contains a pointer to memory allocated by malloc, then it would now be owned by two structures. The memory owned by the maliciousMethod would be released via free() two times, while the memory originally owned by benignMethod would not be freed at all (well, hopefully by garbage collection, but the copy might have broken the link).

So there should be an exception and it is also leaking memory.
Well, real malicious code could simply ignore this. But an exception might ring an alarm on the defensive side.