multiple heap out of bounds reads in libinjection_xss()

[invd]

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]

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

Thank you.

[client9]

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

[zimmerle]

Hi, @client9 It will be a pleasure.

[zimmerle]

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]

@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]

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: https://github.com/client9/libinjection/blob/e86ff4019a4343579cc307d96d79272d5efcd1be/src/libinjection_xss.c#L510-L513

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]

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.

[migueldemoura]

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]

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]

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]

@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 https://github.com/libinjection/libinjection/commit/991433e7f0cd8db3ad24f1b0c429881d44251505 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]

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

[migueldemoura]

@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.

[invd]

From my perspective, https://github.com/libinjection/libinjection/commit/991433e7f0cd8db3ad24f1b0c429881d44251505 in the project successor repository resolves the issue.

At this point, the client9/libinjection repository is unlikely to adopt the documentation patch, so I'm closing this ticket.