Skip to content
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

Integer overflow and null pointer dereference in CMap::Load() in engine/shared/map.cpp #2071

Closed
0n3m4ns4rmy opened this issue Mar 22, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@0n3m4ns4rmy
Copy link

commented Mar 22, 2019

Hello Teeworlds dev team,

There is an integer overflow in CMap::Load() which can lead to a buffer overflow.

CTile *pTiles = static_cast<CTile *>(mem_alloc(pTilemap->m_Width * pTilemap->m_Height * sizeof(CTile), 1));

// extract original tile data
int i = 0;
CTile *pSavedTiles = static_cast<CTile *>(m_DataFile.GetData(pTilemap->m_Data));
while(i < pTilemap->m_Width * pTilemap->m_Height)
{
	for(unsigned Counter = 0; Counter <= pSavedTiles->m_Skip && i < pTilemap->m_Width * pTilemap->m_Height; Counter++)
	{
		pTiles[i] = *pSavedTiles;
		pTiles[i++].m_Skip = 0;
	}

	pSavedTiles++;
}

pTilemap->m_Width and pTilemap->m_Height can be arbitrary integers and there is no check for an integer overflow when multiplying these integers with each other and with sizeof(CTile).

Also there is no check if mem_alloc returns NULL which can lead to a null pointer dereference.

Regards,

Mans van Someren

http://whatthebug.net/

@Dune-jr

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

The code is located here:

CTile *pTiles = static_cast<CTile *>(mem_alloc(pTilemap->m_Width * pTilemap->m_Height * sizeof(CTile), 1));
// extract original tile data
int i = 0;
CTile *pSavedTiles = static_cast<CTile *>(m_DataFile.GetData(pTilemap->m_Data));
while(i < pTilemap->m_Width * pTilemap->m_Height)
{
for(unsigned Counter = 0; Counter <= pSavedTiles->m_Skip && i < pTilemap->m_Width * pTilemap->m_Height; Counter++)
{
pTiles[i] = *pSavedTiles;
pTiles[i++].m_Skip = 0;
}
pSavedTiles++;
}

I agree this is a security issue. Should we simply clamp pTilemap->m_Width and pTilemap->m_Height?

@0n3m4ns4rmy

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

You could limit pTilemap->m_Width and pTilemap->m_Height so that the integer overflow cant occur or add a check after the multiplications. I suggest reading the accepted answer on this stack overflow post: https://stackoverflow.com/questions/1815367/catch-and-compute-overflow-during-multiplication-of-two-large-integers

Dune-jr added a commit to Dune-jr/teeworlds that referenced this issue Mar 25, 2019

Dune-jr added a commit to Dune-jr/teeworlds that referenced this issue Mar 25, 2019

Dune-jr added a commit to Dune-jr/teeworlds that referenced this issue Mar 26, 2019

Dune-jr added a commit to Dune-jr/teeworlds that referenced this issue Mar 27, 2019

@oy oy closed this in #2076 Mar 28, 2019

oy added a commit that referenced this issue Mar 28, 2019

Merge pull request #2076 from Dune-jr/fix-stability2
Fix integer overflow when computing tilemap size. Fixes #2071
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.