Inactive unit tests hide endless loop in MXParser

[joehni]

A bunch of unit tests of org.codehaus.plexus.util.xml.pull.MXParserTest are inactive, because they lack the@Test annotation. However, if you activate them, some will result in an endless loop (DoS attack !). The problem had actually been solved once by Gabrial Belingues (in 7e54bff), but was introduced again by Todd Lipcon (in 59cf121) and overseen because the tests did not execute. The change also re-introduced codehaus-plexus/plexus-xml#7.

[michael-o]

Interesting...

[belingueres]

Good catch! I'll send a quick fix in these days.

[joehni]

The handling of the XML in the processing instruction unfortunately still opens a hole for the DoS attack with an endless loop:

    @Test
    public void testMalformedProcessingInstructionsContainingXmlNoClosingQuestionMark()
        throws Exception
    {
        StringBuffer sb = new StringBuffer();
        sb.append( "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" );
        sb.append( "<project />\n" );
        sb.append( "<?pi\n" );
        sb.append( "   <tag>\n" );
        sb.append( "   </tag>>\n" );

        Xpp3Parser parser = new Xpp3Parser();
        parser.setInput( new StringReader( sb.toString() ) );

        try
        {
            assertEquals( XmlPullParser.PROCESSING_INSTRUCTION, parser.nextToken() );
            assertEquals( XmlPullParser.IGNORABLE_WHITESPACE, parser.nextToken() );
            assertEquals( XmlPullParser.START_TAG, parser.nextToken() );
            assertEquals( XmlPullParser.END_TAG, parser.nextToken() );
            assertEquals( XmlPullParser.IGNORABLE_WHITESPACE, parser.nextToken() );
            assertEquals( XmlPullParser.PROCESSING_INSTRUCTION, parser.nextToken() );
            
            fail( "Should fail since it has invalid PI" );
        }
        catch ( XmlPullParserException ex )
        {
            assertTrue( ex.getMessage().contains( "processing instruction started on line 3 and column 3 was not closed" ) );
        }
    }

Your change throws the exception for the invalid PI only if seenInnerTag is true, but you should add an additional else part there and set the value again to false.

BTW: The columnNumber is wrongly set to 0 in reset(). This affects other unit tests, but either column 3 is wrong here or those tests with a column value for line 1.

[belingueres]

Seems there are still several (unrelated?) issues to fix. Could you open new, independent issues explaining the problems you just found and their expected behavior?

[belingueres]

Hi @joehni !
I've created a project to test the MXParser against the XML conformance test suite. I think it is a good starting point for parser improvement for the future: https://github.com/belingueres/xml-conformance