Out-of-bounds read, write in yr_execute_code

[bnbdr]

yr_execute_code has several bugs in the implementation of the virtual machine. Two of which prove to be security issues that allow code execution by running a specially crafted binary rule:

• An out-of-bounds read in the opcode OP_PUSH_M
• An out-of-bounds write in the opcode OP_POP_M.

These issues have been assigned the CVE-ID's CVE-2018-12034 and CVE-2018-12035, respectively.
Obvious ways to mitigate this:

• checking every access to scratch memory
• require an explicit flag to load and run a compiled rule
• check every relocated address is within the loaded file
• make the loaded file read-only

While this might not be as critical as say, a vulnerability that can be exploited by a rule in source form, YARA will run a binary rule without explicitly being told to do so. This means any service/third-party who doesn't properly validate the user-supplied rule is susceptible.

I suggest you reject rules in binary form unless explicitly being allowed to do so.

For the ones interested, I've written a (very long) write-up and made the PoC exploit available here

[plusvic]

Thanks @bnbdr, the write-up is a nice read, you have explained everything in detail. I've committed a first patch in #892, I'll keep studying other possible mitigations later.

[plusvic]

@bnbdr I have a question. In your write-up you say:

The format, unsurprisingly, starts with a header containing a magic, version and size of the remaining file(kinda). After some basic yet buggy validation yara reads the body of the file and performs relocations.

But you later don't explain why those validations are buggy. I guess your point is that YARA relies on the value in header.size and doesn't actually check the file's size on disk, is that what you meant or is something else?

[bnbdr]

@plusvic basically, yeah.
YARA will try to allocate and read header.size bytes from the file without checking the actual size in:
yr_arena_load_stream, yr_arena_create -> _yr_arena_new_page.

Might be nitpicking but the version check header.version != ARENA_FILE_VERSION will print rules were compiled with a newer version of YARA even if the arena version is older.

I didn't elaborate on this since It felt like bashing the code and the write-up was extremely long as is.

[plusvic]

The reason for not checking the actual file size is that YR_STREAM is an abstraction for anything that can provide two functions read and write, it could be a file or it could be something else, like a network connection. In practice the only existing implementations for YR_STREAM are file-based, so yes, we could add some function for getting the actual file size, but that would make the abstraction weaker. That would prevent YARA from loading rules from any other sources where you don't know the size in advance.

That doesn't mean a security issue by itself, except perhaps for a denial of service by memory exhaustion. If YARA is able to allocate the size specified in the header then it's safe to read that much data from the file, if not, it will fail without reading. But a denial of service will be always possible by corrupting some pointer that make the process crash, so protecting from that is unfeasible and out-of-scope. The only secure solution for that is digitally signing the compiled rules data in order to make sure that no one has tampered with it.

Regarding the version check, the full snippet is:

  if (header.version != ARENA_FILE_VERSION)
    return ERROR_UNSUPPORTED_FILE_VERSION;

Which is the intended behavior because changes in the structure of compiled rules are neither forward nor backward-compatible. The error message can be misleading certainly, as it could be quite the opposite and the rules could have been compiled with an older version of YARA.

[bnbdr]

Regarding the size in the header- maybe check the value doesn't exceed some hard-coded allocation limit?
I meant the error message should fit the check, not the other way round. I said this was nitpicky ;)

Both of these are more quality of life improvements in error reporting and validation rather than security issues.

[plusvic]

In #893 I introduced checks before accessing the scratch memory and some others pointed out by @jbremer. You also propose:

• require an explicit flag to load and run a compiled rule
• check every relocated address is within the loaded file
• make the loaded file read-only

The first one is actually a good idea, but I don't want to introduce backward incompatible changes that could break people's scripts out there, at least not for a minor version change. So, that's something for version 4.0 probably. The second one is already done in:

https://github.com/VirusTotal/yara/blob/master/libyara/arena.c#L978

It doesn't exactly check that the relocated address is within the file, but within the buffer allocated to hold the compiled rules.

The fourth one is unfeasible, the compiled rules has some empty slots that are filled during scanning and must remain writable.

[bnbdr]

https://github.com/VirusTotal/yara/blob/master/libyara/arena.c#L978
It doesn't exactly check that the relocated address is within the file, but within the buffer allocated to hold the compiled rules.

You misunderstood. YARA indeed only relocates offset within the loaded buffer, however, reloc_address itself can point anywhere after the relocation:
https://github.com/VirusTotal/yara/blob/master/libyara/arena.c#L990

[plusvic]

Ok, now I get what you meant. I have a commit ready for fixing that too.

[plusvic]

I've merged this PR in the master branch. Besides the changes aimed to fix the issues reported by @bnbdr, additional measures has been taking for mitigating other attack scenarios suggested by @jbremer and involving OP_CALL, could you guy review the changes?

I haven't assessed the performance impact of these changes but they are probably negligible, however they can't be easily switched-off by setting PARANOID_EXEC to 0.

Still it's highly discouraged to accept compiled rules from un-trusty third-parties, as they can always hand-craft a rule that can crash YARA.