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

AddressSanitizer: heap-buffer-overflow daemon/remote/XmlRpc.cpp:896 in XmlCommand::NextParamAsInt(int*) #567

Closed
sanderjo opened this Issue Jul 20, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@sanderjo
Contributor

sanderjo commented Jul 20, 2018

I was playing with Address Sanitizer (https://github.com/google/sanitizers/wiki/AddressSanitizer): compiled nzbget (github version) with it, started nzbget (worked), but as soon as I access nzbget via the webinterface, nzbget stops and the console says the below. So a heap-buffer-overflow daemon/remote/XmlRpc.cpp:896 in XmlCommand::NextParamAsInt(int*).

Is this useful?

Please note: just playing with ASan.

#0 0x7fd8477d9d2f in __interceptor_pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x37d2f)
#1 0x5582c0fd1413 in Thread::Start() daemon/util/Thread.cpp:115
#2 0x5582c0c905fa in NZBGet::StartRemoteServer() daemon/main/nzbget.cpp:508
#3 0x5582c0ca10f7 in NZBGet::StartRemoteServer() daemon/main/nzbget.cpp:331
#4 0x5582c0ca10f7 in NZBGet::Run(bool) daemon/main/nzbget.cpp:711
#5 0x5582c0ca1628 in RunMain() daemon/main/nzbget.cpp:1022
#6 0x5582c0bb6840 in main daemon/main/nzbget.cpp:164
#7 0x7fd845b00b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
SUMMARY: AddressSanitizer: heap-buffer-overflow daemon/remote/XmlRpc.cpp:896 in XmlCommand::NextParamAsInt(int*)
                                                                                                                                                                                        
Shadow bytes around the buggy address:
                    
  0x0c067fffebc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffebd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffebe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffebf0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffec00: fa fa fd fd fd fd fa fa 00 00 03 fa fa fa 00 00
=>0x0c067fffec10:[03]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffec20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffec30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffec40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffec50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffec60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa

PS:

This is how I configured (hacked?) Address Sanitizer aka ASAN into nzbget's Makefile:

CXXFLAGS = -g -O2 -fsanitize=address -fno-omit-frame-pointer 
DEFS = -DHAVE_CONFIG_H -fsanitize=address -fno-omit-frame-pointer 
@sanderjo

This comment has been minimized.

Contributor

sanderjo commented Jul 29, 2018

Back from holiday, I could spend some time on this: the ASAN heap-buffer-overflow is gone with this code (see original https://github.com/nzbget/nzbget/blob/develop/daemon/remote/XmlRpc.cpp#L896-L900 )

		while (strchr("-+0123456789&", *m_requestPtr))
		{
			m_requestPtr++;
                        if ( *m_requestPtr == 0 ) {
				// end of string, so no need to proceed ... No idea if 'true' is good ...
				return true;
			}
		}
		return true;

So: just return if the end of the string is reached. I don't know if return true is any good (maybe it should be false), but I do know continuing beyond \0 is not useful.

@hugbug any feedback on this?

The debugging was this:

Inside XmlCommand::NextParamAsInt.
m_requestPtr is 
inside m_httpMethod == XmlRpcProcessor::hmGet.

Inside XmlCommand::NextParamAsInt.
m_requestPtr is =1&=0
inside m_httpMethod == XmlRpcProcessor::hmGet.
NOW: m_requestPtr is 1&=0
NOW 2: m_requestPtr is 1&=0
NOW 2: m_requestPtr is &=0

Inside XmlCommand::NextParamAsInt.
m_requestPtr is =0
inside m_httpMethod == XmlRpcProcessor::hmGet.
NOW: m_requestPtr is 0
NOW 2: m_requestPtr is 0
NOW 2: m_requestPtr is 

... and then de ASAN Heap Overflow kicked in.

@hugbug

This comment has been minimized.

Member

hugbug commented Jul 29, 2018

Thank you for fixing this.

I think the following version should do the same but is compacter, can you test it?

while (*m_requestPtr && strchr("-+0123456789&", *m_requestPtr))
{
	m_requestPtr++;
}
true;
@sanderjo

This comment has been minimized.

Contributor

sanderjo commented Jul 29, 2018

Ah, that is much more elegant code. 👍

Tested, and it works! No heap overflow with ASAN compiled in.

(FWIW: I ignored that line true; ... I just leftreturn true; in place)

@hugbug

This comment has been minimized.

Member

hugbug commented Jul 29, 2018

I ignored that line true;

It was meant return true, missed "return" when pasting ;)

Would you like to create a PR?

hugbug added a commit that referenced this issue Jul 30, 2018

#567, #569: NextParamAsInt: Stop parsing at end-of-string
fixed potential crash in web-interface.

@sanderjo sanderjo closed this Jul 30, 2018

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