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

Fix a buffer overflow by getting rid of all strcpy() calls #38

Merged
merged 1 commit into from Jan 8, 2018

Conversation

@fcambus
Copy link
Contributor

fcambus commented Jan 23, 2016

Hello,

When compiling on OpenBSD, the linker produces a warning :

cd src && make all
cc -O2 -pipe   -c nyancat.c
cc  -O2 -pipe   nyancat.o -o nyancat
nyancat.o: In function `main':
nyancat.c:(.text+0x7c7): warning: warning: strcpy() is almost always misused, please use strlcpy()

After reading the code, it appears there are no boundary checks being done, and setting TERM to a sufficiently long string and running nyancat afterwise triggers a buffer overflow.

TERM=0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF

OpenBSD linker suggests using strlcpy but it's not part of C99 and is not available on Linux systems, and strncpy should be avoided.

Here is a diff avoiding copying strings altogether and removing the two calls to strcpy().

@@ -485,7 +484,7 @@ int main(int argc, char ** argv) {
/* This was a response to the TTYPE command, meaning
* that this should be a terminal type */
alarm(2);
strcpy(term, (char *)&sb[2]);
term = (char *)&sb[2];

This comment has been minimized.

Copy link
@klange

klange Jan 23, 2016

Owner

This isn't going to work. It needs at least a strdup.

This comment has been minimized.

Copy link
@fcambus

fcambus Jan 23, 2016

Author Contributor

I assume this is because there can be cases where the content of sb is modified before reaching terminal detection checks?

This comment has been minimized.

Copy link
@klange

klange Jan 23, 2016

Owner

Yes, specifically because we use sb for both the terminal and the terminal size, and the order in which the client gives us those isn't necessarily defined.

This comment has been minimized.

Copy link
@fcambus

fcambus Jan 24, 2016

Author Contributor

Makes sense, thanks for pointing this out. I modified my diff to use strndup, which should fix the issue.

@fcambus

This comment has been minimized.

Copy link
Contributor Author

fcambus commented Jan 23, 2016

For info, I ammended my commit to change char *term; fo char *term = NULL; on L350 as invoking nyancat -t directly from a shell crashed the program when compiled with GCC, didn't notice it as I was using Clang.

@fcambus

This comment has been minimized.

Copy link
Contributor Author

fcambus commented Jan 31, 2016

Any thoughts regarding this diff? Is there anything requiring changes?

@akatrevorjay

This comment has been minimized.

Copy link

akatrevorjay commented Jan 7, 2018

Um, can we merge this?
Think of people running nyancat-server :)

akatrevorjay added a commit to akatrevorjay/nyancat that referenced this pull request Jan 7, 2018
@klange klange merged commit e5a3a2a into klange:master Jan 8, 2018
@fcambus

This comment has been minimized.

Copy link
Contributor Author

fcambus commented Jan 15, 2018

Thanks for merging!

@klange: any chance you could tag a new version so distributions can update their packages?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.