Uploaded image for project: 'Puppet'
  1. Puppet
  2. PUP-10254

Ensure Windows wide character strings have a wide terminator

    XMLWordPrintable

Details

    • Bug
    • Status: Closed
    • Normal
    • Resolution: Fixed
    • None
    • PUP 5.5.21, PUP 6.17.0
    • None
    • Coremunity
    • Platform Core KANBAN
    • Needs Assessment
    • Bug Fix
    • Improves memory safety when puppet converts ruby strings to wide character strings on Windows.
    • Needs Assessment

    Description

      Prior to ruby 2.1.0, it was possible for multibyte strings to have only a single null terminator, followed by garbage which could cause buffer overruns. This was fixed in https://github.com/ruby/ruby/commit/1549894a0699abdfab1fbba0e468afd3f2f990b3, but never backported to 1.9.3.

      We added code to puppet to protect against this. See https://github.com/puppetlabs/puppet/commit/cabd95cbd06ac3d834e0e401bcb07c2878a0b0f2 and https://github.com/puppetlabs/puppet/commit/b7c10f5d045af5890f83159b74ab5d5a23e0abc9

      However, puppet's from_string_to_wide_string copies the wide string from the ruby heap to the native heap using FFI::MemoryPointer#put_array_of_uchar. But it does not explicitly double null terminate the resulting wide string pointer. For example, here's a string of character length 1 and byte length 2.

      irb(main):025:0> str = "a".encode("UTF-16LE")
      => "a"
      irb(main):026:0> str.bytes.length
      => 2
      irb(main):027:0> str.length
      => 1
      

      When copying from the ruby heap to the native heap, we get a pointer to memory of length 2 bytes, which doesn't have room for the double null:

      irb(main):028:0> ptr = FFI::MemoryPointer.new(:byte, str.bytes.length)
      => #<FFI::MemoryPointer address=0x0000000003aea340 size=2>
      irb(main):029:0> ptr.total
      => 2
      

      This hasn't been an issue because Puppet::Util::Windows::String.wide_string embeds a double null in the ruby string:

      irb(main):030:0> str = Puppet::Util::Windows::String.wide_string("a")
      => "a\u0000"
      irb(main):031:0> str.bytes.length
      => 4
      irb(main):032:0> str.length
      => 2
      

      We should always explicitly double null terminate the wide string in the native heap, so as to not rely on the ruby workaround.

      Attachments

        Issue Links

          Activity

            People

              josh Josh Cooper
              josh Josh Cooper
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Zendesk Support