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

multiple heap out of bounds reads in libinjection_xss() #150

Open
invd opened this issue Aug 1, 2020 · 13 comments
Open

multiple heap out of bounds reads in libinjection_xss() #150

invd opened this issue Aug 1, 2020 · 13 comments

Comments

@invd
Copy link

invd commented Aug 1, 2020

Mid-June, I discovered and privately reported out of bounds read issues in the XSS detection to @client9, but so far have not received a reply.

The out of bounds reads happen in multiple code positions. In theory, this may lead to information disclosure.
During analysis, one out of bounds read segfault was observed, but this could not be reproduced and is likely an artifact of the testing environment.

@client9: can you give some quick feedback on whether you want the details to be disclosed publicly here in the bugtracker or prefer them to stay nonpublic until the 16.9.2020 (90 days after initial disclosure)?

@zimmerle
Copy link
Contributor

zimmerle commented Aug 3, 2020

@invd is there any way for us to communicate out-of-band? felipe@zimmerle.org.

Thank you.

@client9
Copy link
Owner

client9 commented Aug 3, 2020

@zimmerle can I transfer this repo to you or someone you can properly maintain it.
@invd great work.

@zimmerle
Copy link
Contributor

zimmerle commented Aug 3, 2020

Hi, @client9 It will be a pleasure.

@zimmerle
Copy link
Contributor

zimmerle commented Aug 5, 2020

Status update

invd's report

I have received an e-mail from @invd with the details on the issue. I am currently analyzing the information. @invd congrats on your work and thanks for sharing the details.

On the behalf of the community I would like to ask you to not disclosure the details, instead give me the chance to analyze it and I will contact you with my initial thoughts before Monday (2020-08-10).

On the project status....

@client9 I have tried to reach you via e-mail, not sure if I have yours up-to-date contact. I would like to coordinate how can I be more useful to the project; you can reach me at: felipe@zimmerle.org (hangout or mail). I have configured this https://github.com/libinjection/ Let me know what do you think.

@invd
Copy link
Author

invd commented Aug 7, 2020

@zimmerle, @client9 : thanks for the positive feedback!

Given the (presumably) low severity of the memory issues and realistic chance of a patch, I have no objections to keeping the issues private until the 16.9.2020 as mentioned previously (or release date of a public security patch, whatever happens first).
If there are unforeseen delays, for example due to your planned project reorganization, an additional two-week extension of the disclosure deadline is generally possible if mutually agreed. Let me know if you think that this is necessary.

I have some additional suggestions that I will make via private channels since they reference parts of the issue discovery.

@invd
Copy link
Author

invd commented Aug 27, 2020

During private discussion, @zimmerle and I have figured out that the out of bounds reads are actually not a problem if libinjection_xss() is called with a pointer to a null-terminated string and a length value that excludes the null terminator.

The function definition doesn't state this very clearly, and so my fuzzer harness called it with a pointer to a buffer without the null termination and regular buffer length.

Relevant code:

/*
* wrapper
*/
int libinjection_xss(const char* s, size_t len)

As discussed with @zimmerle, I recommend marking the call parameters in the function description more clearly, but beyond that there appears to be no practical security problem and this issue can therefore be closed.

@zimmerle
Copy link
Contributor

zimmerle commented Aug 27, 2020

Thank you @invd! I am going to update the code to clarify those aspects, therefore, prevent one to use it in a manner that may lead to a security issue.

zimmerle pushed a commit to libinjection/libinjection that referenced this issue Sep 16, 2020
Patch to address the discussion held on:
client9/libinjection#150
@migueldemoura
Copy link

migueldemoura commented Sep 21, 2020

Is the input supposed to be a \0 terminated string or just a buffer with arbitrary bytes whose last byte is a \0?

@zimmerle added the following to the fork:

 * const char* s: is expected to be a null terminated string.
 * size_t len: should represent the length of the string
 *             without the null terminator - strlen(s). 

which would indicate the former (strlen() stops at the first \0). Is that correct?

I was under the impression that the point of a passing a len (besides O(1) length, etc) was to allow arbitrary bytes in said string.

@invd
Copy link
Author

invd commented Sep 24, 2020

Good question, this is something for @zimmerle to decide / clarify.

My original fuzzer harness was passing arbitrary bytes & length of the byte buffer, but this was not the intended usage.

@zimmerle
Copy link
Contributor

zimmerle commented Sep 24, 2020

Indeed a good question @migueldemoura. Your concern is about the comment itself or about the fact that the library expect the length to be informed, yet, demands the string to be null-terminated?

@migueldemoura
Copy link

migueldemoura commented Sep 26, 2020

@zimmerle

@migueldemoura: Is the input supposed to be a \0 terminated string or just a buffer with arbitrary bytes whose last byte is a \0?

If the former is what's expected (which it looks like from the comments above and the SQLi example), then libinjection/libinjection@991433e seems accurate to me.

However, since this library will likely be used to parse potentially malicious user input I, perhaps wrongly, expected it to accept arbitrary bytes in s, especially since s's length is also required in the function prototype.
Perhaps we could change this in the future, if anything, to avoid having to remove or replace \0 bytes in the input before passing it to libinjection.

@leveryd
Copy link

leveryd commented Dec 11, 2020

@migueldemoura any difference between "\0 terminated string" and "just a buffer with arbitrary bytes whose last byte is a \0"?

@migueldemoura
Copy link

migueldemoura commented Dec 11, 2020

@migueldemoura any difference between "\0 terminated string" and "just a buffer with arbitrary bytes whose last byte is a \0"?

The difference between the two is that a \0 terminated string can't contain any \0's as those signal the end of said string.

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

No branches or pull requests

5 participants