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

Several security vulnerabilities in ELF file parser #255

Closed
zzazzdzz opened this Issue Jun 13, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@zzazzdzz

zzazzdzz commented Jun 13, 2018

Curious of the ELF file loading functionality in VBA-M that I accidentally discovered a few days ago, I quickly looked through the code responsible of making it work.
It turns out, several security vulnerabilities are present in the ELF file parser. Most of them happen to be mitigated by the default hardening features on Windows and Linux platforms (which does not mean they shouldn't be fixed).

Particularly, one of the vulnerabilities is fully exploitable on the Windows platform and allows for information disclosure. The emulated ROM can access arbitrary information from the emulator's process memory, as seen from this screenshot: https://cdn.pbrd.co/images/HpKVzGP.png

The original plan was to privately disclose any details related to these bugs to the project owner, to have them fixed before publishing any information - however, seeing that this is an open-source project with large amount of contributors, and the project site doesn't give any means of private contact, I'm forced to just submit an issue publicly.
For now, I will only disclose the information necessary for bugfixing; all proof of concept exploits and technical documentation will be published once the vulnerabilities get patched.

(1) In handling of both PT_LOAD segments and SHT_PROGBITS sections, memcpy calls like this or similar are used:

if (READ32LE(&ph->paddr) >= 0x2000000 && READ32LE(&ph->paddr) <= 0x203ffff) {
memcpy(&workRAM[READ32LE(&ph->paddr) & 0x3ffff],
data + READ32LE(&ph->offset),
READ32LE(&ph->filesz));
size += READ32LE(&ph->filesz);

The offsets and addresses are loaded directly from the ELF image and they are not sanitized in any way, excluding the starting if statement. There are two potential problems here:

  • While the code logic ensures that the loading address is valid, it does not take the section size into account. Loading a section with large enough size may overflow the destination array (workRAM in this case).
  • No checks of any kind are done on the source pointer. By providing a section offset exceeding the size of the loaded ELF file, it's possible to read from unrelated process memory areas and copy them into the emulated ROM/RAM.

An example solution to this problem would be to validate whether all the pointers are in bounds before using them.

(2) The e_shstrndx field in the program header should determine the index in the section header table that represents the section name table's entry. A value of 0 indicates that the loaded image has no sections (and thus, no name table). This case is handled as follows:

if (READ16LE(&eh->e_shstrndx) != 0) {
stringTable = (char*)elfReadSection(data,
sh[READ16LE(&eh->e_shstrndx)]);
}

stringTable is set to NULL during initialization. If e_shstrndx is set to 0, the variable remains set to NULL. This is normally irrelevant – if the image has no sections, no operation will ever be performed on that pointer. However, it is possible to create a malformed file that both contains a nonzero number of sections, and sets the name table index to 0. This would cause a null pointer dereference the first time a section name is loaded. Additionally, by providing specially crafted name offsets in section header entries, it's possible to point a section name anywhere in memory, creating arbitrary memory reads.

A solution to this problem would be either to parse section headers if and only if e_shstrndx is nonzero, or make elfGetSectionByName return NULL if the name table points to NULL. Or ideally, both.

(3) VBA (and consequently, VBA-M) support loading symbols encoded in .symtab sections, which is a common method of introducing debug information to ELF binaries. The function responsible for getting a symbol name from its address uses a 256-byte buffer - therefore, symbol names can contain at most 255 characters.

Interestingly, if a symbol table is loaded, there is some code logic that displays the symbol names in the built-in disassembly window (but only for the THUMB instruction set, for unknown reason). However, the buffer allocated for each disassembled instruction is only 80 bytes in size, as seen below.

char buf[80];
dis->strings.clear();
dis->addrs.clear();
uint32_t addr = dis->topaddr;
bool arm = dismode == 1 || (armState && dismode != 2);
dis->back_size = arm ? 4 : 2;
for (int i = 0; i < dis->nlines; i++) {
dis->addrs.push_back(addr);
if (arm)
addr += disArm(addr, buf, DIS_VIEW_CODE | DIS_VIEW_ADDRESS);
else
addr += disThumb(addr, buf, DIS_VIEW_CODE | DIS_VIEW_ADDRESS);

Combine that with the 255-character symbol names to get a rather standard buffer overflow.
To solve this one, the target buffer size can be increased to contain the potential 255-character names - for example, to 512 bytes. For extra security, change the disArm and disThumb functions to accept and enforce a maximum buffer size.

Note: All details of the found vulnerabilities, including proof of concept exploits, will be fully disclosed after 14 days (June 27th, 2018), unless the parties agree on extending that period to a maximum of 21 days (July 4th, 2018).

@ZachBacon

This comment has been minimized.

Contributor

ZachBacon commented Jun 13, 2018

I much appreciate the detailed report. Even though I am project lead currently, time is a bit of an issue for me (full time job eating what free time I have lately) I'll see about getting this vulnerability fixed asap. I'll also see about providing a way to contact us if preffered privately first for future.

@ZachBacon

This comment has been minimized.

Contributor

ZachBacon commented Jun 21, 2018

The code is being worked on, just a lot of us do have a life, it's not that we're ignoring it, it's just time. If you're free and are able to jump on gitter or irc we would enjoy the chance in chatting further

@zzazzdzz

This comment has been minimized.

zzazzdzz commented Jun 21, 2018

Thank you for your response. During the past week the repository seemed to be fairly active, which left me under the impression that this issue is being ignored. Sorry for the misunderstanding.

Now that I see the problem is being worked on, there's no need to rush anyone - so I'm willing to agree on a longer disclosure period.

I'll be sure to drop in on IRC when I'm free.

rkitover added a commit that referenced this issue Jun 26, 2018

fix some ELF parsing vulnerabilities #255
Implement the recommendations described in issue #255 by @zzazzdzz:

- Implement bounds checking for reading ELF program header sections.

- Skip reading ELF section headers if the string table pointer is NULL.

- Increase the buffer size for dissassembled instructions in the
  dissassembly view and pass the buffer size to the disArm() and
  disThumb() functions so that rudimentary bounds checking can be done.

Also add the constants WORK_RAM_SIZE and ROM_SIZE to reduce incidence of
magic numbers and make the code a bit cleaner.
@rkitover

This comment has been minimized.

Collaborator

rkitover commented Jun 26, 2018

Hello @zzazzdzz,

First of all, thank you for the extremely detailed analysis, this made implementing fixes much much easier.

Could you please look at d944cc4 which is currently in the branch elf-vulns and let us know if it addresses the problems adequately.

Thank you!

rkitover added a commit that referenced this issue Jun 26, 2018

fix some ELF parsing vulnerabilities #255
Implement the recommendations described in issue #255 by @zzazzdzz:

- Check bounds when reading ELF program header sections.

- Skip reading ELF section headers if the string table pointer is NULL.

- Increase the buffer size for dissassembled instructions in the
  dissassembly view and pass the buffer size to the disArm() and
  disThumb() functions so that rudimentary bounds checking can be done.

Also add the constants WORK_RAM_SIZE and ROM_SIZE to reduce incidence of
magic numbers and make the code a bit cleaner.

@rkitover rkitover self-assigned this Jun 26, 2018

@rkitover

This comment has been minimized.

Collaborator

rkitover commented Jun 26, 2018

Updated commit is 9e9ce98 above, changed a couple ints to unsigned and fixed an incompatibility with clang.

rkitover added a commit that referenced this issue Jun 26, 2018

fix some ELF parsing vulnerabilities #255
Implement the recommendations described in issue #255 by @zzazzdzz:

- Check bounds when reading ELF program header sections.

- Skip reading ELF section headers if the string table pointer is NULL.

- Increase the buffer size for dissassembled instructions in the
  dissassembly view and pass the buffer size to the disArm() and
  disThumb() functions so that rudimentary bounds checking can be done.

Also add the constants WORK_RAM_SIZE and ROM_SIZE to reduce incidence of
magic numbers and make the code a bit cleaner.
@rkitover

This comment has been minimized.

Collaborator

rkitover commented Jun 26, 2018

Fixed off by one error in comparison in b7a3371.

@zzazzdzz

This comment has been minimized.

zzazzdzz commented Jun 29, 2018

I examined the changes and didn't immediately find any problems. I believe these modifications are sufficient to prevent all the described issues from happening.

I would only be cautious about this check:

if (offset + section_size > data_size)
continue;

offset + section_size could potentially overflow and wrap around - for example, a section with load offset 1 and size 0xFFFFFFFF would pass through (since 1+0xFFFFFFFF is 0 in 32-bit unsigned arithmetic).

There are some later checks that seem to catch this and prevent any damage, but I think it's better to just avoid the whole scenario, by checking for offset > data_size and section_size > data_size in addition to offset + section_size > data_size.

rkitover added a commit that referenced this issue Jun 29, 2018

fix some ELF parsing vulnerabilities #255
Implement the recommendations described in issue #255 by @zzazzdzz:

- Check bounds when reading ELF program header sections.

- Skip reading ELF section headers if the string table pointer is NULL.

- Increase the buffer size for dissassembled instructions in the
  dissassembly view and pass the buffer size to the disArm() and
  disThumb() functions so that rudimentary bounds checking can be done.

Also add the constants WORK_RAM_SIZE and ROM_SIZE to reduce incidence of
magic numbers and make the code a bit cleaner.
@rkitover

This comment has been minimized.

Collaborator

rkitover commented Jun 29, 2018

Thank you very much for reviewing the code.

I've made the change you recommended and merged to master.

Let's leave the issue open for now until we do a new release.

@rkitover

This comment has been minimized.

Collaborator

rkitover commented Jul 6, 2018

Released in 2.1.0.

@rkitover rkitover closed this Jul 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment