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

Infinite loop in Parser#next_block #2888

Closed
zelivans opened this Issue Sep 26, 2018 · 10 comments

Comments

Projects
None yet
2 participants
@zelivans
Contributor

zelivans commented Sep 26, 2018

This is another bug found with the fuzzer. With specific input it's possible to cause an inifinite loop in the #next_block method when parsing an asciidoctor file, due to a never breaking while true statement.

I think this bug should be prioritized since it can cause a denial of service in programs that rely on asciidoctor. Specifically GitHub is safe since it uses aggressive timeouts in its markup process, although I did later get a 500 error on the GitHub main page for a few hours that I've never seen before. I can't tell if it's related or not.

Example input

::
///::
x

And also

* x
///::
x

Explanation

In the #next_block method there is a while true statement that is used for flow control, and meant to execute only once according to its documentation. I provide no PR since I am not sure what is missing from the loop logic. Plus, I personally think it would be good practice to rewrite this and eliminate the use of an infinite loop especially since it is only used for flow control.

@zelivans

This comment has been minimized.

Contributor

zelivans commented Oct 16, 2018

Assigned CVE-2018-18385 (DoS)

@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Oct 16, 2018

I haven't forgotten about this. Also, I agree it's time to refactor that while loop into a method. I'll see what I can come up with.

@mojavelinux mojavelinux self-assigned this Oct 16, 2018

@mojavelinux mojavelinux added this to the v1.5.8 milestone Oct 16, 2018

@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Oct 18, 2018

I guess this is what I'll be working on tonight. As soon as I sort out how to fix this, I will begin preparing the 1.5.8 release.

mojavelinux added a commit to mojavelinux/asciidoctor that referenced this issue Oct 19, 2018

resolves asciidoctor#2888 don't hang on description list item that be…
…gins with ///

- regexps for any list item and description list item must agree
@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Oct 19, 2018

The problem is that the regexp for any list and the regexp for a description list did not agree. Specifically, the any list regexp was matching an entry that begins with ///, whereas the description list regexp was not.

@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Oct 19, 2018

I also made the check for a line comment more intelligent. A line comment starts with // unless it starts with ///.

mojavelinux added a commit to mojavelinux/asciidoctor that referenced this issue Oct 19, 2018

resolves asciidoctor#2888 don't hang on description list item that be…
…gins with ///

- regexps for any list item and description list item must agree
@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Oct 19, 2018

For the record, the infinite loop is not in Parser.next_block, and thus not related to the aforementioned while loop. The infinite loop is in Parser.parse_list_item. The method expects Parser.next_block to eventually consume all lines, but due to this bug, the lines are never exhausted.

mojavelinux added a commit to mojavelinux/asciidoctor that referenced this issue Oct 20, 2018

resolves asciidoctor#2888 don't hang on description list item that be…
…gins with ///

- regexps for any list item and description list item must agree
@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Oct 20, 2018

This issue is now resolved in master.

@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Oct 20, 2018

I want to challenge the statement in the CVE that Asciidoctor misuses the while true statement. The statement has an explicit break at the end of the loop which prevents it from ever executing more than once (see https://github.com/asciidoctor/asciidoctor/blob/v1.5.7.1/lib/asciidoctor/parser.rb#L763).

As I explained above, the infinite loop was actually coming from a different while loop. In particular, this one: https://github.com/asciidoctor/asciidoctor/blob/v1.5.7.1/lib/asciidoctor/parser.rb#L1342-L1346

The infinite loop was caused by the fact that Parser.next_block was not exhausting all the lines in the reader as the while loop expected it would. This was happening because the regular expression that detects any list was not agreeing with the regular expression that detects a specific list type. So the line kept getting pushed back onto the reader, hence causing the loop.

@zelivans

This comment has been minimized.

Contributor

zelivans commented Oct 20, 2018

@mojavelinux I am sorry for having mislead you with the explanation. When I manually killed the execution few times I was stopped at Parser.next_block so I made the faulty conclusion that the bug is in this method. This may have took you some time debugging the wrong thing 😞

The CVE was supposed to be reserved until the issue was closed. I'll request to have its description corrected ASAP using your explanation in the above comment, plus indicate the fixed version.

Thank you for solving this so quickly (and all other bugs too, but taking care of this one seriously given I didn't have a good solution or explanation).

@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Oct 20, 2018

@zelivans You're welcome. And thank you. I want you to know that I am very grateful that you discovered and reported this issue. Trust me, your report didn't lead me astray at all. Even though the problem turned out to be on a different line than you suggested, it was still in the same vicinity, which allowed me to track it down quickly. I only pointed out the distinction in an effort to provide as much information as possible for the report. So there's no reason to be 😞. Instead, let's be 😄 about teamwork.

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