CVE-2022-29181: Improper Handling of Unexpected Data Type (CWE-241)
value: Nokogiri is an open source XML and HTML library for Ruby. Nokogiri prior to version 1.13.6 does not type-check all inputs into the XML and HTML4 SAX parsers, allowing specially crafted untrusted inputs to cause illegal memory access errors (segfault) or reads from unrelated memory. Version 1.13.6 contains a patch for this issue. As a workaround, ensure the untrusted input is a `String` by calling `#to_s` or equivalent.
CWE description: The product does not handle or incorrectly handles when a particular element is not the expected type, e.g. it expects a digit (0-9) but is provided with a letter (A-Z).
CVSS v3 base score: 8.2
Published: 2022-05-20T19:15Z
Repository: https://github.com/sparklemotion/nokogiri
Language: Ruby
Files changed: CHANGELOG.md, ext/java/nokogiri/Html4SaxParserContext.java, ext/java/nokogiri/XmlSaxParserContext.java, ext/java/nokogiri/internals/ParserContext.java, ext/nokogiri/html4_sax_parser_context.c, ext/nokogiri/xml_sax_parser_context.c, lib/nokogiri/html4/sax/parser.rb, test/html4/sax/test_parser.rb, test/html4/sax/test_parser_context.rb, test/xml/sax/test_parser.rb, test/xml/sax/test_parser_context.rb
Commit date: 2022-05-06 21:59:33 -0400
Commit message: fix: {HTML4,XML}::SAX::{Parser,ParserContext} check arg types

Previously, arguments of the wrong type might cause segfault on CRuby.

Diff: diff --git a/CHANGELOG.md b/CHANGELOG.md
index 60965d602..61bb5588b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -26,19 +26,20 @@ This version of Nokogiri uses [`jar-dependencies`](https://github.com/mkristian/
 
 ### Fixed
 
 * [CRuby] UTF-16-encoded documents longer than ~4000 code points now serialize properly. Previously the serialized document was corrupted when it exceeded the length of libxml2's internal string buffer. [[#752](https://github.com/sparklemotion/nokogiri/issues/752)]
 * [HTML5] The Gumbo parser now correctly handles text at the end of `form` elements.
+* `{HTML4,XML}::SAX::{Parser,ParserContext}` constructor methods now raise `TypeError` instead of segfaulting when an incorrect type is passed. (Thanks to [@agustingianni](https://github.com/agustingianni) from the Github Security Lab for reporting!)
 
 
 ### Improved
 
 * Compare `Encoding` objects rather than compare their names. This is a slight performance improvement and is future-proof. [[#2454](https://github.com/sparklemotion/nokogiri/issues/2454)] (Thanks, [@casperisfine](https://github.com/casperisfine)!)
 * Avoid compile-time conflict with system-installed `gumbo.h` on OpenBSD. [[#2464](https://github.com/sparklemotion/nokogiri/issues/2464)]
 * Remove calls to `vasprintf` in favor of platform-independent `rb_vsprintf`
 * Prefer `ruby_xmalloc` to `malloc` within the C extension. [[#2480](https://github.com/sparklemotion/nokogiri/issues/2480)] (Thanks, [@Garfield96](https://github.com/Garfield96)!)
-* Installation from source on systems missing libiconv will once again generate a helpful error message (broken since v1.11.0). [#2505]
+* Installation from source on systems missing libiconv will once again generate a helpful error message (broken since v1.11.0). [[#2505](https://github.com/sparklemotion/nokogiri/issues/2505)]
 
 
 ## 1.13.5 / 2022-05-04
 
 ### Security
diff --git a/ext/java/nokogiri/Html4SaxParserContext.java b/ext/java/nokogiri/Html4SaxParserContext.java
index 7c6f1aba1..2e8a3d272 100644
--- a/ext/java/nokogiri/Html4SaxParserContext.java
+++ b/ext/java/nokogiri/Html4SaxParserContext.java
@@ -229,10 +229,17 @@ public class Html4SaxParserContext extends XmlSaxParserContext
   parse_file(ThreadContext context,
              IRubyObject klass,
              IRubyObject data,
              IRubyObject encoding)
   {
+    if (!(data instanceof RubyString)) {
+      throw context.getRuntime().newTypeError("data must be kind_of String");
+    }
+    if (!(encoding instanceof RubyString)) {
+      throw context.getRuntime().newTypeError("data must be kind_of String");
+    }
+
     Html4SaxParserContext ctx = Html4SaxParserContext.newInstance(context.runtime, (RubyClass) klass);
     ctx.setInputSourceFile(context, data);
     String javaEncoding = findEncodingName(context, encoding);
     if (javaEncoding != null) {
       ctx.getInputSource().setEncoding(javaEncoding);
@@ -245,10 +252,14 @@ public class Html4SaxParserContext extends XmlSaxParserContext
   parse_io(ThreadContext context,
            IRubyObject klass,
            IRubyObject data,
            IRubyObject encoding)
   {
+    if (!(encoding instanceof RubyFixnum)) {
+      throw context.getRuntime().newTypeError("encoding must be kind_of String");
+    }
+
     Html4SaxParserContext ctx = Html4SaxParserContext.newInstance(context.runtime, (RubyClass) klass);
     ctx.setIOInputSource(context, data, context.nil);
     String javaEncoding = findEncodingName(context, encoding);
     if (javaEncoding != null) {
       ctx.getInputSource().setEncoding(javaEncoding);
diff --git a/ext/java/nokogiri/XmlSaxParserContext.java b/ext/java/nokogiri/XmlSaxParserContext.java
index 05c7cfea5..53ea7383b 100644
--- a/ext/java/nokogiri/XmlSaxParserContext.java
+++ b/ext/java/nokogiri/XmlSaxParserContext.java
@@ -129,13 +129,16 @@ public class XmlSaxParserContext extends ParserContext
   @JRubyMethod(name = "io", meta = true)
   public static IRubyObject
   parse_io(ThreadContext context,
            IRubyObject klazz,
            IRubyObject data,
-           IRubyObject enc)
+           IRubyObject encoding)
   {
-    //int encoding = (int)enc.convertToInteger().getLongValue();
+    // check the type of the unused encoding to match behavior of CRuby
+    if (!(encoding instanceof RubyFixnum)) {
+      throw context.getRuntime().newTypeError("encoding must be kind_of String");
+    }
     final Ruby runtime = context.runtime;
     XmlSaxParserContext ctx = newInstance(runtime, (RubyClass) klazz);
     ctx.initialize(runtime);
     ctx.setIOInputSource(context, data, runtime.getNil());
     return ctx;
diff --git a/ext/java/nokogiri/internals/ParserContext.java b/ext/java/nokogiri/internals/ParserContext.java
index 8c8a7749b..27fa835dc 100644
--- a/ext/java/nokogiri/internals/ParserContext.java
+++ b/ext/java/nokogiri/internals/ParserContext.java
@@ -58,10 +58,16 @@ public abstract class ParserContext extends RubyObject
   setIOInputSource(ThreadContext context, IRubyObject data, IRubyObject url)
   {
     source = new InputSource();
     ParserContext.setUrl(context, source, url);
 
+    Ruby ruby = context.getRuntime();
+
+    if (!(data.respondsTo("read"))) {
+      throw ruby.newTypeError("must respond to :read");
+    }
+
     source.setByteStream(new IOInputStream(data));
     if (java_encoding != null) {
       source.setEncoding(java_encoding);
     }
   }
@@ -73,11 +79,11 @@ public abstract class ParserContext extends RubyObject
     ParserContext.setUrl(context, source, url);
 
     Ruby ruby = context.getRuntime();
 
     if (!(data instanceof RubyString)) {
-      throw ruby.newArgumentError("must be kind_of String");
+      throw ruby.newTypeError("must be kind_of String");
     }
 
     RubyString stringData = (RubyString) data;
 
     if (stringData.encoding(context) != null) {
diff --git a/ext/nokogiri/html4_sax_parser_context.c b/ext/nokogiri/html4_sax_parser_context.c
index dec0d88fe..54adca4cb 100644
--- a/ext/nokogiri/html4_sax_parser_context.c
+++ b/ext/nokogiri/html4_sax_parser_context.c
@@ -17,13 +17,12 @@ deallocate(xmlParserCtxtPtr ctxt)
 static VALUE
 parse_memory(VALUE klass, VALUE data, VALUE encoding)
 {
   htmlParserCtxtPtr ctxt;
 
-  if (NIL_P(data)) {
-    rb_raise(rb_eArgError, "data cannot be nil");
-  }
+  Check_Type(data, T_STRING);
+
   if (!(int)RSTRING_LEN(data)) {
     rb_raise(rb_eRuntimeError, "data cannot be empty");
   }
 
   ctxt = htmlCreateMemoryParserCtxt(StringValuePtr(data),
diff --git a/ext/nokogiri/xml_sax_parser_context.c b/ext/nokogiri/xml_sax_parser_context.c
index 60449347a..60c491984 100644
--- a/ext/nokogiri/xml_sax_parser_context.c
+++ b/ext/nokogiri/xml_sax_parser_context.c
@@ -1,9 +1,11 @@
 #include <nokogiri.h>
 
 VALUE cNokogiriXmlSaxParserContext ;
 
+static ID id_read;
+
 static void
 deallocate(xmlParserCtxtPtr ctxt)
 {
   NOKOGIRI_DEBUG_START(ctxt);
 
@@ -24,10 +26,14 @@ static VALUE
 parse_io(VALUE klass, VALUE io, VALUE encoding)
 {
   xmlParserCtxtPtr ctxt;
   xmlCharEncoding enc = (xmlCharEncoding)NUM2INT(encoding);
 
+  if (!rb_respond_to(io, id_read)) {
+    rb_raise(rb_eTypeError, "argument expected to respond to :read");
+  }
+
   ctxt = xmlCreateIOParserCtxt(NULL, NULL,
                                (xmlInputReadCallback)noko_io_read,
                                (xmlInputCloseCallback)noko_io_close,
                                (void *)io, enc);
   if (ctxt->sax) {
@@ -60,13 +66,12 @@ parse_file(VALUE klass, VALUE filename)
 static VALUE
 parse_memory(VALUE klass, VALUE data)
 {
   xmlParserCtxtPtr ctxt;
 
-  if (NIL_P(data)) {
-    rb_raise(rb_eArgError, "data cannot be nil");
-  }
+  Check_Type(data, T_STRING);
+
   if (!(int)RSTRING_LEN(data)) {
     rb_raise(rb_eRuntimeError, "data cannot be empty");
   }
 
   ctxt = xmlCreateMemoryParserCtxt(StringValuePtr(data),
@@ -276,6 +281,8 @@ noko_init_xml_sax_parser_context()
   rb_define_method(cNokogiriXmlSaxParserContext, "replace_entities", get_replace_entities, 0);
   rb_define_method(cNokogiriXmlSaxParserContext, "recovery=", set_recovery, 1);
   rb_define_method(cNokogiriXmlSaxParserContext, "recovery", get_recovery, 0);
   rb_define_method(cNokogiriXmlSaxParserContext, "line", line, 0);
   rb_define_method(cNokogiriXmlSaxParserContext, "column", column, 0);
+
+  id_read = rb_intern("read");
 }
diff --git a/lib/nokogiri/html4/sax/parser.rb b/lib/nokogiri/html4/sax/parser.rb
index 2a074b6b9..1f8c03af6 100644
--- a/lib/nokogiri/html4/sax/parser.rb
+++ b/lib/nokogiri/html4/sax/parser.rb
@@ -26,11 +26,11 @@ module Nokogiri
       # For more information on SAX parsers, see Nokogiri::XML::SAX
       class Parser < Nokogiri::XML::SAX::Parser
         ###
         # Parse html stored in +data+ using +encoding+
         def parse_memory(data, encoding = "UTF-8")
-          raise ArgumentError unless data
+          raise TypeError unless String === data
           return if data.empty?
 
           ctx = ParserContext.memory(data, encoding)
           yield ctx if block_given?
           ctx.parse_with(self)
diff --git a/test/html4/sax/test_parser.rb b/test/html4/sax/test_parser.rb
index 3917dce2e..4dc6ac61f 100644
--- a/test/html4/sax/test_parser.rb
+++ b/test/html4/sax/test_parser.rb
@@ -52,11 +52,11 @@ module Nokogiri
             @parser.parse_file(File.dirname(__FILE__))
           end
         end
 
         def test_parse_memory_nil
-          assert_raises(ArgumentError) do
+          assert_raises(TypeError) do
             @parser.parse_memory(nil)
           end
         end
 
         def test_parse_force_encoding
@@ -159,9 +159,15 @@ module Nokogiri
         end
 
         def test_empty_processing_instruction
           @parser.parse_memory("<strong>this will segfault<?strong>")
         end
+
+        it "handles invalid types gracefully" do
+          assert_raises(TypeError) { Nokogiri::HTML::SAX::Parser.new.parse(0xcafecafe) }
+          assert_raises(TypeError) { Nokogiri::HTML::SAX::Parser.new.parse_memory(0xcafecafe) }
+          assert_raises(TypeError) { Nokogiri::HTML::SAX::Parser.new.parse_io(0xcafecafe) }
+        end
       end
     end
   end
 end
diff --git a/test/html4/sax/test_parser_context.rb b/test/html4/sax/test_parser_context.rb
index 0a0eda64c..594a3ac82 100644
--- a/test/html4/sax/test_parser_context.rb
+++ b/test/html4/sax/test_parser_context.rb
@@ -38,9 +38,18 @@ module Nokogiri
           ctx = ParserContext.file(HTML_FILE, "UTF-8")
           parser = Parser.new(Doc.new)
           ctx.parse_with(parser)
           # end
         end
+
+        def test_graceful_handling_of_invalid_types
+          assert_raises(TypeError) { ParserContext.new(0xcafecafe) }
+          assert_raises(TypeError) { ParserContext.memory(0xcafecafe, "UTF-8") }
+          assert_raises(TypeError) { ParserContext.io(0xcafecafe, 1) }
+          assert_raises(TypeError) { ParserContext.io(StringIO.new("asdf"), "should be an index into ENCODINGS") }
+          assert_raises(TypeError) { ParserContext.file(0xcafecafe, "UTF-8") }
+          assert_raises(TypeError) { ParserContext.file("path/to/file", 0xcafecafe) }
+        end
       end
     end
   end
 end
diff --git a/test/xml/sax/test_parser.rb b/test/xml/sax/test_parser.rb
index 6c0f1fc88..ac7d4ead0 100644
--- a/test/xml/sax/test_parser.rb
+++ b/test/xml/sax/test_parser.rb
@@ -71,10 +71,16 @@ module Nokogiri
           assert_raises(RuntimeError) do
             parser.parse("")
           end
         end
 
+        it "handles invalid types gracefully" do
+          assert_raises(TypeError) { Nokogiri::XML::SAX::Parser.new.parse(0xcafecafe) }
+          assert_raises(TypeError) { Nokogiri::XML::SAX::Parser.new.parse_memory(0xcafecafe) }
+          assert_raises(TypeError) { Nokogiri::XML::SAX::Parser.new.parse_io(0xcafecafe) }
+        end
+
         it :test_namespace_declaration_order_is_saved do
           parser.parse(<<~EOF)
             <root xmlns:foo='http://foo.example.com/' xmlns='http://example.com/'>
               <a foo:bar='hello' />
             </root>
@@ -261,11 +267,11 @@ module Nokogiri
             parser.parse_file(File.expand_path(File.dirname(__FILE__)))
           end
         end
 
         it :test_render_parse_nil_param do
-          assert_raises(ArgumentError) { parser.parse_memory(nil) }
+          assert_raises(TypeError) { parser.parse_memory(nil) }
         end
 
         it :test_bad_encoding_args do
           assert_raises(ArgumentError) { Nokogiri::XML::SAX::Parser.new(Doc.new, "not an encoding") }
           assert_raises(ArgumentError) { parser.parse_io(StringIO.new("<root/>"), "not an encoding") }
diff --git a/test/xml/sax/test_parser_context.rb b/test/xml/sax/test_parser_context.rb
index e38efd271..ba9857943 100644
--- a/test/xml/sax/test_parser_context.rb
+++ b/test/xml/sax/test_parser_context.rb
@@ -78,10 +78,17 @@ module Nokogiri
 
           pc.recovery = true
           assert(pc.recovery)
         end
 
+        def test_graceful_handling_of_invalid_types
+          assert_raises(TypeError) { ParserContext.new(0xcafecafe) }
+          assert_raises(TypeError) { ParserContext.memory(0xcafecafe) }
+          assert_raises(TypeError) { ParserContext.io(0xcafecafe, 1) }
+          assert_raises(TypeError) { ParserContext.io(StringIO.new("asdf"), "should be an index into ENCODINGS") }
+        end
+
         def test_from_io
           ctx = ParserContext.new(StringIO.new("fo"), "UTF-8")
           assert(ctx)
         end
 

Vulnerable code: * Installation from source on systems missing libiconv will once again generate a helpful error message (broken since v1.11.0). [#2505]
           IRubyObject enc)
    //int encoding = (int)enc.convertToInteger().getLongValue();
      throw ruby.newArgumentError("must be kind_of String");
  if (NIL_P(data)) {
    rb_raise(rb_eArgError, "data cannot be nil");
  }
  if (NIL_P(data)) {
    rb_raise(rb_eArgError, "data cannot be nil");
  }
          raise ArgumentError unless data
          assert_raises(ArgumentError) do
          assert_raises(ArgumentError) { parser.parse_memory(nil) }
Fixed code: * `{HTML4,XML}::SAX::{Parser,ParserContext}` constructor methods now raise `TypeError` instead of segfaulting when an incorrect type is passed. (Thanks to [@agustingianni](https://github.com/agustingianni) from the Github Security Lab for reporting!)
* Installation from source on systems missing libiconv will once again generate a helpful error message (broken since v1.11.0). [[#2505](https://github.com/sparklemotion/nokogiri/issues/2505)]
    if (!(data instanceof RubyString)) {
      throw context.getRuntime().newTypeError("data must be kind_of String");
    }
    if (!(encoding instanceof RubyString)) {
      throw context.getRuntime().newTypeError("data must be kind_of String");
    }

    if (!(encoding instanceof RubyFixnum)) {
      throw context.getRuntime().newTypeError("encoding must be kind_of String");
    }

           IRubyObject encoding)
    // check the type of the unused encoding to match behavior of CRuby
    if (!(encoding instanceof RubyFixnum)) {
      throw context.getRuntime().newTypeError("encoding must be kind_of String");
    }
    Ruby ruby = context.getRuntime();

    if (!(data.respondsTo("read"))) {
      throw ruby.newTypeError("must respond to :read");
    }

      throw ruby.newTypeError("must be kind_of String");
  Check_Type(data, T_STRING);

static ID id_read;

  if (!rb_respond_to(io, id_read)) {
    rb_raise(rb_eTypeError, "argument expected to respond to :read");
  }

  Check_Type(data, T_STRING);


  id_read = rb_intern("read");
          raise TypeError unless String === data
          assert_raises(TypeError) do

        it "handles invalid types gracefully" do
          assert_raises(TypeError) { Nokogiri::HTML::SAX::Parser.new.parse(0xcafecafe) }
          assert_raises(TypeError) { Nokogiri::HTML::SAX::Parser.new.parse_memory(0xcafecafe) }
          assert_raises(TypeError) { Nokogiri::HTML::SAX::Parser.new.parse_io(0xcafecafe) }
        end

        def test_graceful_handling_of_invalid_types
          assert_raises(TypeError) { ParserContext.new(0xcafecafe) }
          assert_raises(TypeError) { ParserContext.memory(0xcafecafe, "UTF-8") }
          assert_raises(TypeError) { ParserContext.io(0xcafecafe, 1) }
          assert_raises(TypeError) { ParserContext.io(StringIO.new("asdf"), "should be an index into ENCODINGS") }
          assert_raises(TypeError) { ParserContext.file(0xcafecafe, "UTF-8") }
          assert_raises(TypeError) { ParserContext.file("path/to/file", 0xcafecafe) }
        end
        it "handles invalid types gracefully" do
          assert_raises(TypeError) { Nokogiri::XML::SAX::Parser.new.parse(0xcafecafe) }
          assert_raises(TypeError) { Nokogiri::XML::SAX::Parser.new.parse_memory(0xcafecafe) }
          assert_raises(TypeError) { Nokogiri::XML::SAX::Parser.new.parse_io(0xcafecafe) }
        end

          assert_raises(TypeError) { parser.parse_memory(nil) }
        def test_graceful_handling_of_invalid_types
          assert_raises(TypeError) { ParserContext.new(0xcafecafe) }
          assert_raises(TypeError) { ParserContext.memory(0xcafecafe) }
          assert_raises(TypeError) { ParserContext.io(0xcafecafe, 1) }
          assert_raises(TypeError) { ParserContext.io(StringIO.new("asdf"), "should be an index into ENCODINGS") }
        end