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 strdup() on possibly unterminated string #40
fix strdup() on possibly unterminated string #40
Conversation
Otherwise, a buffer read overflow may happen at file.c line 236
@pauldreik Thanks for this patch. I'm away on vacation at the moment so I'm not going to have a chance to look into this more deeply until next week. Can you check into why the build is failing? Looks like one of the automated tests is failing. Thanks. |
this relaxes the memory limit check, so one byte extra may be allowed, after the check vs the limit the user sets.
10e0216
to
3993ca1
Compare
It was the memory allocation test which failed, because it compared the output to the last time. I reworked it a bit, now it passes. |
Hi @verdammelt , have you had a chance to look at this yet? |
Instead of changing the interface of xcalloc/xmalloc can we just have that the extra ADDNULL macros which |
I started with the +1 approach, but that is dangerous since it creates a possibility for integer overflow on the call site. See here: 7109b9e Also, it will make the tests break since they check the debug output compared to the checked in output from an earlier version. Then I moved the "+1 functionality" inside the xcalloc call: 375d20f I understand you do not like it, it is pretty ugly, but at least it is correct. You can implement this however you prefer, I am happy as long as the end result is free of undefined behaviour, buffer overflows and integer overflows! |
ah, I think I see your point. I'll merge. |
Also @pauldreik thanks for the PRs... |
My pleasure! |
@verdammelt can you comment on what the consequence could be of this bug? I would say opening a crafted file could lead to a file with a weird filename may be created on the system, but could that weird file name end up being in another directory (say ../../.ssh/authorized_keys if the heap could be prepared)? |
Yes, it seems perhaps with a specially constructed file you could get files written to unexpected locations or with contents not as expected. I don't know how easy it would be to do either - but it seems that this bug would make that possible. |
This has been assigned CVE-2019-18849 |
A buffer read overflow may happen at
file.c line 236 where strdup() is invoked.
strdup() will just search until it finds a null terminator. If there was none, it will continue past the heap allocated memory. (Update 20191110: there is another one in mapi_attr.c, see 3ae8b93)
The effect of this read overflow is that either the
application crashes from a segfault, or "random" data is being
duplicated, the effect of which I do not know. Writing a file with
garbage suffixed to the name, perhaps, but there seem to be some kind of
sanitation in path.c preventing that.
Update: there is another similar situation in mapi_attr.c, also fixed in this pull request.
An example input that triggers the behaviour is base64 encoded here:
To prove the bug, run
and get (source lines are not accurate, I ran it on the patched version, with the fix disabled)