Validate array lengths and token counts when parsing ROFF files (GHSA…

.github/workflows/build-and-test.yml

@@ -73,7 +73,7 @@ jobs:
             build_type: "Debug", cc: "clang", cxx: "clang++"
           }
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v5
 
     - name: Configure CMake
       run: cmake -B ${{github.workspace}}/build -DCMAKE_BUILD_TYPE=${{ matrix.config.build_type }}

.github/workflows/clang-format.yml

@@ -11,12 +11,12 @@ jobs:
           sudo apt-get update
           sudo apt install clang-format-15
           clang-format --version
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v5
       - name: Check format
         run: |
           find -name *.h -o -name *.cpp -o -name *.inl | xargs clang-format -i
           git diff
-      - uses: peter-evans/create-pull-request@v4
+      - uses: peter-evans/create-pull-request@v8
         with:
           token: ${{ secrets.GITHUB_TOKEN }}
           commit-message: "Fixes by clang-format"

src/BinaryTokenizer.cpp

@@ -211,6 +211,8 @@ std::vector<Token> BinaryTokenizer::tokenizeArrayTagKey( std::istream& stream )
     int  numElements = parseInt( numElementsToken.value(), stream );
     stream.seekg( streamPos );
 
+    if ( numElements < 0 ) throw std::runtime_error( "Invalid array length." );
+
     std::vector<Token> arrayTokens = tokenizeArrayData( stream, numElements, typeToken.kind() );
     tokens.insert( tokens.end(), arrayTokens.begin(), arrayTokens.end() );
     return tokens;

src/Parser.cpp

@@ -54,17 +54,25 @@ void Parser::parse( std::istream&                                     stream,
     auto        it           = tokens.begin();
     std::string tagGroupName = "";
     std::string lastName     = "";
+
+    // Step to the next token, failing instead of moving past the end of the token list
+    auto next = [&it, &tokens]()
+    {
+        if ( std::distance( it, tokens.end() ) <= 1 ) throw std::runtime_error( "Unexpected end of file." );
+        std::advance( it, 1 );
+    };
+
     while ( it != tokens.end() )
     {
         if ( it->kind() == Token::Kind::TAG )
         {
-            std::advance( it, 1 );
+            next();
             assert( it->kind() == Token::Kind::NAME );
             tagGroupName = parseString( *it, stream );
         }
         else if ( isSimpleType( it->kind() ) )
         {
-            auto simpleType = parseSimpleType( it, tagGroupName, stream );
+            auto simpleType = parseSimpleType( it, tokens.end(), tagGroupName, stream );
             if ( simpleType.first == "parameter.name" )
             {
                 lastName = std::get<std::string>( simpleType.second );
@@ -81,12 +89,12 @@ void Parser::parse( std::istream&                                     stream,
         else if ( it->kind() == Token::Kind::ARRAY )
         {
             // Extract the type
-            std::advance( it, 1 );
+            next();
             Token typeToken = *it;
             assert( isSimpleType( typeToken.kind() ) );
 
             // Extract the name
-            std::advance( it, 1 );
+            next();
             std::string name = tagGroupName + "." + parseString( *it, stream );
 
             // Special handling for parameter tags.
@@ -107,18 +115,38 @@ void Parser::parse( std::istream&                                     stream,
             }
 
             // Extract number of array items
-            std::advance( it, 1 );
+            next();
             long arrayLength = parseInt( *it, stream );
+            if ( arrayLength < 0 ) throw std::runtime_error( "Invalid array length." );
+
+            // The index of the first item
+            long startIndex = static_cast<long>( std::distance( tokens.begin(), it ) ) + 1;
 
-            // Move to the first item to get the index
-            std::advance( it, 1 );
-            long startIndex = static_cast<long>( std::distance( tokens.begin(), it ) );
+            // The array length is read from the file and must be backed by actual tokens
+            const long tokensAfterLength = static_cast<long>( std::distance( it, tokens.end() ) ) - 1;
+            const bool isBlob            = tokensAfterLength > 0 && ( it + 1 )->kind() == Token::Kind::ARRAYBLOB;
 
-            if ( it->kind() != Token::Kind::ARRAYBLOB )
+            if ( isBlob )
             {
-                // Skip rest of the array when not blob.
-                // The blob is always only one token and is skip already.
-                std::advance( it, arrayLength - 1 );
+                // The blob is always only one token
+                std::advance( it, 1 );
+
+                // The data of the blob must be present in the file
+                stream.clear();
+                const auto currentPos = stream.tellg();
+                stream.seekg( 0, std::ios::end );
+                const auto streamSize = stream.tellg();
+                stream.seekg( currentPos );
+                if ( it->end() > static_cast<size_t>( streamSize ) )
+                    throw std::runtime_error( "Array data is incomplete." );
+            }
+            else
+            {
+                if ( arrayLength > tokensAfterLength )
+                    throw std::runtime_error( "Array length exceeds the number of items in the file." );
+
+                // Skip rest of the array
+                std::advance( it, arrayLength );
             }
 
             arrayInfo[name] = std::make_pair( startIndex, arrayLength );
@@ -146,12 +174,15 @@ bool Parser::isSimpleType( Token::Kind kind )
 ///
 //--------------------------------------------------------------------------------------------------
 std::pair<std::string, RoffScalar> Parser::parseSimpleType( std::vector<Token>::const_iterator& it,
+                                                            std::vector<Token>::const_iterator  end,
                                                             const std::string&                  tagGroupName,
                                                             std::istream&                       stream ) const
 {
     assert( isSimpleType( it->kind() ) );
     Token::Kind kind = it->kind();
 
+    if ( std::distance( it, end ) <= 2 ) throw std::runtime_error( "Unexpected end of file." );
+
     std::advance( it, 1 );
     assert( it->kind() == Token::Kind::NAME );
     std::string name = parseString( *it, stream );

src/Parser.hpp

@@ -42,6 +42,7 @@ class Parser
                 std::map<std::string, std::pair<long, long>>&     arrayInfo ) const;
 
     std::pair<std::string, RoffScalar> parseSimpleType( std::vector<Token>::const_iterator& it,
+                                                        std::vector<Token>::const_iterator  end,
                                                         const std::string&                  tagGroupName,
                                                         std::istream&                       stream ) const;
 

tests/ReaderTests.cpp

@@ -20,6 +20,7 @@
 
 #include <algorithm>
 #include <fstream>
+#include <sstream>
 #include <string>
 #include <variant>
 
@@ -310,3 +311,72 @@ TEST( ReaderTests, testReadWrongFileType )
     }
     ASSERT_EQ( errMsg, std::string( "Unexpected file type." ) );
 }
+
+//--------------------------------------------------------------------------------------------------
+/// The declared array length is larger than the number of items in the file. Used to read outside
+/// the token list.
+//--------------------------------------------------------------------------------------------------
+TEST( ReaderTests, testParseArrayLengthLargerThanContent )
+{
+    auto makeFile = []( const std::string& codeNamesLength )
+    {
+        return "roff-asc\n"
+               "#ROFF file#\n"
+               "tag filedata\n"
+               "int byteswaptest 1\n"
+               "endtag\n"
+               "tag parameter\n"
+               "char name  \"composite\"\n"
+               "array char codeNames " +
+               codeNamesLength +
+               "\n"
+               " \"code name 1\"\n"
+               " \"code name 2\"\n"
+               "array int codeValues 2\n"
+               " 0 1\n"
+               "endtag\n"
+               "tag eof\n"
+               "endtag\n";
+    };
+
+    {
+        std::istringstream stream( makeFile( "2" ) );
+        Reader             reader( stream );
+        reader.parse();
+        EXPECT_EQ( 2u, reader.getArrayLength( "composite.codeNames" ) );
+    }
+
+    for ( const std::string length : { "100000", "-1" } )
+    {
+        std::istringstream stream( makeFile( length ) );
+        Reader             reader( stream );
+        EXPECT_THROW( reader.parse(), std::runtime_error ) << "Array length " << length;
+    }
+}
+
+//--------------------------------------------------------------------------------------------------
+/// Files ending in the middle of a declaration used to read outside the token list.
+//--------------------------------------------------------------------------------------------------
+TEST( ReaderTests, testParseTruncatedFile )
+{
+    for ( const std::string& content : { std::string( "roff-asc\n#ROFF file#\ntag" ),
+                                         std::string( "roff-asc\n#ROFF file#\ntag filedata\nint" ),
+                                         std::string( "roff-asc\n#ROFF file#\ntag filedata\nint byteswaptest" ),
+                                         std::string( "roff-asc\n#ROFF file#\ntag a\narray" ),
+                                         std::string( "roff-asc\n#ROFF file#\ntag a\narray int" ),
+                                         std::string( "roff-asc\n#ROFF file#\ntag a\narray int values" ),
+                                         std::string( "roff-asc\n#ROFF file#\ntag a\narray int values 3" ),
+                                         std::string( "roff-asc\n#ROFF file#\ntag a\narray int values 3\n 1 2" ) } )
+    {
+        std::istringstream stream( content );
+        Reader             reader( stream );
+        try
+        {
+            reader.parse();
+        }
+        catch ( std::runtime_error& )
+        {
+            // Rejecting the file is fine, reading outside the tokens is not
+        }
+    }
+}