Segmentation Fault #656

Activity

1. balthisar commented on Dec 10, 2017

   

   balthisar

   on Dec 10, 2017

   Last edited by balthisar

   Member

   More actions

   @yngwei, 谢谢, we'll have a look at it. Can you post your Poc file here? Add it as an attachment to a message.
2. geoffmcl commented on Dec 10, 2017

   

   geoffmcl

   on Dec 10, 2017

   Contributor

   More actions

   @yngwei thank you for the issue... and certainly agree with you, the loop logic is flawed... after modification in the loop currentNode->next could be invalid, and lead to a segfault...

   As @balthisar points out we will look at it soonest... and fix it... maybe a simple currentNode = (currentNode ? currentNode->next : NULL), or if (!currentNode) break; before each continue;, or something like that...

   That is with or without a sample PoC, but having one certainly aids quick testing, and provides a later regression test sample... thanks...
3. geoffmcl

   added

   Bug

   on Dec 10, 2017
4. geoffmcl

   added this to the 5.7 milestone on Dec 10, 2017
5. geoffmcl commented on Dec 10, 2017

   

   geoffmcl

   on Dec 10, 2017

   Contributor

   More actions

   @yngwei I have experimented with the following patch, and seems to work for me...

   diff --git a/src/clean.c b/src/clean.c
   index de4caf5..e96dd3f 100644
   --- a/src/clean.c
   +++ b/src/clean.c
   @@ -2211,8 +2211,10 @@ Bool TY_(TidyMetaCharset)(TidyDocImpl* doc)
        tidyBufAppend(&charsetString, "charset=", 8);
        tidyBufAppend(&charsetString, (char*)enc, TY_(tmbstrlen)(enc));
        tidyBufAppend(&charsetString, "\0", 1); /* zero terminate the buffer */
   -                                            /* process the children of the head */
   -    for (currentNode = head->content; currentNode; currentNode = currentNode->next)
   +    /* process the children of the head */
   +    /* Issue #656 - guard against 'currentNode' being set NULL in loop */
   +    for (currentNode = head->content; currentNode; 
   +        currentNode = (currentNode ? currentNode->next : NULL))
        {
            if (!nodeIsMETA(currentNode))
                continue;   /* not a meta node */

   Appreciated if you, or others, could give it a try... and/or comments... thanks...

   And on a PoC sample html, I now too would certainly appreciate that...

   I tried several ways to construct my own, but could not produce a simple sample where currentNode->prev was NULL, but still see it as a theoretic possibility, hence the suggested added loop guard...

   As @balthisar asked, "Can you post your Poc file here? Add it as an attachment to a message."... thanks...
6. yngwei commented on Dec 11, 2017

   

   yngwei

   on Dec 11, 2017

   Author

   More actions

   @geoffmcl This is the poc file.
   poc.zip
7. geoffmcl commented on Dec 11, 2017

   

   geoffmcl

   on Dec 11, 2017

   Last edited by geoffmcl

   Contributor

   More actions

   @yngwei thank you for the PoC file... it is the bad line 10, <meta charset<"utf-8"> that causes the problem to show up...

   From that I was able to construct a minimal test sample, which I would never have been able to guess, which exposes the problem -

   <!DOCTYPE html>
   <html lang="en">
     <head>
       <meta charset<"utf-8">
       <title>Issue #656-1</title>
     </head>
     <body>
       <h1>Issue #656-1</h1>
     </body>
   </html>

   Certainly applying my patch fixes the problem.

   Of course the tidy generated html, after finding that extra < is not very pretty, and including a <meta... with no value is not good, but will leave those as separate issues to maybe be raised. Seems the Segmentation Fault is fixed...

   Hope you can get a chance to try it and confirm... thanks...
8. carnil commented on Dec 14, 2017

   

   carnil

   on Dec 14, 2017

   More actions

   FTR, this issue has been assigned CVE-2017-17497.
9. geoffmcl

   added a commit that references this issue on Dec 16, 2017

   Issue #656 - protect against NULL node set in loop

   a0414d6
10. geoffmcl commented on Dec 16, 2017

    

    geoffmcl

    on Dec 16, 2017

    Contributor

    More actions

    Create PR #661 for testing and review... thanks...
11. geoffmcl

    added a commit that references this issue on Jan 1, 2018

    Merge pull request #661 from htacg/issue-656

    ...

    a111d7a
12. geoffmcl

    closed this as completedin a111d7aon Jan 1, 2018
13. geoffmcl

    added a commit that references this issue on Jan 1, 2018

    Is #656 PR #661 - Bump to 5.7.1 for this merge

    5408bb7