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

LcfReader::ReadString heap corruption on maliciously crafted files. #194

Closed
scrippie opened this Issue Dec 5, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@scrippie

scrippie commented Dec 5, 2016

LcfReader::ReadString() can corrupt the heap if the size calculation size + 1 wraps around. This allocates a region of size 0, while copying more data than this space can hold.

Below a simple test to trigger the problem through LDB_Reader which reads a signed integer of -1 for the size, which is converted to a size_t, resulting in heap corruption on my Linux system.

It's probably a more safe idea to error in this situation, rather than opening the door for execution of arbitrary code through malicious LDB files.

#include <cstdio>
#include <cstdlib>
#include "ldb_reader.h"
#include "reader_lcf.h"
#include "writer_lcf.h"

void build_test_ldb(void)
{
        LcfWriter writer("test_ldb_reader.bin", "UTF-8");

        writer.WriteInt(-1);

        for (unsigned int i = 0; i < 10000; i++)
                writer.Write((uint8_t)'A');
}

int main(void)
{
        build_test_ldb();

        if (!LDB_Reader::Load("test_ldb_reader.bin", "UTF-8")) {
                fprintf(stderr, "%s\n", LcfReader::GetError().c_str());
                exit(EXIT_FAILURE);
        }

        exit(EXIT_SUCCESS);
}

Proposed untested patch is below. This does not add space for the terminating 0-byte, as it is unnecessary for the std::string constructor called. This also allows 0-bytes to be part of the string, which is arguably more sensible for length encoded rather than 0-terminated strings.

diff --git a/src/reader_lcf.cpp b/src/reader_lcf.cpp
index b3141bb..3e07e99 100644
--- a/src/reader_lcf.cpp
+++ b/src/reader_lcf.cpp
@@ -160,10 +160,9 @@ void LcfReader::Read<uint32_t>(std::vector<uint32_t> &buffer, size_t size) {
 }
 
 void LcfReader::ReadString(std::string& ref, size_t size) {
-       char* chars = new char[size + 1];
-       chars[size] = '\0';
+       char* chars = new char[size];
        Read(chars, 1, size);
-       ref = Encode(std::string(chars));
+       ref = Encode(std::string(chars, size));
        delete[] chars;
 }
@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Dec 6, 2016

Member

Thanks for the patch. Please note that we currently don't care much about security in liblcf and Player. I assume that it is quite easy to create a malicious ldb file which generates out of bounds reads and writes in the Player due to missing range checks.

But bugs that allow the exploitation of liblcf directly are of course worse because the lib is used by multiple programs. If you find any other vulnerable spots please report them (but not normal crashes, there are probably hundreds ;) )

Member

Ghabry commented Dec 6, 2016

Thanks for the patch. Please note that we currently don't care much about security in liblcf and Player. I assume that it is quite easy to create a malicious ldb file which generates out of bounds reads and writes in the Player due to missing range checks.

But bugs that allow the exploitation of liblcf directly are of course worse because the lib is used by multiple programs. If you find any other vulnerable spots please report them (but not normal crashes, there are probably hundreds ;) )

@scrippie

This comment has been minimized.

Show comment
Hide comment
@scrippie

scrippie Dec 6, 2016

Yeah, I figured security wasn't high on the priority list at the moment. I was giving the project a read out of interest in the engine itself, but come from a security background so I always keep my eyes open for such issues.

I'll submit similar issues when I run into them, but for now will only bother with the ones that seem easy to fix and affect a small localized region in the code. That should be relatively low effort for you guys to deal with, while it hardens the engine a little for the future.

scrippie commented Dec 6, 2016

Yeah, I figured security wasn't high on the priority list at the moment. I was giving the project a read out of interest in the engine itself, but come from a security background so I always keep my eyes open for such issues.

I'll submit similar issues when I run into them, but for now will only bother with the ones that seem easy to fix and affect a small localized region in the code. That should be relatively low effort for you guys to deal with, while it hardens the engine a little for the future.

@Ghabry Ghabry added this to the 0.5.1 milestone Feb 21, 2017

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Feb 21, 2017

Member

Looks like a good candidate to fix before 0.5.1 ^^

Member

Ghabry commented Feb 21, 2017

Looks like a good candidate to fix before 0.5.1 ^^

carstene1ns added a commit that referenced this issue Apr 8, 2017

Merge pull request #198 from Ghabry/development
Fix #194 (heap corruption) and #196 (LcfWriter writing incorrect strings)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment